Skip to content

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

Merged
merged 4 commits into from
Jan 25, 2019

Conversation

weaverryan
Copy link
Member

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 messenger-php-serialization branch from 18a4de7 to b4788e4 Compare January 22, 2019 20:44
@@ -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" />
Copy link
Contributor

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.

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

Copy link
Member

@nicolas-grekas nicolas-grekas left a 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.

@nicolas-grekas
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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo here 😄

@javiereguiluz
Copy link
Member

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
Copy link
Contributor

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
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
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
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
Copy link
Contributor

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

@dunglas
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
Copy link
Member

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
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
Copy link
Contributor

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
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
Copy link
Contributor

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
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
Copy link
Contributor

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

Copy link
Member

@nicolas-grekas nicolas-grekas left a 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?

@weaverryan
Copy link
Member Author

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 messenger-php-serialization branch from 9127dc7 to 97e2e32 Compare January 25, 2019 15:10
@nicolas-grekas
Copy link
Member

Thank you @weaverryan.

@nicolas-grekas nicolas-grekas merged commit 97e2e32 into symfony:master Jan 25, 2019
nicolas-grekas added a commit that referenced this pull request Jan 25, 2019
…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 messenger-php-serialization branch January 25, 2019 15:50
/**
* @author Ryan Weaver<ryan@symfonycasts.com>
*
* @experimental in 4.2
Copy link
Contributor

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?

Copy link
Member Author

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.

@akorz
Copy link

akorz commented Jun 3, 2019

Hello,

I have framework.yaml

    messenger:
        serializer:
            id: 'native_php_serializer'

but get error

In ArrayNode.php line 331:
                                                                                                                                      
  Unrecognized option "id" under "framework.messenger.serializer". Available options are "default_serializer", "symfony_serializer".  

What should I fix in my config? Thanks

@xabbuh
Copy link
Member

xabbuh commented Jun 3, 2019

This should work (assuming that native_php_serializer is the id of a service that you defined):

framework:
    messenger:
        serializer:
            default_serializer: 'native_php_serializer'

@akorz
Copy link

akorz commented Jun 4, 2019

@xabbuh I was thinking it should be defined somewhere in component?

Now I have something else

Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The service "enqueue.messenger_transport.factory" has a dependency on a non-existent service "messenger.transport.serializer".

I could not find any docs about it :(

@xabbuh
Copy link
Member

xabbuh commented Jun 4, 2019

I guess you can best use one of the support channels to get help setting things up properly.

@mtomala
Copy link

mtomala commented Jun 21, 2019

@weaverryan
There is a bug (probably) in blog post about this feature:
https://symfony.com/blog/new-in-symfony-4-3-native-php-serialization-for-messenger

It should be default_serializer: instead of id:

@javiereguiluz
Copy link
Member

@mtomala when you and @weaverryan decide if this is a bug or not ... please ping me so I can update the blog post. Thanks.

@kunicmarko20
Copy link

At the time when this feature was added #29958 it was still id now it is default_serializer

@ogizanagi
Copy link
Contributor

ogizanagi commented Jun 21, 2019

@javiereguiluz: Since #30628, the serializer to use is actually configurable per transport.

The messenger.serializer section remains to configure both native (default) and symfony serializers:

->arrayNode('serializer')
->addDefaultsIfNotSet()
->children()
->scalarNode('default_serializer')
->defaultValue('messenger.transport.native_php_serializer')
->info('Service id to use as the default serializer for the transports.')
->end()
->arrayNode('symfony_serializer')
->addDefaultsIfNotSet()
->children()
->scalarNode('format')->defaultValue('json')->info('Serialization format for the messenger.transport.symfony_serializer service (which is not the serializer used by default).')->end()
->arrayNode('context')
->normalizeKeys(false)
->useAttributeAsKey('name')
->defaultValue([])
->info('Context array for the messenger.transport.symfony_serializer service (which is not the serializer used by default).')
->prototype('variable')->end()
->end()
->end()
->end()
->end()
->end()

but each transport can specify the messenger.transport.[name].serializer option to specify the service id to use.

->scalarNode('serializer')->defaultNull()->info('Service id of a custom serializer to use.')->end()

(default is used otherwise)

Note doc is up-to-date: https://symfony.com/doc/current/messenger.html#serializing-messages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.