[Messenger] Middleware factories support in config #27128

ogizanagi · 2018-05-02T19:49:00Z

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

Following #26864, this would allow to configure easily the middlewares by using an abstract factory definition to which are provided simple arguments (just scalars, no services references).

For instance, here is how the DoctrineBundle would benefit from such a feature (also solving the wiring of the DoctrineTransactionMiddleware reverted in #26684):

framework:
    messenger:
      buses:
        default:
          middleware:
            - logger
            - doctrine_transaction_middleware: ['entity_manager_name']

where doctrine_transaction_middleware would be an abstract factory definition provided by the doctrine bundle:

services:

    doctrine.orm.messenger.middleware_factory.transaction:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddlewareFactory
      arguments: ['@doctrine']

    doctrine_transaction_middleware:
      class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddleware
      factory: ['@doctrine.orm.messenger.middleware_factory.transaction', 'createMiddleware']
      abstract: true 
      # the default arguments to use when none provided from config.
      # i.e: 
      #   middlewares:
      #     - doctrine_transaction_middleware: ~
      arguments: ['default']

and is interpreted as:

buses:
    default:
        middleware:
            -
                id: logger
                arguments: {  }
            -
                id: doctrine_transaction_middleware
                arguments:
                    - entity_manager_name
        default_middleware: true

Here is the whole config reference with these changes:

# Messenger configuration
messenger:
    enabled:              true
    routing:

        # Prototype
        message_class:
            senders:              []
    serializer:
        enabled:              true
        format:               json
        context:

            # Prototype
            name:                 ~
    encoder:              messenger.transport.serializer
    decoder:              messenger.transport.serializer
    adapters:

        # Prototype
        name:
            dsn:                  ~
            options:              []
    default_bus:          null
    buses:

        # Prototype
        name:
            default_middleware:  true
            middleware:

                # Prototype
                -
                    id:                   ~ # Required
                    arguments:            []

sroze · 2018-05-02T20:00:15Z

I don't see the point of the - doctrine_transaction_middleware: ['entity_manager_name'] syntax if we support - {id: doctrine_transaction_middleware, arguments: ['entity_manager_name']}. And the last one is more explicit.

ogizanagi · 2018-05-02T20:23:17Z

I don't see the point of the - doctrine_transaction_middleware: ['entity_manager_name'] syntax if we support - {id: doctrine_transaction_middleware, arguments: ['entity_manager_name']}.

Same as for keeping the simple - logger syntax, which could be replaced by - id: logger.
It's less cumbersome to read & write.

sroze · 2018-05-02T21:00:23Z

To me, using these arguments is the "advanced" way of configuring the middleware: using the object syntax with the id is simple enough that we don't need to use another confusing syntax IMHO.

-             - doctrine_transaction_middleware: ['entity_manager_name']
+             - {id: 'doctrine_transaction_middleware', arguments: ['entity_manager_name']}

Or do I miss something?

nicolas-grekas · 2018-05-02T23:07:18Z

@sroze or drop the {id: *} style altogether?

ogizanagi · 2018-05-03T06:23:37Z

@sroze : Obviously you didn't miss anything. I just like this syntax :) (and I expected end-users to have a preference for it too but that's just my guess).
So, let's have a little poll:

Current (concrete services and factories short syntaxes)

middleware:
  - logger
  - doctrine_transaction_middleware: ['entity_manager_name']

Drop the factories short syntax:

middleware:
  - logger
  - id: doctrine_transaction_middleware:
    arguments: ['entity_manager_name']

Drop every short syntaxes:

middleware:
  - id: logger
  - id: doctrine_transaction_middleware:
    arguments: ['entity_manager_name']

Drop array syntax:

middleware:
  logger: ~
  doctrine_transaction_middleware: ['entity_manager_name']

My own preference is 1. Perhaps 4.
I think I blindly excluded the last one at first because it requires unique keys, but actually I don't think it makes sense to configure the same middleware, even with a different config, in the very same bus. ~~The very same limitation still exists anyway in the current internal implementation but the syntax would allow it later if we need it.~~ Changed
The other reason is that it enforces providing ~ for concrete services (logger: ~) which are not configurable anyway but might let you think so.

sroze · 2018-05-03T06:56:49Z

Looking at it this way, 4 looks nicer but the “one only” limitation is too many assumptions IMHO. 3 is verbose and 4 not really coherent. Let’s keep 1 then, good point 😃

sroze · 2018-05-09T11:30:48Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

+                foreach (array_values($arguments ?? array()) as $key => $argument) {
+                    // Parent definition can provide default arguments.
+                    // Replace each explicitly or add if not set:
+                    $key < $count ? $childDefinition->replaceArgument($key, $argument) : $childDefinition->addArgument($argument);


Why not just using setArgument here?

Due to the way child definitions arguments work, using replaceArgument saves an extra hard coded index_ prefix for the key here.

symfony/src/Symfony/Component/DependencyInjection/ChildDefinition.php

Lines 98 to 99 in d264fce

if (is_int($index)) {

$this->arguments['index_'.$index] = $value;

sroze · 2018-05-09T11:32:26Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

@@ -202,22 +202,35 @@ private function registerBusToCollector(ContainerBuilder $container, string $bus

    private function registerBusMiddleware(ContainerBuilder $container, string $busId, array $middleware)


Could you rename to $middlewareCollection to make it more explicit?

Sure, good idea 👍 (uncountable nouns are hell in programmation)

sroze · 2018-05-09T11:33:57Z

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

-                $container->setDefinition($messengerMiddlewareId = $busId.'.middleware.'.$name, $childDefinition);
+                $container->setDefinition($messengerMiddlewareId = $busId.'.middleware.'.$id, $childDefinition);
+            } elseif ($arguments) {
+                throw new RuntimeException(sprintf('Invalid middleware factory "%s": a middleware factory must be an abstract definition.', $id));


Could we put that first as a simple if so we don't have this extra indentation?

I don't think so, it'll still need the abstract check and won't save any indent, right?

sroze · 2018-05-09T11:34:30Z

src/Symfony/Component/Messenger/Tests/DependencyInjection/MessengerPassTest.php

        $container->register(UselessMiddleware::class, UselessMiddleware::class);

-        $container->setParameter($middlewareParameter = $fooBusId.'.middleware', array(UselessMiddleware::class, 'allow_no_handler'));
+        $container->setParameter($middlewareParameter = $fooBusId.'.middleware', array(
+            array('id' => UselessMiddleware::class, 'arguments' => array()),


As you deal with the fact that arguments might not be present, shouldn't we remove it here?

The config adds the default [] value, but not the default middleware in https://github.com/ogizanagi/symfony/blob/a36dad010b7634bbe3c35f74490ced841d0fc2a1/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L1472-L1474

Should I update the extension and enforce in the pass? Or the config so it removes it if empty? Not really worth it IMHO.

NVM, understood. I thought this was the FrameworkExtensionTest class 😅

sroze · 2018-05-10T10:55:33Z

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

+                                                    $arguments = reset($middleware);
+
+                                                    return array(
+                                                        'id' => key($middleware),


I think we should throw an exception is we have multiple keys in here

The issue is that beforeNormalization isn't really designed for throwing. It's the validate responsibility which would also prepend the path. But at this point we would have already ignored the extra keys.

So we could add manually something like:

if (\count($middleware) > 1) { throw new \InvalidArgumentException(sprintf('At path "framework.messenger.buses.middleware": expected a single entry for a middleware item array, with factory id as key and arguments as value. Got "%s".', json_encode($middleware))); }

Do you think it is worth it?

I’d say, it’s easy to end up with something like that:

middleware: - foo: 1 bar: { baz: ~ }

Alright, added

sroze

Great. Needs to be in 4.1 because it changes the XML syntax for middlewares for the good 👍

sroze · 2018-05-11T10:03:11Z

Failure on deps=high is expected.

ogizanagi · 2018-05-11T12:24:18Z

I've just created the PR on doctrine/DoctrineBundle#817 to wire the services when the component is enabled.

sroze · 2018-05-14T19:17:34Z

Thank you @ogizanagi.

…izanagi) This PR was squashed before being merged into the 4.1 branch (closes #27128). Discussion ---------- [Messenger] Middleware factories support in config | Q | A | ------------- | --- | Branch? | master  | Bug fix? | no | New feature? | yes  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | N/A  | License | MIT | Doc PR | todo Following #26864, this would allow to configure easily the middlewares by using an abstract factory definition to which are provided simple arguments (just scalars, no services references). For instance, here is how the DoctrineBundle would benefit from such a feature (also solving the wiring of the `DoctrineTransactionMiddleware` reverted in #26684): ```yaml framework: messenger: buses: default: middleware: - logger - doctrine_transaction_middleware: ['entity_manager_name'] ``` where `doctrine_transaction_middleware` would be an abstract factory definition provided by the doctrine bundle: ```yml services: doctrine.orm.messenger.middleware_factory.transaction: class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddlewareFactory arguments: ['@doctrine'] doctrine_transaction_middleware: class: Symfony\Bridge\Doctrine\Messenger\DoctrineTransactionMiddleware factory: ['@doctrine.orm.messenger.middleware_factory.transaction', 'createMiddleware'] abstract: true # the default arguments to use when none provided from config. # i.e: # middlewares: # - doctrine_transaction_middleware: ~ arguments: ['default'] ``` and is interpreted as: ```yml buses: default: middleware: - id: logger arguments: { } - id: doctrine_transaction_middleware arguments: - entity_manager_name default_middleware: true ``` --- <details> <summary>Here is the whole config reference with these changes: </summary> ```yaml # Messenger configuration messenger: enabled: true routing: # Prototype message_class: senders: [] serializer: enabled: true format: json context: # Prototype name: ~ encoder: messenger.transport.serializer decoder: messenger.transport.serializer adapters: # Prototype name: dsn: ~ options: [] default_bus: null buses: # Prototype name: default_middleware: true middleware: # Prototype - id: ~ # Required arguments: [] ``` </details> Commits ------- f5ef421 [Messenger] Middleware factories support in config

…iereguiluz) This PR was merged into the 4.1 branch. Discussion ---------- [Messenger] Document middleware factories  Refs: - Symfony PR: symfony/symfony#27128 - DoctrineBundle PR: doctrine/DoctrineBundle#817 Commits ------- c5ef7c8 Reowrd to restore the original meaning a87f21b Minor rewords 2880027 [Messenger] Document middleware factories

ogizanagi added the Messenger label May 2, 2018

ogizanagi requested a review from sroze May 2, 2018 19:49

carsonbot added Status: Needs Review Feature labels May 2, 2018

nicolas-grekas added this to the 4.1 milestone May 2, 2018

ogizanagi mentioned this pull request May 7, 2018

[Messenger] Fix missing s/tolerate_no_handler/allow_no_handler/g #27176

Merged

nicolas-grekas changed the base branch from master to 4.1 May 7, 2018 15:05

carsonbot removed the Status: Needs Review label May 9, 2018

sroze suggested changes May 9, 2018

View reviewed changes

carsonbot added the Status: Needs Work label May 9, 2018

ogizanagi added Status: Needs Review and removed Status: Needs Work labels May 9, 2018

ogizanagi requested a review from lyrixx as a code owner May 9, 2018 20:06

ogizanagi removed the request for review from lyrixx May 9, 2018 20:09

carsonbot removed the Status: Needs Review label May 10, 2018

sroze approved these changes May 10, 2018

View reviewed changes

carsonbot added the Status: Reviewed label May 10, 2018

sroze approved these changes May 11, 2018

View reviewed changes

ogizanagi mentioned this pull request May 11, 2018

[Messenger] Wire the transaction middleware factory when component is enabled doctrine/DoctrineBundle#817

Merged

chalasr approved these changes May 14, 2018

View reviewed changes

[Messenger] Middleware factories support in config

f5ef421

sroze merged commit f5ef421 into symfony:4.1 May 14, 2018

ogizanagi deleted the messenger/middleware_factories branch May 14, 2018 19:21

ogizanagi mentioned this pull request May 15, 2018

[Messenger] Document middleware factories symfony/symfony-docs#9774

Merged

fabpot mentioned this pull request May 21, 2018

Release v4.1.0-BETA2 #27331

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Middleware factories support in config #27128

[Messenger] Middleware factories support in config #27128

ogizanagi commented May 2, 2018 •

edited

Loading

sroze commented May 2, 2018

ogizanagi commented May 2, 2018 •

edited

Loading

sroze commented May 2, 2018

nicolas-grekas commented May 2, 2018 •

edited

Loading

ogizanagi commented May 3, 2018 •

edited

Loading

sroze commented May 3, 2018

sroze May 9, 2018

ogizanagi May 9, 2018

sroze May 9, 2018

ogizanagi May 9, 2018

sroze May 9, 2018

ogizanagi May 9, 2018

sroze May 9, 2018

ogizanagi May 9, 2018

ogizanagi May 9, 2018

sroze May 10, 2018

ogizanagi May 10, 2018

sroze May 10, 2018 •

edited

Loading

ogizanagi May 10, 2018

sroze left a comment

sroze commented May 11, 2018

ogizanagi commented May 11, 2018

sroze commented May 14, 2018

	if (is_int($index)) {
	$this->arguments['index_'.$index] = $value;

		@@ -202,22 +202,35 @@ private function registerBusToCollector(ContainerBuilder $container, string $bus

		private function registerBusMiddleware(ContainerBuilder $container, string $busId, array $middleware)

[Messenger] Middleware factories support in config #27128

[Messenger] Middleware factories support in config #27128

Conversation

ogizanagi commented May 2, 2018 • edited Loading

sroze commented May 2, 2018

ogizanagi commented May 2, 2018 • edited Loading

sroze commented May 2, 2018

nicolas-grekas commented May 2, 2018 • edited Loading

ogizanagi commented May 3, 2018 • edited Loading

sroze commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze left a comment

Choose a reason for hiding this comment

sroze commented May 11, 2018

ogizanagi commented May 11, 2018

sroze commented May 14, 2018

ogizanagi commented May 2, 2018 •

edited

Loading

ogizanagi commented May 2, 2018 •

edited

Loading

nicolas-grekas commented May 2, 2018 •

edited

Loading

ogizanagi commented May 3, 2018 •

edited

Loading

sroze May 10, 2018 •

edited

Loading