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] Lazy dependency to the Serializer#26908
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
sroze commentedApr 12, 2018
AppVeyor's failure is unrelated. |
| publicfunction__construct(SerializerInterface$serializer =null,string$format ='json') | ||
| { | ||
| if (null ===$serializer) { | ||
| thrownew \InvalidArgumentException('A Serializer is required to use this Messenger transport. Try running "composer req 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point. Changed the wording 👍
| publicfunction__construct(SerializerInterface$serializer =null,string$format ='json') | ||
| { | ||
| if (null ===$serializer) { | ||
| thrownew \InvalidArgumentException('A Serializer is required to use this Messenger transport. Try 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.
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.
ogizanagiApr 12, 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.
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 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.
sroze commentedApr 12, 2018 via email
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> . |
ogizanagi commentedApr 12, 2018
Can't it be done from the FrameworkExtension by throwing when trying to use the amqp adapter without the Serializer component installed/enabled? |
sroze commentedApr 13, 2018
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 🤔 |
ogizanagi commentedApr 13, 2018
@sroze : 🤔AFAIK, there is no configuration for changing the default encoder/decoder. So when using the Or perhaps you had in mind making the |
sroze commentedApr 16, 2018
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: