-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
introducing native php serialize() support for Messenger transport #29958
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
introducing native php serialize() support for Messenger transport #29958
Conversation
44d2ce4
to
18a4de7
Compare
18a4de7
to
b4788e4
Compare
@@ -24,6 +24,8 @@ | |||
</service> | |||
<service id="Symfony\Component\Messenger\Transport\Serialization\SerializerInterface" alias="messenger.transport.serializer" /> | |||
|
|||
<service id="messenger.transport.native_php_serializer" class="Symfony\Component\Messenger\Transport\Serialization\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.
Extra space before the "class" + wrong class.
Given the issues we currently face in the Security component related to PHP's |
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.
Definitely 👍, this issue is really killing productivity right now for no good reason. Security is not an issue here: messages are not user input by themselves.
#29951 is about the Serializable interface and parent::serialize(). We should activity discourage using it (the method call but also the interface) anywhere BTW. |
use Symfony\Component\Messenger\Exception\InvalidArgumentException; | ||
|
||
/** | ||
* @author Ruyan Weaver<ryan@symfonycasts.com> |
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.
Typo here 😄
I haven't used this component, but as an outsider, compatibility with third-party systems look like a huge and important feature to me. Are you sure it's OK to make us incompatible with anything not related to PHP? Somewhat related: for the future, we could consider using Google's Protobuf, which is what most cool and modern apps use nowadays (https://developers.google.com/protocol-buffers/docs/reference/php-generated). |
Both your points are about the serializer doing a bad job with DTOs. Why not improve the Serializer to support DTOs better instead, it fixes this problem by proxy, but also improves the other component. |
In my opinion we should support both out of the box, and indeed default to PHP's Protobuf is also very interesting for this kind of use cases (but is not as popular than JSON). We could support both. |
@dkarlovi we need to improve DTO support indeed (even if it has already been dramatically improved in 4.1/4.2), but it's not always the right tool anyway for messenger: |
I agree that using the Serializer Component makes has some drawbacks compared to using I also agree with @dunglas that the only useful way to fix this is to make it configurable: Symfony has always encouraged a pluggable system where you could easily modify the system to your specific needs while providing a sensible default setup. I believe that's what should be done here: allow using any serialiser but default to one using PHPs built-in serialisation functionality. |
@dunglas don't really understand: the message in Messenger context is in its public context. |
@dkarlovi not really:
It hurts DX (especially for those used to how Laravel works). Messaging could be considered internal (as done when using a sync handler) from an app level. The serialization is just an implementation detail, and so preserving states would improve both DX and performance (it just works). I would say that it's different if the message is intended to the current app (like when using a messenger handler) => private, or for another app (like one written in another language) => public. |
no need to write this. I've no idea what Laravel is, yet I spent an afternoon debugging that transport thing until I created #29163. We have a serious issue here. The serialization mechanism is already "pluggable", here we're talking about changing the default. |
@nicolas-grekas I mean when you use Enqueue (it's the use case described by @weaverryan in the PR description), messaging is considered something purely internal and transparent. You don't even have to know that the message is serialized. It's "magic", but it works. When switching to Messenger, you have to think about using public properties, because JSON.
It's easy to do (as showcased by the code you linked).
But why? native serialization is faster, and easy to use. |
It depends how you define "private", I guess. From Messenger's POV, the message passed is definitely in its public representation since it's going out of the local app's context onto a transport, it's irrelevant if the consumer is the same app again, that will be determined long after you've already serialized it. |
We have such kind of configs in other components at least. Could be something like: messenger:
# ...
serialize: private # default, can also be public (JSON) or a service name |
I agree with this point and instead of private/public I'd suggest |
the answer is yes, I just needed to look at the first file in the PR :) |
This will probably continue to be debated, we might need "stats" about what proportion of apps use or don't use 3rd-party apps 😕 |
Let's agree the DX is seriously broken, up to the point where advocating the component to the masses is questionable currently. This PR is a serious solution to the problem. |
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.
Oh, one last thing:
Symfony\Component\Messenger\Transport\Serialization\Serializer::create()
should return that new serializer, isn't it? Maybe not actually, can anyone confirm?
I think no, because that's a factory to literally create an instance of THAT class. So, it should continue to create an instance of itself. However, there were 4 places in the code that used this factory function to create a default serializer if none was passed. I change those to create the new |
9127dc7
to
97e2e32
Compare
Thank you @weaverryan. |
…er transport (weaverryan, xabbuh) This PR was merged into the 4.3-dev branch. Discussion ---------- introducing native php serialize() support for Messenger transport | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes and no | New feature? | yes and no | BC breaks? | maybe (yes if we change the default) | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29163 | License | MIT | Doc PR | TODO! Messenger currently uses the Serialize to serialize to JSON and then unserialize. This creates a lot of issues: 1) The default serializer requires you to have getter & setter method (or public properties) for them to be serialized. This makes it easy for data to disappear. I've seen several really smart people have this problem. 2) Related to the above, the forced getters/setters (and no required constructor args) force you to design your message classes around this. It's not that the serializer is doing a bad job - it's just not the right use-case for it. This PR proposes simply using `serialize()` and `unserialize()`. This is the behavior we want: we want to put objects to sleep and wake them back up. I believe the original reason we did not do this was so that we could export "generic JSON", in case we wanted other workers (not our Symfony app) to consume the messages. But, that's an edge case, and could still be accomplished by creating your own serializer. Btw, Laravel uses `serialize()` as does Enqueue for (un)serializing Event objects. We're making our life more difficult for no benefit. Cheers! Commits ------- 97e2e32 Changing default serializer in Messenger component to PhpSerializer 3111cef Update src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.xml 4132bfe updating CHANGELOGs and fixing tests b4788e4 introducing native php serialize() support for Messenger transport
/** | ||
* @author Ryan Weaver<ryan@symfonycasts.com> | ||
* | ||
* @experimental in 4.2 |
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.
Shouldn't this be 4.3
?
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.
Hmm, actually - not sure. The component is experimental in 4.2 - it won’t be (as of now in 4.3). I added this for consistency with the rest of the component. Either all will be updates at the same time to 4.3 or (hopefully) all will be removed.
Hello, I have framework.yaml
but get error
What should I fix in my config? Thanks |
This should work (assuming that framework:
messenger:
serializer:
default_serializer: 'native_php_serializer' |
@xabbuh I was thinking it should be defined somewhere in component? Now I have something else
I could not find any docs about it :( |
I guess you can best use one of the support channels to get help setting things up properly. |
@weaverryan It should be |
@mtomala when you and @weaverryan decide if this is a bug or not ... please ping me so I can update the blog post. Thanks. |
At the time when this feature was added #29958 it was still |
@javiereguiluz: Since #30628, the serializer to use is actually configurable per transport. The symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Lines 1155 to 1176 in dca9325
but each transport can specify the symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Line 1189 in dca9325
(default is used otherwise) Note doc is up-to-date: https://symfony.com/doc/current/messenger.html#serializing-messages |
Messenger currently uses the Serialize to serialize to JSON and then unserialize. This creates a lot of issues:
The default serializer requires you to have getter & setter method (or public properties) for them to be serialized. This makes it easy for data to disappear. I've seen several really smart people have this problem.
Related to the above, the forced getters/setters (and no required constructor args) force you to design your message classes around this.
It's not that the serializer is doing a bad job - it's just not the right use-case for it.
This PR proposes simply using
serialize()
andunserialize()
. This is the behavior we want: we want to put objects to sleep and wake them back up.I believe the original reason we did not do this was so that we could export "generic JSON", in case we wanted other workers (not our Symfony app) to consume the messages. But, that's an edge case, and could still be accomplished by creating your own serializer.
Btw, Laravel uses
serialize()
as does Enqueue for (un)serializing Event objects. We're making our life more difficult for no benefit.Cheers!