Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Contributor

@ro0NL ro0NL Jan 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@ro0NL ro0NL Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

->children()
->scalarNode('format')->defaultNull()->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()
->arrayNode('options')
->normalizeKeys(false)
->defaultValue([])
Expand Down
21 changes: 19 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
use Symfony\Component\Messenger\MessageBus;
use Symfony\Component\Messenger\MessageBusInterface;
use Symfony\Component\Messenger\Middleware\RouterContextMiddleware;
use Symfony\Component\Messenger\Transport\Serialization\FormatAndContextAwareSerializerInterface;
use Symfony\Component\Messenger\Transport\Serialization\SerializerInterface;
use Symfony\Component\Messenger\Transport\TransportFactoryInterface;
use Symfony\Component\Messenger\Transport\TransportInterface;
Expand Down Expand Up @@ -1947,8 +1948,8 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
$container->removeAlias(SerializerInterface::class);
} else {
$container->getDefinition('messenger.transport.symfony_serializer')
->replaceArgument(1, $config['serializer']['symfony_serializer']['format'])
->replaceArgument(2, $config['serializer']['symfony_serializer']['context']);
->addMethodCall('setFormat', [$config['serializer']['symfony_serializer']['format']])
->addMethodCall('setContext', [$config['serializer']['symfony_serializer']['context']]);
$container->setAlias('messenger.default_serializer', $config['serializer']['default_serializer']);
}

Expand Down Expand Up @@ -1976,6 +1977,22 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
$transportRetryReferences = [];
foreach ($config['transports'] as $name => $transport) {
$serializerId = $transport['serializer'] ?? 'messenger.default_serializer';

if (isset($transport['symfony_serializer'])) {
$serializerDefinition = $container->findDefinition($serializerId);

if (!isset(class_implements($serializerDefinition->getClass())[FormatAndContextAwareSerializerInterface::class])) {
Copy link
Contributor Author

@nikophil nikophil Jan 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ogizanagi ogizanagi Jan 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@nikophil nikophil Jan 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @ogizanagi here

throw new InvalidArgumentException(sprintf('Serializer for transport "%s" should implement "%s" in order to have custom format or context.', $name, FormatAndContextAwareSerializerInterface::class));
}

$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']])
;
Comment on lines +1988 to +1991
Copy link
Contributor Author

@nikophil nikophil Jan 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.


$serializerId = $serializerWithCustomConfigurationId;
}

$transportDefinition = (new Definition(TransportInterface::class))
->setFactory([new Reference('messenger.transport_factory'), 'createTransport'])
->setArguments([$transport['dsn'], $transport['options'] + ['transport_name' => $name], new Reference($serializerId)])
Expand Down
22 changes: 22 additions & 0 deletions ...rkBundle/Tests/DependencyInjection/Fixtures/php/messenger_transport_serializer_config.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

$container->loadFromExtension('framework', [
'messenger' => [
'serializer' => [
'default_serializer' => 'messenger.transport.symfony_serializer',
'symfony_serializer' => [
'format' => 'json',
'context' => ['some_context' => true]
]
],
'transports' => [
'foo' => [
'dsn' => 'null://',
'symfony_serializer' => [
'format' => 'xml',
'context' => ['some_other_context' => true]
]
],
],
],
]);
14 changes: 14 additions & 0 deletions .../Tests/DependencyInjection/Fixtures/php/messenger_transport_serializer_config_invalid.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php

$container->loadFromExtension('framework', [
'messenger' => [
'transports' => [
'foo' => [
'dsn' => 'null://',
'symfony_serializer' => [
'format' => 'xml',
]
],
],
],
]);
26 changes: 26 additions & 0 deletions ...rkBundle/Tests/DependencyInjection/Fixtures/xml/messenger_transport_serializer_config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?xml version="1.0" encoding="utf-8" ?>
<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns:framework="http://symfony.com/schema/dic/symfony"
xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd
http://symfony.com/schema/dic/symfony https://symfony.com/schema/dic/symfony/symfony-1.0.xsd">

<framework:config>
<framework:messenger>
<framework:serializer default-serializer="messenger.transport.symfony_serializer">
<framework:symfony-serializer format="xml">
<framework:context>
<framework:some_context>true</framework:some_context>
</framework:context>
</framework:symfony-serializer>
</framework:serializer>
<framework:transport name="foo" dsn="null://">
<framework:symfony_serializer format="xml">
<framework:context>
<framework:some_other_context>true</framework:some_other_context>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@nikophil nikophil Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

ok thanks, I was thinking it was something automatically generated

</framework:context>
</framework:symfony_serializer>
</framework:transport>
</framework:messenger>
</framework:config>
</container>
15 changes: 15 additions & 0 deletions .../Tests/DependencyInjection/Fixtures/xml/messenger_transport_serializer_config_invalid.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?xml version="1.0" encoding="utf-8" ?>
<container xmlns="http://symfony.com/schema/dic/services"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns:framework="http://symfony.com/schema/dic/symfony"
xsi:schemaLocation="http://symfony.com/schema/dic/services https://symfony.com/schema/dic/services/services-1.0.xsd
http://symfony.com/schema/dic/symfony https://symfony.com/schema/dic/symfony/symfony-1.0.xsd">

<framework:config>
<framework:messenger>
<framework:transport name="foo" dsn="null://">
<framework:symfony_serializer format="xml"/>
</framework:transport>
</framework:messenger>
</framework:config>
</container>
15 changes: 15 additions & 0 deletions ...rkBundle/Tests/DependencyInjection/Fixtures/yml/messenger_transport_serializer_config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
framework:
messenger:
serializer:
default_serializer: messenger.transport.symfony_serializer
symfony_serializer:
format: 'json'
context:
some_context: true
transports:
foo:
dsn: 'null://'
symfony_serializer:
format: 'xml'
context:
some_other_context: true
7 changes: 7 additions & 0 deletions .../Tests/DependencyInjection/Fixtures/yml/messenger_transport_serializer_config_invalid.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
framework:
messenger:
transports:
foo:
dsn: 'null://'
symfony_serializer:
format: 'xml'
33 changes: 31 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Exception\LogicException;
use Symfony\Component\DependencyInjection\Loader\ClosureLoader;
use Symfony\Component\DependencyInjection\ParameterBag\EnvPlaceholderParameterBag;
Expand Down Expand Up @@ -901,8 +902,13 @@ public function testMessengerTransportConfiguration()
$this->assertSame('messenger.transport.symfony_serializer', (string) $container->getAlias('messenger.default_serializer'));

$serializerTransportDefinition = $container->getDefinition('messenger.transport.symfony_serializer');
$this->assertSame('csv', $serializerTransportDefinition->getArgument(1));
$this->assertSame(['enable_max_depth' => true], $serializerTransportDefinition->getArgument(2));
self::assertSame(
[
['setFormat', ['csv']],
['setContext', [['enable_max_depth' => true]]],
],
$serializerTransportDefinition->getMethodCalls()
);
}

public function testMessengerWithMultipleBuses()
Expand Down Expand Up @@ -955,6 +961,29 @@ public function testMessengerInvalidTransportRouting()
$this->createContainerFromFile('messenger_routing_invalid_transport');
}

public function testMessengerWithTransportSerializerConfig()
{
$container = $this->createContainerFromFile('messenger_transport_serializer_config');
self::assertTrue($container->has('messenger.transport.foo.serializer'));

$serializerDefinition = $container->findDefinition('messenger.transport.foo.serializer');
self::assertSame(
[
['setFormat', ['xml']],
['setContext', [['some_other_context' => true, 'some_context' => true]]],
],
$serializerDefinition->getMethodCalls()
);
}

public function testMessengerWithTransportSerializerConfigThrowsIfWrongSerializerClass()
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('Serializer for transport "foo" should implement "Symfony\Component\Messenger\Transport\Serialization\FormatAndContextAwareSerializerInterface" in order to have custom format or context.');

$this->createContainerFromFile('messenger_transport_serializer_config_invalid');
}

public function testTranslator()
{
$container = $this->createContainerFromFile('full');
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/Messenger/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ CHANGELOG
---

* Add `SerializedMessageStamp` to avoid serializing a message when a retry occurs.
* Add serializer configuration at transport level.

6.0
---
Expand Down
22 changes: 22 additions & 0 deletions .../Component/Messenger/Transport/Serialization/FormatAndContextAwareSerializerInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Messenger\Transport\Serialization;

/**
* @author Nicolas PHILIPPE <nikophil@gmail.com>
*/
interface FormatAndContextAwareSerializerInterface extends SerializerInterface
Copy link
Contributor Author

@nikophil nikophil Jan 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I've introduced this new interface to not break BC

{
public function setFormat(string $format): void;

public function setContext(array $context): void;
}
31 changes: 31 additions & 0 deletions ...fony/Component/Messenger/Transport/Serialization/FormatAndContextAwareSerializerTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Messenger\Transport\Serialization;

/**
* @author Nicolas PHILIPPE <nikophil@gmail.com>
*/
trait FormatAndContextAwareSerializerTrait
{
private string $format;
private array $context;

public function setFormat(string $format): void
{
$this->format = $format;
}

public function setContext(array $context): void
{
$this->context = $context + [Serializer::MESSENGER_SERIALIZATION_CONTEXT => true];
}
}
10 changes: 5 additions & 5 deletions src/Symfony/Component/Messenger/Transport/Serialization/Serializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,20 @@
/**
* @author Samuel Roze <samuel.roze@gmail.com>
*/
class Serializer implements SerializerInterface
class Serializer implements FormatAndContextAwareSerializerInterface
{
use FormatAndContextAwareSerializerTrait;

public const MESSENGER_SERIALIZATION_CONTEXT = 'messenger_serialization';
private const STAMP_HEADER_PREFIX = 'X-Message-Stamp-';

private SymfonySerializerInterface $serializer;
private string $format;
private array $context;

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);
}
Comment on lines 41 to 46
Copy link
Contributor Author

@nikophil nikophil Jan 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?


public static function create(): self
Expand Down