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] Allow to configure the transport#26941
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
293eb67 to0c3d0a3Compare| $loader->load('messenger.xml'); | ||
| if ($config['transport']['serializer']['enabled']) { |
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.
Detail, but could useExtension::isConfigEnabled
| $container->removeAlias('messenger.transport.default_decoder'); | ||
| if (!$container->has('serializer') &&$container->hasAlias('messenger.transport.encoder')) { | ||
| if ('messenger.transport.symfony_serializer_encoder' === (string)$container->getAlias('messenger.transport.encoder')) { | ||
| thrownewRuntimeException('Using the default encoder/decoder, Symfony Messenger requires the Serializer. Try running "composer require symfony/serializer-pack".'); |
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.
\LogicException
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.
That could also happen when the serializer component is simply disabled (but sure, using flex the component is enabled by default when installed).
However, what about throwing directly from theFrameworkExtension? When$this->isConfigEnabled($container, $config['transport']['serializer']) istrue and the$this->isConfigEnabled($container, $config['serializer']) isfalse, we should throw an exception about enabling the component. If trying to enabling it without the component installed, the Fwb ext will already hint for installing the component.
The'messenger.transport.symfony_serializer_encoder' === (string) $container->getAlias('messenger.transport.encoder') check doesn't look really relevant to me. If themessenger.transport.serializer config is enabled without the Serializer component installed/enabled, it should throw anyway.
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.
However, what about throwing directly from the FrameworkExtension? When $this->isConfigEnabled($container, $config['transport']['serializer']) is true and the $this->isConfigEnabled($container, $config['serializer']) is false, we should throw an exception about enabling the component. If trying to enabling it without the component installed, the Fwb ext will already hint for installing the component.
Very good idea!
| <serviceid="messenger.transport.default_encoder"alias="messenger.transport.serialize_message_with_type_in_headers"public="true" /> | ||
| <serviceid="messenger.transport.default_decoder"alias="messenger.transport.serialize_message_with_type_in_headers"public="true" /> | ||
| <serviceid="messenger.transport.symfony_serializer_encoder"alias="messenger.transport.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.
Are those aliases really needed? I thinkmessenger.transport.symfony_serializer is fine as defaultencoder/decoder config value.
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.
Yeah, definitely now that we have the aliases generated by FWB.
f8145c4 to2fd43abComparesroze commentedApr 16, 2018
@ogizanagi changed the PR based on your feedbacks. Looks better 👌 |
6de67e1 to97ffc7bCompare| $loader->load('messenger.xml'); | ||
| if ($this->isConfigEnabled($container,$config['transport']['serializer'])) { | ||
| if (count($config['adapters']) >0 && !$this->isConfigEnabled($container,$serializerConfig)) { |
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.
Is thecount($config['adapters']) > 0 required?
To me, as soon as the$config['transport']['serializer'] is enabled without serializer support enabled we should throw anyway. We don't really care if it's used by an adapter or not at this point:
in any case,messenger.transport.serializer will never be automatically enabled, unless using flex and both Serializer and Messenger component are installed (so everything is fine).
If enabled without without the Serializer installed/enabled, it means the developer explicitly enabledmessenger.transport.serializer or disabledserializer. 😃
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 was actually changing this (see the last commit). Now this makes sense (by have the Symfony Serialized based Transport enabled no matter what). It's important to have thecount($config['adapters']) so that it doesn't throw anything if you don't use any adapter (i.e. don't require an encoder/decoder).
3c41fb9 to45cdf4eCompare| ->end() | ||
| ->end() | ||
| ->end() | ||
| ->scalarNode('encoder')->defaultValue('messenger.transport.symfony_serializer')->end() |
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.
what about removing thesymdony_ here (and in the other places in this PR also)?
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 don't see either a benefit nor a downside 🙃
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.
Except that it's about Symfony's internal here so it might make sense to removesymfony_
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.
Removed 👍
| </service> | ||
| <serviceid="messenger.transport.default_encoder"alias="messenger.transport.serialize_message_with_type_in_headers"public="true" /> | ||
| <serviceid="messenger.transport.default_decoder"alias="messenger.transport.serialize_message_with_type_in_headers"public="true" /> |
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.
Is there a reason to move to PHP? If not, better keep here, isn't it?
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.
There is a reason: it's now configurable via theframework.messenger.transport configuration.
sroze commentedApr 17, 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.
Though, if we rename "adapters" to "transports" this would clash 🤔. WDYT about removing this |
nicolas-grekas commentedApr 17, 2018
Yes for less nesting levels |
b8e7347 to1a3f0bbCompareThis PR was squashed before being merged into the 4.1-dev branch (closes#26941).Discussion----------[Messenger] Allow to configure the transport| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | ish| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#26900,#26908,#26935| License | MIT| Doc PR | øWe allow users to configure the encoder/decoder used by the built-in adapter(s). This also adds the support of configuring the default's encoder/decoder format and context.Commits-------1a3f0bb [Messenger] Allow to configure the transport
r3m4k3 commentedOct 7, 2018
Why framework.messenger.serializer is expecting an array instead of a string? |
sroze commentedOct 7, 2018
@r3m4k3 it doesnow supports a string directly on |
r3m4k3 commentedOct 7, 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 Thank you for the answer. But hmm, I got this error: |
sroze commentedOct 7, 2018
"now" is in 4.2, the development branch. So make sure you use it :) |
r3m4k3 commentedOct 7, 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 I have managed to do this, now I use dev branch. But is there any example of this serializer implementing Encoder and DecoderInterface, which is handling circular references properly? I'm asking because it's working like it still loads default symfony serializer instead of my custom, setting |
We allow users to configure the encoder/decoder used by the built-in adapter(s). This also adds the support of configuring the default's encoder/decoder format and context.