Skip to content

[Messenger] Relax messenger config and fix some bugs #27084

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 1 commit into from
May 7, 2018

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Apr 28, 2018

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

After playing a little with the config of this component I found some bugs around.

Reproducer:

  1. Install a fresh Symfony flex project with ^4.0@dev dependencies
  2. Add the symfony/messenger requirement
  3. Add the following configuration separately:

Note that symfony/serializer has not been installed yet. ATM it's not required.


Configuring my custom transport (not amqp):

framework:
    messenger:
        transports:
            custom: 'my_transport://localhost/msgs'
        routing:
            '*': custom

Before (Current behaviour):
Threw a logic exception, IMO unrelated at this point:

Using the default encoder/decoder, Symfony Messenger requires the Serializer. Enable it or install it by running "composer require symfony/serializer-pack".

After (Proposal):
Pass! The Messenger's serializer config is disabled by definition because the Serializer component is not installed yet, then the related (default) encoder/decoder aliases are invalid, so the amqp transport factory service is removed altogether.


According to the previous exception I configured my custom encoder/decoder services:

framework:
    messenger:
        encoder: App\Serializer\Serializer
        decoder: App\Serializer\Serializer
        transports:
            custom: 'my_transport://localhost/msgs'
        routing:
            '*': custom

Before:
The same exception has been thrown, now a bit vague according to the config:

Using the default encoder/decoder, Symfony Messenger requires the Serializer. Enable it or install it by running "composer require symfony/serializer-pack".

After:
Pass! the serializer is disabled by definition but there is custom encoder/decoder services, so let's keep the amqp transport factory with their custom encoder/decoder deps.


Just enabling the serializer option:

framework:
    messenger:
        serializer: true

Before:
Pass! Unexpected, as there is no transport configuration the exception wasn't thrown and still keeps the amqp transport factory service with invalid encoder/decoder (Serializer) dependencies.

After:
The Serializer config & support is verified if it's enabled regardless of the transport configuration and this exception is thrown for this case:

The default Messenger serializer cannot be enabled as the Serializer support is not enabled.

I've removed the "install" part because at this point only we're checking whether the framework.serializer is enabled or not, so the next step after that should be enable the Serializer support in framework.serializer, which already verify whether the Serializer component is installed or not. IMO "composer require symfony/serializer-pack" should be there and not here. Also because symfony/serializer is not a hard dependency of this component.


By last, I disabled the serializer option manually:

framework:
    messenger:
        serializer: false
        transports:
            custom: 'my_transport://localhost/msgs'
        routing:
            '*': custom

Before:
I received this DI exception:

The service "messenger.transport.amqp.factory" has a dependency on a non-existent service "messenger.transport.serializer".

After:
Pass! (same behaviour that the first example)


As I explained earlier, this PR enables or disables the Messenger's serializer config based on the current Symfony platform and whether the Serializer component is installed or not, like other config with similar behaviour.

Tests included :)

Cheers!

@yceruto yceruto force-pushed the relax_messenger_config branch 3 times, most recently from e7e1214 to 0a57dc6 Compare April 28, 2018 06:46
@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Apr 29, 2018
@yceruto yceruto force-pushed the relax_messenger_config branch from 0a57dc6 to eaf960b Compare April 30, 2018 23:57
@sroze
Copy link
Contributor

sroze commented May 1, 2018

With these changes, if I am a developer that just get started with Messenger and uses the AMQP examples, here is what I'll have, as an exception labelled as coming from ChainAdapterFactory line 51:

No adapter supports the given DSN "amqp://guest:guest@localhost:5672/%2f/messages

My reaction would be "WTF?!". I would definitely favour having a DX that almost forces you to install Symfony's Serializer than having to Google things out 🙃

@yceruto
Copy link
Member Author

yceruto commented May 1, 2018

@sroze we've the same point, that's the goal of this PR: improving DX with or without AMQP requirements as AMQP is optional, right?

No adapter supports the given DSN "amqp://guest:guest@localhost:5672/%2f/messages

It's a good example of what I'm trying to improve here. In that case the Serializer component is not enabled or installed so the AMQP adapter was removed. Are you suggesting check by 0 === strpos($dsn, 'amqp://') somewhere to show a better exception message?

My reaction would be "WTF?!". I would definitely favour having a DX that almost forces you to install Symfony's Serializer than having to Google things out

Probably I can react the same if I'm a developer who just started with Messenger and I don't use the AMQP :P Is there any opinion against what I try to solve? or how has it been solved?

@yceruto
Copy link
Member Author

yceruto commented May 1, 2018

a DX that almost forces you to install Symfony's Serializer...

That was precisely my inspiration to create this PR :)

@sroze
Copy link
Contributor

sroze commented May 1, 2018 via email

@yceruto
Copy link
Member Author

yceruto commented May 2, 2018

How can we make it more explicit than we need the serializer to be installed when using the AMQP adapter?

Well, I'm proposing one option here: disabling the AMQP adapter feature if there is no all dependencies to make it work, then we could add a note about composer require symfony/serializer into Messenger documentation to enable it if you want to use the built-in AMQP adapter.

Actually there is many places and features inside Symfony that are enabled/disabled according to req installation, such as doctrine/annotations for serializer and routing components, symfony/security-csrf, symfony/twig-bridge, symfony/translation, symfony/validator for form component and ocramius/proxy-manager for lazy services in DI component for just to mention some.

@sroze
Copy link
Contributor

sroze commented May 2, 2018

Well, I'm proposing one option here: disabling the AMQP adapter feature if there is no all dependencies to make it work, then we could add a note about composer require symfony/serializer into Messenger documentation to enable it if you want to use the built-in AMQP adapter.

I'm happy with disabling it by default but we need a clearer message than this one:

No adapter supports the given DSN "amqp://guest:guest@localhost:5672/%2f/messages

The trick is to be able to provide one, I'm not sure what's the best approach as I've tried multiple times without great success (see #26908 and #26941)

@yceruto yceruto force-pushed the relax_messenger_config branch from eaf960b to 5c595c0 Compare May 3, 2018 13:10
@@ -1506,6 +1509,10 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
$container->getDefinition('messenger.asynchronous.routing.sender_locator')->replaceArgument(1, $messageToSenderIdsMapping);

foreach ($config['adapters'] as $name => $adapter) {
if (0 === strpos($adapter['dsn'], 'amqp://') && !$container->hasDefinition('messenger.adapter.amqp.factory')) {
throw new LogicException('The default AMQP adapter is not available. Make sure you have installed and enabled the Serializer component.');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a sensible tradeoff. But we need to propose a next step to the user: (install it by running "composer require symfony/serializer".)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@yceruto yceruto force-pushed the relax_messenger_config branch from 5c595c0 to 4edc356 Compare May 3, 2018 13:21
@yceruto
Copy link
Member Author

yceruto commented May 3, 2018

@sroze displayed a better exception message when you're trying to use AMQP and the default AMQP adapter is not available (either Serializer component is not installed or it isn't enabled).

@nicolas-grekas
Copy link
Member

Rebase needed after #27129

@yceruto yceruto force-pushed the relax_messenger_config branch 2 times, most recently from ecad67b to fd8c675 Compare May 4, 2018 15:17
<service id="messenger.transport_factory" class="Symfony\Component\Messenger\Transport\Factory\ChainTransportFactory">
<argument type="tagged" tag="messenger.transport_factory" />
</service>

<service id="messenger.adapter.amqp.factory" class="Symfony\Component\Messenger\Transport\AmqpExt\AmqpTransportFactory">
<service id="messenger.transport.amqp.factory" class="Symfony\Component\Messenger\Transport\AmqpExt\AmqpTransportFactory">
Copy link
Member Author

Choose a reason for hiding this comment

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

Renaming according to #27129

@yceruto yceruto force-pushed the relax_messenger_config branch from fd8c675 to d836787 Compare May 4, 2018 19:41
@yceruto
Copy link
Member Author

yceruto commented May 4, 2018

Rebased again after #27150

@yceruto
Copy link
Member Author

yceruto commented May 5, 2018

Shall we go ahead with this one?

@@ -1506,6 +1509,10 @@ private function registerMessengerConfiguration(array $config, ContainerBuilder
$container->getDefinition('messenger.asynchronous.routing.sender_locator')->replaceArgument(1, $messageToSenderIdsMapping);

foreach ($config['transports'] as $name => $transport) {
if (0 === strpos($transport['dsn'], 'amqp://') && !$container->hasDefinition('messenger.transport.amqp.factory')) {
throw new LogicException('The default AMQP transport is not available. Make sure you have installed and enabled the Serializer component. Try enable it or install it by running "composer require symfony/serializer".');
Copy link
Contributor

Choose a reason for hiding this comment

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

We need the serializer-pack I think, just the Serializer wouldn't be enough (i.e. missing normalisers)

Copy link
Contributor

@ogizanagi ogizanagi May 6, 2018

Choose a reason for hiding this comment

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

The serializer-pack does not install any extra normalizer.
But I guess you meant in order for the ObjectNormalizer denormalization to work efficiently based on typehints/docblocks, for which the PropertyInfo component and phpdocumentor/reflection-docblock are required.
So 👍 for mentioning the pack, but that's also probably a hint we already miss when just using the Serializer component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to -pack.

@yceruto yceruto force-pushed the relax_messenger_config branch from d836787 to a05e2e2 Compare May 6, 2018 14:57
@yceruto
Copy link
Member Author

yceruto commented May 6, 2018

Status: Needs Review

@fabpot
Copy link
Member

fabpot commented May 7, 2018

Thank you @yceruto.

@fabpot fabpot merged commit a05e2e2 into symfony:master May 7, 2018
fabpot added a commit that referenced this pull request May 7, 2018
…uto)

This PR was merged into the 4.1-dev branch.

Discussion
----------

[Messenger] Relax messenger config and fix some bugs

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

After playing a little with the config of this component I found some bugs around.

Reproducer:

 1. Install a fresh Symfony flex project with `^4.0@dev` dependencies
 2. Add the `symfony/messenger` requirement
 3. Add the following configuration separately:

Note that `symfony/serializer` has not been installed yet. ATM it's not required.

----------------------

Configuring my custom transport (not amqp):
```yaml
framework:
    messenger:
        transports:
            custom: 'my_transport://localhost/msgs'
        routing:
            '*': custom
```

**Before**  (Current behaviour):
Threw a logic exception, IMO unrelated at this point:
> Using the default encoder/decoder, Symfony Messenger requires the Serializer. Enable it or install it by running "composer require symfony/serializer-pack".

**After** (Proposal):
Pass! The Messenger's serializer config is disabled by definition because the Serializer component is not installed yet, then the related (default) encoder/decoder aliases are invalid, so the amqp transport factory service is removed altogether.

----------------------

According to the previous exception I configured my custom encoder/decoder services:
```yaml
framework:
    messenger:
        encoder: App\Serializer\Serializer
        decoder: App\Serializer\Serializer
        transports:
            custom: 'my_transport://localhost/msgs'
        routing:
            '*': custom
```

**Before**:
The same exception has been thrown, now a bit vague according to the config:
> Using the default encoder/decoder, Symfony Messenger requires the Serializer. Enable it or install it by running "composer require symfony/serializer-pack".

**After**:
Pass! the serializer is disabled by definition but there is custom encoder/decoder services, so let's keep the amqp transport factory with their custom encoder/decoder deps.

----------------------

Just enabling the serializer option:
```yaml
framework:
    messenger:
        serializer: true
```

**Before**:
Pass! Unexpected, as there is no transport configuration the exception wasn't thrown and still keeps the amqp transport factory service with invalid encoder/decoder (Serializer) dependencies.

**After**:
The Serializer config & support is verified if it's enabled regardless of the transport configuration and this exception is thrown for this case:
> The default Messenger serializer cannot be enabled as the Serializer support is not enabled.

I've removed the "install" part because at this point only we're checking whether the `framework.serializer` is enabled or not, so the next step after that should be enable the Serializer support in `framework.serializer`, which already verify whether the Serializer component is installed or not. IMO "composer require symfony/serializer-pack" should be there and not here. Also because `symfony/serializer` is not a hard dependency of this component.

----------------------

By last, I disabled the serializer option manually:
```yaml
framework:
    messenger:
        serializer: false
        transports:
            custom: 'my_transport://localhost/msgs'
        routing:
            '*': custom
```

**Before**:
I received this DI exception:
> The service "messenger.transport.amqp.factory" has a dependency on a non-existent service "messenger.transport.serializer".

**After**:
Pass! (same behaviour that the first example)

----------------------

As I explained earlier, this PR enables or disables the Messenger's serializer config based on the current Symfony platform and whether the Serializer component is installed or not, like other config with similar behaviour.

Tests included :)

Cheers!

Commits
-------

a05e2e2 Relax Messenger config and fix some bugs
@yceruto yceruto deleted the relax_messenger_config branch May 7, 2018 11:18
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.

6 participants