Skip to content
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

Merged
merged 4 commits into from Jan 25, 2019

Conversation

@weaverryan
Copy link
Member

weaverryan commented Jan 22, 2019

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!

@weaverryan weaverryan force-pushed the weaverryan:messenger-php-serialization branch from 44d2ce4 to 18a4de7 Jan 22, 2019

@weaverryan weaverryan force-pushed the weaverryan:messenger-php-serialization branch from 18a4de7 to b4788e4 Jan 22, 2019

@@ -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" />

This comment has been minimized.

Copy link
@fancyweb

fancyweb Jan 22, 2019

Contributor

Extra space before the "class" + wrong class.

@xabbuh

This comment has been minimized.

Copy link
Member

xabbuh commented Jan 22, 2019

Given the issues we currently face in the Security component related to PHP's serialize() implementation (see for example #29951) I am not sure if we should actively support this.

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

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.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 22, 2019

#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>

This comment has been minimized.

Copy link
@stloyd

stloyd Jan 23, 2019

Contributor

Typo here 😄

@javiereguiluz

This comment has been minimized.

Copy link
Member

javiereguiluz commented Jan 23, 2019

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).

@dkarlovi

This comment has been minimized.

Copy link
Contributor

dkarlovi commented Jan 23, 2019

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.

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Jan 23, 2019

In my opinion we should support both out of the box, and indeed default to PHP's serialize(). @weaverryan's arguments make sense, but only when the consumer is written in PHP. It often makes sense to use other languages for async consumers (Go or C for computations, Python for Machine Learning...), in such cases consuming JSON is easier than parsing the PHP internal format.

Protobuf is also very interesting for this kind of use cases (but is not as popular than JSON). We could support both.

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Jan 23, 2019

@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: serialize preserves private and protected states. It makes no senses to add this kind of features to the Symfony serializer (its goal is to transform the object in a public representation).

@alcaeus

This comment has been minimized.

Copy link
Contributor

alcaeus commented Jan 23, 2019

I agree that using the Serializer Component makes has some drawbacks compared to using serialize. On the other hand, it ensures that developers can choose a different serialisation method depending on their environment.

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.

@dkarlovi

This comment has been minimized.

Copy link
Contributor

dkarlovi commented Jan 23, 2019

@dunglas don't really understand: the message in Messenger context is in its public context.

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Jan 23, 2019

@dkarlovi not really:

  • private states are preserved when using synchronous handlers
  • they aren't when a transport is configured

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.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 23, 2019

especially for those used to how Laravel works

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.
I don't remember if we have a DI config integration to leverage this pluggability?

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Jan 23, 2019

@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.

Seems it's time to revisit this convention of serializer and think about adding possibility to serialize such props as well

It's easy to do (as showcased by the code you linked).

they don't use native serialization

But why? native serialization is faster, and easy to use.

@dkarlovi

This comment has been minimized.

Copy link
Contributor

dkarlovi commented Jan 23, 2019

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.

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.

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Jan 23, 2019

I don't remember if we have a DI config integration to leverage this pluggability?

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

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 23, 2019

@Pierstoval

This comment has been minimized.

Copy link
Contributor

Pierstoval commented Jan 23, 2019

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 native|{service_id}, referring to services sounds like the best IMO

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 23, 2019

I don't remember if we have a DI config integration to leverage this pluggability?

the answer is yes, I just needed to look at the first file in the PR :)
There is nothing else to do here, by default we consider messages "private/internal", which we should.
And ppl who need interop with third-party apps have more work to do.
That's how things should be IMHO.

@Pierstoval

This comment has been minimized.

Copy link
Contributor

Pierstoval commented Jan 23, 2019

And ppl who need interop with third-party apps have more work to do.

This will probably continue to be debated, we might need "stats" about what proportion of apps use or don't use 3rd-party apps 😕

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 23, 2019

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.
The Serializer component is really nice when compat with other techno is needed.
But that comes AFTER.

@weaverryan weaverryan force-pushed the weaverryan:messenger-php-serialization branch from 7d4dbd1 to 6d20bbc Jan 23, 2019

@weaverryan weaverryan force-pushed the weaverryan:messenger-php-serialization branch from 6d20bbc to 4132bfe Jan 23, 2019

Update src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.xml
Co-Authored-By: weaverryan <weaverryan+github@gmail.com>
@xabbuh

xabbuh approved these changes Jan 24, 2019

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

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?

@weaverryan

This comment has been minimized.

Copy link
Member Author

weaverryan commented Jan 25, 2019

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 PhpSerializer. I think we should consistently use the same default serializer everywhere.

@weaverryan weaverryan force-pushed the weaverryan:messenger-php-serialization branch from 9127dc7 to 97e2e32 Jan 25, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 25, 2019

Thank you @weaverryan.

@nicolas-grekas nicolas-grekas merged commit 97e2e32 into symfony:master Jan 25, 2019

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jan 25, 2019

feature #29958 introducing native php serialize() support for Messeng…
…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

@weaverryan weaverryan deleted the weaverryan:messenger-php-serialization branch Jan 25, 2019

/**
* @author Ryan Weaver<ryan@symfonycasts.com>
*
* @experimental in 4.2

This comment has been minimized.

Copy link
@jdreesen

jdreesen Jan 27, 2019

Contributor

Shouldn't this be 4.3?

This comment has been minimized.

Copy link
@weaverryan

weaverryan Jan 27, 2019

Author Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.