[Messenger] Lazy dependency to the Serializer #26908
Conversation
|
AppVeyor's failure is unrelated. |
| { | ||
| if (null === $serializer) { | ||
| throw new \InvalidArgumentException('A Serializer is required to use this Messenger transport. Try running "composer req serializer".'); |
There was a problem hiding this comment.
req -> require (consistency with other spots)
serializer -> symfony/serializer (I think we don't use the aliases in core... just in case there is no Flex)
There was a problem hiding this comment.
Good point. Changed the wording 👍
| { | ||
| if (null === $serializer) { | ||
| throw new \InvalidArgumentException('A Serializer is required to use this Messenger transport. Try running "composer require symfony/serializer".'); |
There was a problem hiding this comment.
IMO it's not the responsibility of this class to do that. It should be part of the DI pass, where it was handled before.
There was a problem hiding this comment.
I agree. The benefit is that it'd remain a compile-time check. Here it's only at runtime. Also, it's not necessarily the component that is not installed. It may just not be enabled.
|
Any idea on how we can achieve such DX in that case? 🤔
…On Thu, 12 Apr 2018 at 22:13, Maxime Steinhausser ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/Symfony/Component/Messenger/Transport/Serialization/Serializer.php
<#26908 (comment)>:
> {
+ if (null === $serializer) {
+ throw new \InvalidArgumentException('A Serializer is required to use this Messenger transport. Try running "composer require symfony/serializer".');
I agree. The benefit is that it'd remains a compile-time check. Here it's
only at runtime. Also, it's not necessarily the component that is not
installed. It may just not be enabled.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#26908 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAxHEZ4x1tIs2oSQFujSeYa3RjdBt8Ajks5tn8N8gaJpZM4TSD7S>
.
|
|
Can't it be done from the FrameworkExtension by throwing when trying to use the amqp adapter without the Serializer component installed/enabled? |
|
The thing is that you could use the AMQP adapter without Symfony's Serializer component if you use another encoder/decoder. Maybe we can know it when you wire the default encoder/decoder 🤔 |
|
@sroze : 🤔AFAIK, there is no configuration for changing the default encoder/decoder. So when using the Or perhaps you had in mind making the |
|
I think the approach in #26941 is much better. Closing this one. |
This 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
So far, when the serializer was not installed we had the following error message:
It will now be: