Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
e7e1214 to0a57dc6Compare0a57dc6 toeaf960bComparesroze commentedMay 1, 2018
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 from
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 commentedMay 1, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@sroze we've the same point, that's the goal of this PR: improving DX with orwithout AMQP requirements as AMQP is optional, right?
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 by
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 commentedMay 1, 2018
That was precisely my inspiration to create this PR :) |
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 commentedMay 2, 2018
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 Actually there is many places and features inside Symfony that are enabled/disabled according to req installation, such as |
sroze commentedMay 2, 2018
I'm happy with disabling it by defaultbut we need a clearer message than this one:
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) |
| 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.'); |
There was a problem hiding this comment.
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".)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
yceruto commentedMay 3, 2018
@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 commentedMay 4, 2018
Rebase needed after#27129 |
ecad67b tofd8c675Compare| </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"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Renaming according to#27129
yceruto commentedMay 4, 2018
Rebased again after#27150 |
yceruto commentedMay 5, 2018
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".'); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed to-pack.
yceruto commentedMay 6, 2018
Status: Needs Review |
fabpot commentedMay 7, 2018
Thank you@yceruto. |
…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
Uh oh!
There was an error while loading.Please reload this page.
After playing a little with the config of this component I found some bugs around.
Reproducer:
^4.0@devdependenciessymfony/messengerrequirementNote that
symfony/serializerhas not been installed yet. ATM it's not required.Configuring my custom transport (not amqp):
Before (Current behaviour):
Threw a logic exception, IMO unrelated at this point:
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:
Before:
The same exception has been thrown, now a bit vague according to the config:
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:
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:
I've removed the "install" part because at this point only we're checking whether the
framework.serializeris 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/serializeris not a hard dependency of this component.By last, I disabled the serializer option manually:
Before:
I received this DI exception:
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!