Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Messenger] Relax messenger config and fix some bugs#27084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromyceruto:relax_messenger_config
May 7, 2018

Conversation

@yceruto
Copy link
Member

@ycerutoyceruto commentedApr 28, 2018
edited
Loading

QA
Branch?master
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

After playing a little with the config of this component I found some bugs around.

Reproducer:

  1. Install a fresh Symfony flex project with^4.0@dev dependencies
  2. Add thesymfony/messenger requirement
  3. Add the following configuration separately:

Note thatsymfony/serializer has not been installed yet. ATM it's not required.


Configuring my custom transport (not amqp):

framework:messenger:transports:custom:'my_transport://localhost/msgs'routing:'*':custom

Before (Current behaviour):
Threw a logic exception, IMO unrelated at this point:

Using the default encoder/decoder, Symfony Messenger requires the Serializer. Enable it or install it by running "composer require symfony/serializer-pack".

After (Proposal):
Pass! The Messenger's serializer config is disabled by definition because the Serializer component is not installed yet, then the related (default) encoder/decoder aliases are invalid, so the amqp transport factory service is removed altogether.


According to the previous exception I configured my custom encoder/decoder services:

framework:messenger:encoder:App\Serializer\Serializerdecoder:App\Serializer\Serializertransports:custom:'my_transport://localhost/msgs'routing:'*':custom

Before:
The same exception has been thrown, now a bit vague according to the config:

Using the default encoder/decoder, Symfony Messenger requires the Serializer. Enable it or install it by running "composer require symfony/serializer-pack".

After:
Pass! the serializer is disabled by definition but there is custom encoder/decoder services, so let's keep the amqp transport factory with their custom encoder/decoder deps.


Just enabling the serializer option:

framework:messenger:serializer:true

Before:
Pass! Unexpected, as there is no transport configuration the exception wasn't thrown and still keeps the amqp transport factory service with invalid encoder/decoder (Serializer) dependencies.

After:
The Serializer config & support is verified if it's enabled regardless of the transport configuration and this exception is thrown for this case:

The default Messenger serializer cannot be enabled as the Serializer support is not enabled.

I've removed the "install" part because at this point only we're checking whether theframework.serializer is enabled or not, so the next step after that should be enable the Serializer support inframework.serializer, which already verify whether the Serializer component is installed or not. IMO "composer require symfony/serializer-pack" should be there and not here. Also becausesymfony/serializer is not a hard dependency of this component.


By last, I disabled the serializer option manually:

framework:messenger:serializer:falsetransports:custom:'my_transport://localhost/msgs'routing:'*':custom

Before:
I received this DI exception:

The service "messenger.transport.amqp.factory" has a dependency on a non-existent service "messenger.transport.serializer".

After:
Pass! (same behaviour that the first example)


As I explained earlier, this PR enables or disables the Messenger's serializer config based on the current Symfony platform and whether the Serializer component is installed or not, like other config with similar behaviour.

Tests included :)

Cheers!

@ycerutoycerutoforce-pushed therelax_messenger_config branch 3 times, most recently frome7e1214 to0a57dc6CompareApril 28, 2018 06:46
@nicolas-grekasnicolas-grekas added this to the4.1 milestoneApr 29, 2018
@ycerutoycerutoforce-pushed therelax_messenger_config branch from0a57dc6 toeaf960bCompareApril 30, 2018 23:57
@sroze
Copy link
Contributor

With these changes, if I am a developer that just get started with Messenger and uses the AMQP examples, here is what I'll have, as an exception labelled as coming fromChainAdapterFactory line 51:

No adapter supports the given DSN "amqp://guest:guest@localhost:5672/%2f/messages

My reaction would be "WTF?!". I would definitely favour having a DX that almost forces you to install Symfony's Serializer than having to Google things out 🙃

@yceruto
Copy link
MemberAuthor

yceruto commentedMay 1, 2018
edited
Loading

@sroze we've the same point, that's the goal of this PR: improving DX with orwithout AMQP requirements as AMQP is optional, right?

No adapter supports the given DSN "amqp://guest:guest@localhost:5672/%2f/messages

It's a good example of what I'm trying to improve here. In that case the Serializer component is not enabled or installed so the AMQP adapter was removed. Are you suggesting check by0 === strpos($dsn, 'amqp://') somewhere to show a better exception message?

My reaction would be "WTF?!". I would definitely favour having a DX that almost forces you to install Symfony's Serializer than having to Google things out

Probably I can react the same if I'm a developer who just started with Messenger and Idon't use the AMQP :P Is there any opinion against what I try to solve? or how has it been solved?

@yceruto
Copy link
MemberAuthor

a DX that almost forces you to install Symfony's Serializer...

That was precisely my inspiration to create this PR :)

@sroze
Copy link
Contributor

sroze commentedMay 1, 2018 via email

I definitely get you on that one :). I guess the key is to answer “how canwe make it more explicit than we need the serializer to be installed whenusing the AMQP adapter?” then. I tried earlier this month to move the checkat runtime but it ended up having the “composer req serializer message”within the Messenger component wamespace which didn’t make sense either.
On Tue, 1 May 2018 at 21:31, Yonel Ceruto ***@***.***> wrote: a DX that almost forces you to install Symfony's Serializer... That was precisely my inspiration to create this PR :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27084 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHEdt-mM8bcMiM7XtqpwH0UeFAXbOQks5tuMYxgaJpZM4TrQz_> .

@yceruto
Copy link
MemberAuthor

How can we make it more explicit than we need the serializer to be installed when using the AMQP adapter?

Well, I'm proposing one option here: disabling the AMQP adapter feature if there is no all dependencies to make it work, then we could add a note aboutcomposer require symfony/serializer into Messenger documentation to enable it if you want to use the built-in AMQP adapter.

Actually there is many places and features inside Symfony that are enabled/disabled according to req installation, such asdoctrine/annotations forserializer androuting components,symfony/security-csrf,symfony/twig-bridge,symfony/translation,symfony/validator for form component andocramius/proxy-manager for lazy services in DI component for just to mention some.

@sroze
Copy link
Contributor

Well, I'm proposing one option here: disabling the AMQP adapter feature if there is no all dependencies to make it work, then we could add a note about composer require symfony/serializer into Messenger documentation to enable it if you want to use the built-in AMQP adapter.

I'm happy with disabling it by defaultbut we need a clearer message than this one:

No adapter supports the given DSN "amqp://guest:guest@localhost:5672/%2f/messages

The trick is to be able to provide one, I'm not sure what's the best approach as I've tried multiple times without great success (see#26908 and#26941)

@ycerutoycerutoforce-pushed therelax_messenger_config branch fromeaf960b to5c595c0CompareMay 3, 2018 13:10

foreach ($config['adapters']as$name =>$adapter) {
if (0 ===strpos($adapter['dsn'],'amqp://') && !$container->hasDefinition('messenger.adapter.amqp.factory')) {
thrownewLogicException('The default AMQP adapter is not available. Make sure you have installed and enabled the Serializer component.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think it's a sensible tradeoff. But we need to propose a next step to the user:(install it by running "composer require symfony/serializer".)

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Done.

@ycerutoycerutoforce-pushed therelax_messenger_config branch from5c595c0 to4edc356CompareMay 3, 2018 13:21
@yceruto
Copy link
MemberAuthor

@sroze displayed a better exception message when you're trying to use AMQP and the default AMQP adapter is not available (either Serializer component is not installed or it isn't enabled).

@nicolas-grekas
Copy link
Member

Rebase needed after#27129

@ycerutoycerutoforce-pushed therelax_messenger_config branch 2 times, most recently fromecad67b tofd8c675CompareMay 4, 2018 15:17
</service>

<serviceid="messenger.adapter.amqp.factory"class="Symfony\Component\Messenger\Transport\AmqpExt\AmqpTransportFactory">
<serviceid="messenger.transport.amqp.factory"class="Symfony\Component\Messenger\Transport\AmqpExt\AmqpTransportFactory">
Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Renaming according to#27129

@ycerutoycerutoforce-pushed therelax_messenger_config branch fromfd8c675 tod836787CompareMay 4, 2018 19:41
@yceruto
Copy link
MemberAuthor

Rebased again after#27150

@yceruto
Copy link
MemberAuthor

Shall we go ahead with this one?


foreach ($config['transports']as$name =>$transport) {
if (0 ===strpos($transport['dsn'],'amqp://') && !$container->hasDefinition('messenger.transport.amqp.factory')) {
thrownewLogicException('The default AMQP transport is not available. Make sure you have installed and enabled the Serializer component. Try enable it or install it by running "composer require symfony/serializer".');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

We need theserializer-pack I think, just the Serializer wouldn't be enough (i.e. missing normalisers)

Copy link
Contributor

@ogizanagiogizanagiMay 6, 2018
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Theserializer-pack does not install any extra normalizer.
But I guess you meant in order for theObjectNormalizer denormalization to work efficiently based on typehints/docblocks, for which the PropertyInfo component andphpdocumentor/reflection-docblock are required.
So 👍 for mentioning the pack, but that's also probably a hint we already miss when just using the Serializer component.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Changed to-pack.

@ycerutoycerutoforce-pushed therelax_messenger_config branch fromd836787 toa05e2e2CompareMay 6, 2018 14:57
@yceruto
Copy link
MemberAuthor

Status: Needs Review

@fabpot
Copy link
Member

Thank you@yceruto.

@fabpotfabpot merged commita05e2e2 intosymfony:masterMay 7, 2018
fabpot added a commit that referenced this pull requestMay 7, 2018
…uto)This PR was merged into the 4.1-dev branch.Discussion----------[Messenger] Relax messenger config and fix some bugs| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -After playing a little with the config of this component I found some bugs around.Reproducer: 1. Install a fresh Symfony flex project with `^4.0@dev` dependencies 2. Add the `symfony/messenger` requirement 3. Add the following configuration separately:Note that `symfony/serializer` has not been installed yet. ATM it's not required.----------------------Configuring my custom transport (not amqp):```yamlframework:    messenger:        transports:            custom: 'my_transport://localhost/msgs'        routing:            '*': custom```**Before**  (Current behaviour):Threw a logic exception, IMO unrelated at this point:> Using the default encoder/decoder, Symfony Messenger requires the Serializer. Enable it or install it by running "composer require symfony/serializer-pack".**After** (Proposal):Pass! The Messenger's serializer config is disabled by definition because the Serializer component is not installed yet, then the related (default) encoder/decoder aliases are invalid, so the amqp transport factory service is removed altogether.----------------------According to the previous exception I configured my custom encoder/decoder services:```yamlframework:    messenger:        encoder: App\Serializer\Serializer        decoder: App\Serializer\Serializer        transports:            custom: 'my_transport://localhost/msgs'        routing:            '*': custom```**Before**:The same exception has been thrown, now a bit vague according to the config:> Using the default encoder/decoder, Symfony Messenger requires the Serializer. Enable it or install it by running "composer require symfony/serializer-pack".**After**:Pass! the serializer is disabled by definition but there is custom encoder/decoder services, so let's keep the amqp transport factory with their custom encoder/decoder deps.----------------------Just enabling the serializer option:```yamlframework:    messenger:        serializer: true```**Before**:Pass! Unexpected, as there is no transport configuration the exception wasn't thrown and still keeps the amqp transport factory service with invalid encoder/decoder (Serializer) dependencies.**After**:The Serializer config & support is verified if it's enabled regardless of the transport configuration and this exception is thrown for this case:> The default Messenger serializer cannot be enabled as the Serializer support is not enabled.I've removed the "install" part because at this point only we're checking whether the `framework.serializer` is enabled or not, so the next step after that should be enable the Serializer support in `framework.serializer`, which already verify whether the Serializer component is installed or not. IMO "composer require symfony/serializer-pack" should be there and not here. Also because `symfony/serializer` is not a hard dependency of this component.----------------------By last, I disabled the serializer option manually:```yamlframework:    messenger:        serializer: false        transports:            custom: 'my_transport://localhost/msgs'        routing:            '*': custom```**Before**:I received this DI exception:> The service "messenger.transport.amqp.factory" has a dependency on a non-existent service "messenger.transport.serializer".**After**:Pass! (same behaviour that the first example)----------------------As I explained earlier, this PR enables or disables the Messenger's serializer config based on the current Symfony platform and whether the Serializer component is installed or not, like other config with similar behaviour.Tests included :)Cheers!Commits-------a05e2e2 Relax Messenger config and fix some bugs
@ycerutoyceruto deleted the relax_messenger_config branchMay 7, 2018 11:18
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

+2 more reviewers

@ogizanagiogizanagiogizanagi left review comments

@srozesrozesroze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

6 participants

@yceruto@sroze@nicolas-grekas@fabpot@ogizanagi@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp