[Messenger] availability to add serializer config at transport level #44951

nikophil · 2022-01-08T13:44:58Z

Q	A
Branch?	6.1
Bug fix?	no
New feature?	yes
Deprecations?	no
License	MIT
Doc PR	will be added if feature gets accepted

Hello,

this PR adds the possibility to override default format and context for messenger's serializer:

framework:
    messenger:
        transports:
            foo:
                serializer: messenger.transport.symfony_serializer
                dsn: 'null://'
                symfony_serializer:
                    format: 'xml'
                    context:
                        some_context: true

I hope this does not introduce BC breaks - tried to avoid it

nikophil · 2022-01-08T13:46:19Z

...eworkBundle/Tests/DependencyInjection/Fixtures/xml/messenger_transport_serializer_config.xml

+            <framework:transport name="foo" dsn="null://">
+                <framework:symfony_serializer format="xml">
+                    <framework:context>
+                        <framework:some_other_context>true</framework:some_other_context>


tests are failing here

1) Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\XmlFrameworkExtensionTest::testMessengerWithTransportSerializerConfig Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: Unable to parse file "/home/nikophil/works/symfony/src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/messenger_transport_serializer_config.xml": [ERROR 1871] Element '{http://symfony.com/schema/dic/symfony}symfony_serializer': This element is not expected. Expected is one of ( {http://symfony.com/schema/dic/symfony}options, {http://symfony.com/schema/dic/symfony}retry-strategy ). (in /home/nikophil/works/symfony/ - line 18, column 0)

don't know how to make them pass

The schema needs an update

symfony/src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

Line 499 in 6b9fafb

<xsd:element name="symfony-serializer" type="messenger_symfony_serializer" minOccurs="0" />

ok thanks, I was thinking it was something automatically generated

nikophil · 2022-01-08T13:47:06Z

...ony/Component/Messenger/Transport/Serialization/FormatAndContextAwareSerializerInterface.php

+/**
+ * @author Nicolas PHILIPPE <nikophil@gmail.com>
+ */
+interface FormatAndContextAwareSerializerInterface extends SerializerInterface


I've introduced this new interface to not break BC

nikophil · 2022-01-08T13:48:28Z

src/Symfony/Component/Messenger/Transport/Serialization/Serializer.php

    public function __construct(SymfonySerializerInterface $serializer = null, string $format = 'json', array $context = [])
    {
        $this->serializer = $serializer ?? self::create()->serializer;
-        $this->format = $format;
-        $this->context = $context + [self::MESSENGER_SERIALIZATION_CONTEXT => true];
+        $this->setFormat($format);
+        $this->setContext($context);
    }


I think removing $format and $context from constructor would have been a BC break, that's why I let them here. Maybe we should introduce some deprecation?

nikophil · 2022-01-08T13:50:58Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+                $container->setDefinition($serializerWithCustomConfigurationId = "messenger.transport.{$name}.serializer", new ChildDefinition($serializerId))
+                    ->addMethodCall('setFormat', [$transport['symfony_serializer']['format'] ?? $config['serializer']['symfony_serializer']['format']])
+                    ->addMethodCall('setContext', [($transport['symfony_serializer']['context'] ?? []) + $config['serializer']['symfony_serializer']['context']])
+                ;


new definition(s) needed here: if we want different config with the same service (for example default symfony's serializer) it has to be duplicated.

nikophil · 2022-01-08T13:52:23Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+            if (isset($transport['symfony_serializer'])) {
+                $serializerDefinition = $container->findDefinition($serializerId);
+
+                if (!isset(class_implements($serializerDefinition->getClass())[FormatAndContextAwareSerializerInterface::class])) {


IMO it's safer to rely on an interface to inject these parameters instead of constructor's parameters order: the service could be defined in user land

I don't think this interface makes sense in the Messenger package. It's way too much tied to the Symfony Serializer implementation, and it's not even used at runtime, only here for DI.

the service could be defined in user land

If the user defines the service in userland, then they can inject the format and context themselves and won't need this configuration entry, right?

I'd remove the interface, trait and method call to inject format and context using constructor's parameters, as for the messenger.symfony_serializer section.

If the user defines the service in userland, then they can inject the format and context themselves and won't need this configuration entry, right?

it depends: presently there is no way to define a custom serializer in framework.messenger.serializer.default_serializer which could serialize in both json and xml (except by adding some plumbing of course)

I agree with @ogizanagi here

carsonbot · 2022-01-09T09:48:55Z

Hey!

I think @monteiro has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

carsonbot · 2022-01-09T09:58:59Z

Hey!

I think @alirezamirsepassi has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

ro0NL · 2022-01-09T11:52:44Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -1386,6 +1386,18 @@ function ($a) {
                                ->children()
                                    ->scalarNode('dsn')->end()
                                    ->scalarNode('serializer')->defaultNull()->info('Service id of a custom serializer to use.')->end()
+                                    ->arrayNode('symfony_serializer')


what about a less specific context node, eg.context.format to be used by SF serializer

you mean we could have this kind of configuration?

messenger: transports: foo: context: format: xml xml_root_node_name: foo

and then $context['format'] would be passsed to $this->serializer->(de)serialize() in Symfony\Component\Messenger\Transport\Serialization\SerializerInterface?

yes, but it was it bit pre-mature of me. I think messenger.transports.N.serializer is what couples transports with a serializer, and this new config complexifies things for little gain.

In that sense, i prefer to add a new prototyped messenger.serializers and deprecate messenger.serializer

ro0NL

why not configure a dedicated serializer instead?

edit; i see, it's not prototyped :( to me that would be more flexible. And then there's no need for a new "format & context aware" abstraction.

then on a per-transport base, maybe configuring default stamps is better (eg. SerializerStamp)

nikophil · 2022-01-09T15:20:12Z

hi @ro0NL
thanks for you review

it's not prototyped :( to me that would be more flexible.

what do you mean by "prototyped"?

ro0NL · 2022-01-09T16:13:33Z

see https://symfony.com/doc/current/components/config/definition.html#array-nodes

instead of a single symfony_serializer under framework.messenger.serializer, we have could many that can be shared across transports. (say 1 for yaml and 1 for xml respectively)

note you can also use any service ID of yours at transports.foo.serializer, which i tend to prefer over more semantic config in this case.

i guess ideally we'd allow to configure many serializer instances directly under framework.serializer 🤔 for you to reference (see eg. #38542 to build upon)

nikophil · 2022-01-10T11:32:58Z

OK thanks for explanation, indeed why not defining several serializers under framework.messenger.serializer, this could be less verbose for transports which shares format/context

note you can also use any service ID of yours at transports.foo.serializer, which i tend to prefer over more semantic config in this case.

yep, but in my experience, all these "custom" transport serializers tend to always be used to vary format and context (and to chose in which POPO message class it should deserialize incoming message if it comes from outside of the current app)

i guess ideally we'd allow to configure many serializer instances directly under framework.serializer

why not, but in our case, it would be only useful for the context, and not to change the format. And by the way it would require some extra changes in messenger's configuration since messenger.transports.foo.serializer and messenger.serializer.default_serializer expect an instance of Symfony\Component\Messenger\Transport\Serialization\SerializerInterface and not Symfony\Component\Serializer\SerializerInterface

ro0NL · 2022-01-10T12:55:05Z

it would be only useful for the context, and not to change the format

👍 let's not couple framework.serializer with framework.messenger.serializer

nikophil · 2022-01-10T13:41:31Z

let's not couple framework.serializer with framework.messenger.serializer

That was not my intention, just saying it was not covering the whole scope of this PR

weaverryan · 2022-01-11T14:15:09Z

I like the idea, in theory, of re-using framework.serializer config to, for example, add the ability to register multiple serializers. And then, re-use those services over in Messenger. But, as @nikophil pointed out:

And by the way it would require some extra changes in messenger's configuration since messenger.transports.foo.serializer and messenger.serializer.default_serializer expect an instance of Symfony\Component\Messenger\Transport\Serialization\SerializerInterface and not Symfony\Component\Serializer\SerializerInterface

In other words, the "messenger serializer" is a different thing that the "Symfony serializer" (though sometimes we use Symfony's serializer inside of a messenger serializer).

So, I like this PR on a high level. The Messenger serializer that leverages the Symfony serializer - https://github.com/symfony/symfony/blob/6.1/src/Symfony/Component/Messenger/Transport/Serialization/Serializer.php - already is able to handle a custom "format" and "context". So this PR just allows you to specify that at a transport-level as well.

What I'm less sure about is exactly how the configuration should look. It does feel a bit weird to have a serializer node under each transport and then also a symfony_serializer node, which would only be used if... (I think?) you set the serializer key to messenger.symfony_serializer. Or, as @ro0NL may have been suggesting here - https://github.com/symfony/symfony/pull/44951/files#r780771460 - do we just add some context node that allows any keys. And then we systematically pass that to the serializer (e.g. maybe we introduce a ContextAwareSerializerInterface with a setContext() method and if the context key is set, we call this method, or throw an exception if the serializer doesn't implement this).

Cheers!

chalasr · 2022-01-11T17:14:02Z

Note that we just deprecated ContextAware*Interface from Serializer in #43982. The purpose of these interfaces was to allow the $context extra parameter for the supports*() methods in (de)normalizers/serializers. Now it is part of the base interfaces.
On another hand, #44125 added a setDefaultContext() method to the DateTimeNormalizer.

carsonbot added Status: Needs Review Feature Messenger labels Jan 8, 2022

carsonbot added this to the 6.1 milestone Jan 8, 2022

nikophil marked this pull request as draft January 8, 2022 13:45

nikophil commented Jan 8, 2022

View reviewed changes

nikophil marked this pull request as ready for review January 8, 2022 13:53

[Messenger] availability to add serializer config at transport level

ebbfac2

ro0NL reviewed Jan 9, 2022

View reviewed changes

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022

nikophil closed this by deleting the head repository Dec 29, 2022

nikophil mentioned this pull request Jan 24, 2023

[Messenger] ease usage when dealing with "external" messages #49094

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] availability to add serializer config at transport level #44951

[Messenger] availability to add serializer config at transport level #44951

nikophil commented Jan 8, 2022 •

edited

Loading

nikophil Jan 8, 2022

chalasr Jan 10, 2022

nikophil Jan 11, 2022 •

edited

Loading

nikophil Jan 8, 2022 •

edited

Loading

nikophil Jan 8, 2022 •

edited

Loading

nikophil Jan 8, 2022 •

edited

Loading

nikophil Jan 8, 2022 •

edited

Loading

ogizanagi Jan 10, 2022 •

edited

Loading

nikophil Jan 10, 2022 •

edited

Loading

chalasr Jan 10, 2022

carsonbot commented Jan 9, 2022

carsonbot commented Jan 9, 2022

ro0NL Jan 9, 2022 •

edited

Loading

nikophil Jan 10, 2022

ro0NL Jan 11, 2022 •

edited

Loading

ro0NL left a comment •

edited

Loading

nikophil commented Jan 9, 2022 •

edited

Loading

ro0NL commented Jan 9, 2022

nikophil commented Jan 10, 2022 •

edited

Loading

ro0NL commented Jan 10, 2022

nikophil commented Jan 10, 2022

weaverryan commented Jan 11, 2022

chalasr commented Jan 11, 2022 •

edited

Loading

[Messenger] availability to add serializer config at transport level #44951

[Messenger] availability to add serializer config at transport level #44951

Conversation

nikophil commented Jan 8, 2022 • edited Loading

nikophil Jan 8, 2022

Choose a reason for hiding this comment

chalasr Jan 10, 2022

Choose a reason for hiding this comment

nikophil Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

nikophil Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

nikophil Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

nikophil Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

nikophil Jan 8, 2022 • edited Loading

Choose a reason for hiding this comment

ogizanagi Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

nikophil Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

chalasr Jan 10, 2022

Choose a reason for hiding this comment

carsonbot commented Jan 9, 2022

carsonbot commented Jan 9, 2022

ro0NL Jan 9, 2022 • edited Loading

Choose a reason for hiding this comment

nikophil Jan 10, 2022

Choose a reason for hiding this comment

ro0NL Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

ro0NL left a comment • edited Loading

Choose a reason for hiding this comment

nikophil commented Jan 9, 2022 • edited Loading

ro0NL commented Jan 9, 2022

nikophil commented Jan 10, 2022 • edited Loading

ro0NL commented Jan 10, 2022

nikophil commented Jan 10, 2022

weaverryan commented Jan 11, 2022

chalasr commented Jan 11, 2022 • edited Loading

nikophil commented Jan 8, 2022 •

edited

Loading

nikophil Jan 11, 2022 •

edited

Loading

nikophil Jan 8, 2022 •

edited

Loading

nikophil Jan 8, 2022 •

edited

Loading

nikophil Jan 8, 2022 •

edited

Loading

nikophil Jan 8, 2022 •

edited

Loading

ogizanagi Jan 10, 2022 •

edited

Loading

nikophil Jan 10, 2022 •

edited

Loading

ro0NL Jan 9, 2022 •

edited

Loading

ro0NL Jan 11, 2022 •

edited

Loading

ro0NL left a comment •

edited

Loading

nikophil commented Jan 9, 2022 •

edited

Loading

nikophil commented Jan 10, 2022 •

edited

Loading

chalasr commented Jan 11, 2022 •

edited

Loading