-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e7e1214
to
0a57dc6
Compare
0a57dc6
to
eaf960b
Compare
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 🙃 |
@sroze we've the same point, that's the goal of this PR: improving DX with or without 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 I don't use the AMQP :P Is there any opinion against what I try to solve? or how has it been solved? |
That was precisely my inspiration to create this PR :) |
I definitely get you on that one :). I guess the key is to answer “how can
we make it more explicit than we need the serializer to be installed when
using the AMQP adapter?” then. I tried earlier this month to move the check
at 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_>
.
|
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 |
I'm happy with disabling it by default but 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) |
eaf960b
to
5c595c0
Compare
@@ -1506,6 +1509,10 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder | |||
$container->getDefinition('messenger.asynchronous.routing.sender_locator')->replaceArgument(1, $messageToSenderIdsMapping); | |||
|
|||
foreach ($config['adapters'] as $name => $adapter) { | |||
if (0 === strpos($adapter['dsn'], 'amqp://') && !$container->hasDefinition('messenger.adapter.amqp.factory')) { | |||
throw new LogicException('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.
5c595c0
to
4edc356
Compare
@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). |
Rebase needed after #27129 |
ecad67b
to
fd8c675
Compare
<service id="messenger.transport_factory" class="Symfony\Component\Messenger\Transport\Factory\ChainTransportFactory"> | ||
<argument type="tagged" tag="messenger.transport_factory" /> | ||
</service> | ||
|
||
<service id="messenger.adapter.amqp.factory" class="Symfony\Component\Messenger\Transport\AmqpExt\AmqpTransportFactory"> | ||
<service id="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
fd8c675
to
d836787
Compare
Rebased again after #27150 |
Shall we go ahead with this one? |
@@ -1506,6 +1509,10 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder | |||
$container->getDefinition('messenger.asynchronous.routing.sender_locator')->replaceArgument(1, $messageToSenderIdsMapping); | |||
|
|||
foreach ($config['transports'] as $name => $transport) { | |||
if (0 === strpos($transport['dsn'], 'amqp://') && !$container->hasDefinition('messenger.transport.amqp.factory')) { | |||
throw new LogicException('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 the serializer-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.
The serializer-pack does not install any extra normalizer.
But I guess you meant in order for the ObjectNormalizer denormalization to work efficiently based on typehints/docblocks, for which the PropertyInfo component and phpdocumentor/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
.
d836787
to
a05e2e2
Compare
Status: Needs Review |
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): ```yaml 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: ```yaml framework: 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: ```yaml 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 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: ```yaml framework: 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
After playing a little with the config of this component I found some bugs around.
Reproducer:
^4.0@dev
dependenciessymfony/messenger
requirementNote that
symfony/serializer
has 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.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:
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!