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

[Messenger] Allow to configure the transport #26941

Merged
merged 1 commit into from Apr 17, 2018

Conversation

Projects
None yet
5 participants
@sroze
Member

sroze commented Apr 16, 2018

Q A
Branch? master
Bug fix? no
New feature? ish
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26900, #26908, #26935
License MIT
Doc PR ø

We allow users to configure the encoder/decoder used by the built-in adapter(s). This also adds the support of configuring the default's encoder/decoder format and context.

@@ -1446,6 +1446,20 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
$loader->load('messenger.xml');
if ($config['transport']['serializer']['enabled']) {

This comment has been minimized.

@ogizanagi

ogizanagi Apr 16, 2018

Member

Detail, but could use Extension::isConfigEnabled

$container->removeAlias('messenger.transport.default_decoder');
if (!$container->has('serializer') && $container->hasAlias('messenger.transport.encoder')) {
if ('messenger.transport.symfony_serializer_encoder' === (string) $container->getAlias('messenger.transport.encoder')) {
throw new RuntimeException('Using the default encoder/decoder, Symfony Messenger requires the Serializer. Try running "composer require symfony/serializer-pack".');

This comment has been minimized.

@ogizanagi

ogizanagi Apr 16, 2018

Member

\LogicException

This comment has been minimized.

@ogizanagi

ogizanagi Apr 16, 2018

Member

That could also happen when the serializer component is simply disabled (but sure, using flex the component is enabled by default when installed).

However, what about throwing directly from the FrameworkExtension? When $this->isConfigEnabled($container, $config['transport']['serializer']) is true and the $this->isConfigEnabled($container, $config['serializer']) is false, we should throw an exception about enabling the component. If trying to enabling it without the component installed, the Fwb ext will already hint for installing the component.

The 'messenger.transport.symfony_serializer_encoder' === (string) $container->getAlias('messenger.transport.encoder') check doesn't look really relevant to me. If the messenger.transport.serializer config is enabled without the Serializer component installed/enabled, it should throw anyway.

This comment has been minimized.

@sroze

sroze Apr 16, 2018

Member

However, what about throwing directly from the FrameworkExtension? When $this->isConfigEnabled($container, $config['transport']['serializer']) is true and the $this->isConfigEnabled($container, $config['serializer']) is false, we should throw an exception about enabling the component. If trying to enabling it without the component installed, the Fwb ext will already hint for installing the component.

Very good idea!

</service>
<service id="messenger.transport.default_encoder" alias="messenger.transport.serialize_message_with_type_in_headers" public="true" />
<service id="messenger.transport.default_decoder" alias="messenger.transport.serialize_message_with_type_in_headers" public="true" />
<service id="messenger.transport.symfony_serializer_encoder" alias="messenger.transport.symfony_serializer" />

This comment has been minimized.

@ogizanagi

ogizanagi Apr 16, 2018

Member

Are those aliases really needed? I think messenger.transport.symfony_serializer is fine as default encoder/decoder config value.

This comment has been minimized.

@sroze

sroze Apr 16, 2018

Member

Yeah, definitely now that we have the aliases generated by FWB.

@sroze sroze added this to the 4.1 milestone Apr 16, 2018

@sroze

This comment has been minimized.

Member

sroze commented Apr 16, 2018

@ogizanagi changed the PR based on your feedbacks. Looks better 👌

{
if (!interface_exists(MessageBusInterface::class)) {
throw new LogicException('Messenger support cannot be enabled as the Messenger component is not installed.');
}
$loader->load('messenger.xml');
if ($this->isConfigEnabled($container, $config['transport']['serializer'])) {
if (count($config['adapters']) > 0 && !$this->isConfigEnabled($container, $serializerConfig)) {

This comment has been minimized.

@ogizanagi

ogizanagi Apr 16, 2018

Member

Is the count($config['adapters']) > 0 required?
To me, as soon as the $config['transport']['serializer'] is enabled without serializer support enabled we should throw anyway. We don't really care if it's used by an adapter or not at this point:
in any case, messenger.transport.serializer will never be automatically enabled, unless using flex and both Serializer and Messenger component are installed (so everything is fine).
If enabled without without the Serializer installed/enabled, it means the developer explicitly enabled messenger.transport.serializer or disabled serializer. 😃

This comment has been minimized.

@sroze

sroze Apr 16, 2018

Member

I was actually changing this (see the last commit). Now this makes sense (by have the Symfony Serialized based Transport enabled no matter what). It's important to have the count($config['adapters']) so that it doesn't throw anything if you don't use any adapter (i.e. don't require an encoder/decoder).

->end()
->end()
->end()
->scalarNode('encoder')->defaultValue('messenger.transport.symfony_serializer')->end()

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 16, 2018

Member

what about removing the symdony_ here (and in the other places in this PR also)?

This comment has been minimized.

@sroze

sroze Apr 16, 2018

Member

I don't see either a benefit nor a downside 🙃

This comment has been minimized.

@sroze

sroze Apr 16, 2018

Member

Except that it's about Symfony's internal here so it might make sense to remove symfony_

This comment has been minimized.

@sroze

sroze Apr 16, 2018

Member

Removed 👍

</service>
<service id="messenger.transport.default_encoder" alias="messenger.transport.serialize_message_with_type_in_headers" public="true" />
<service id="messenger.transport.default_decoder" alias="messenger.transport.serialize_message_with_type_in_headers" public="true" />

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 16, 2018

Member

Is there a reason to move to PHP? If not, better keep here, isn't it?

This comment has been minimized.

@sroze

sroze Apr 16, 2018

Member

There is a reason: it's now configurable via the framework.messenger.transport configuration.

@sroze

This comment has been minimized.

Member

sroze commented Apr 17, 2018

Though, if we rename "adapters" to "transports" this would clash 🤔. WDYT about removing this .transport configuration layer and just having framework.messenger.encoder, framework.messenger.decoder & framework.messenger.serializer ?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 17, 2018

Yes for less nesting levels

@sroze sroze merged commit 1a3f0bb into symfony:master Apr 17, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

sroze added a commit that referenced this pull request Apr 17, 2018

feature #26941 [Messenger] Allow to configure the transport (sroze)
This PR was squashed before being merged into the 4.1-dev branch (closes #26941).

Discussion
----------

[Messenger] Allow to configure the transport

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | ish
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #26900, #26908, #26935
| License       | MIT
| Doc PR        | ø

We allow users to configure the encoder/decoder used by the built-in adapter(s). This also adds the support of configuring the default's encoder/decoder format and context.

Commits
-------

1a3f0bb [Messenger] Allow to configure the transport

@sroze sroze deleted the sroze:configure-messenger-transport branch Apr 17, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

@r3m4k3

This comment has been minimized.

r3m4k3 commented Oct 7, 2018

Why framework.messenger.serializer is expecting an array instead of a string?

@sroze

This comment has been minimized.

Member

sroze commented Oct 7, 2018

@r3m4k3 it does now supports a string directly on framework.messenger.serializer (it will configure framework.messenger.serializer.id)

@r3m4k3

This comment has been minimized.

r3m4k3 commented Oct 7, 2018

@sroze Thank you for the answer. But hmm, I got this error: Invalid type for path "framework.messenger.serializer". Expected array, but got string when I set serializer: App\Serializer\CustomSerializer under framework.messenger.

@sroze

This comment has been minimized.

Member

sroze commented Oct 7, 2018

"now" is in 4.2, the development branch. So make sure you use it :)

@r3m4k3

This comment has been minimized.

r3m4k3 commented Oct 7, 2018

@sroze I have managed to do this, now I use dev branch.
I have installed symfony/* component with all dependencies on version 4.2@dev.

But is there any example of this serializer implementing Encoder and DecoderInterface, which is handling circular references properly?

I'm asking because it's working like it still loads default symfony serializer instead of my custom, setting setCircularReferenceLimit or setCircularReferenceHandler doesn't change anything indeed.

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