feature #28945 [Messenger] remove AllowNoHandlerMiddleware in favor of a constructor argument on HandleMessageMiddleware (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Messenger] remove AllowNoHandlerMiddleware in favor of a constructor argument on HandleMessageMiddleware

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

`AllowNoHandlerMiddleware` adds a frame in the call stack but its concern should be handled by `HandleMessageMiddleware` to me, and `HandlerLocatorInterface::getHandler()` should not care about whether it's an error or not to not have a middleware (this makes it on par with `SenderLocator` in this regard.)

Commits
-------

aedb281b76 [Messenger] remove AllowNoHandlerMiddleware in favor of a constructor argument on HandleMessageMiddleware

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

@@ -1081,19 +1081,20 @@ class Configuration implements ConfigurationInterface
                         ->arrayNode('buses')
                             ->defaultValue(array('messenger.bus.default' => array('default_middleware' => true, 'middleware' => array())))
                             ->useAttributeAsKey('name')
-                            ->prototype('array')
+                            ->arrayPrototype()
                                 ->addDefaultsIfNotSet()
                                 ->children()
-                                    ->booleanNode('default_middleware')->defaultTrue()->end()
+                                    ->enumNode('default_middleware')
+                                        ->values(array(true, false, 'allow_no_handlers'))
+                                        ->defaultTrue()
+                                    ->end()
                                     ->arrayNode('middleware')
                                         ->beforeNormalization()
-                                            ->ifString()
-                                            ->then(function (string $middleware) {
-                                                return array($middleware);
-                                            })
+                                            ->ifTrue(function ($v) { return \is_string($v) || !\is_int(key($v)); })
+                                            ->then(function ($v) { return array($v); })
                                         ->end()
                                         ->defaultValue(array())
-                                        ->prototype('array')
+                                        ->arrayPrototype()
                                             ->beforeNormalization()
                                                 ->always()
                                                 ->then(function ($middleware): array {
@@ -1103,8 +1104,8 @@ class Configuration implements ConfigurationInterface
                                                     if (isset($middleware['id'])) {
                                                         return $middleware;
                                                     }
-                                                    if (\count($middleware) > 1) {
-                                                        throw new \InvalidArgumentException(sprintf('There is an error at path "framework.messenger" in one of the buses middleware definitions: expected a single entry for a middleware item config, with factory id as key and arguments as value. Got "%s".', json_encode($middleware)));
+                                                    if (1 < \count($middleware)) {
+                                                        throw new \InvalidArgumentException(sprintf('Invalid middleware at path "framework.messenger": a map with a single factory id as key and its arguments as value was expected, %s given.', json_encode($middleware)));
                                                     }
 
                                                     return array(

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

@@ -1524,7 +1524,16 @@ class FrameworkExtension extends Extension
             'after' => array(array('id' => 'route_messages'), array('id' => 'call_message_handler')),
         );
         foreach ($config['buses'] as $busId => $bus) {
-            $middleware = $bus['default_middleware'] ? array_merge($defaultMiddleware['before'], $bus['middleware'], $defaultMiddleware['after']) : $bus['middleware'];
+            $middleware = $bus['middleware'];
+
+            if ($bus['default_middleware']) {
+                if ('allow_no_handlers' === $bus['default_middleware']) {
+                    $defaultMiddleware['after'][1]['arguments'] = array(true);
+                } else {
+                    unset($defaultMiddleware['after'][1]['arguments']);
+                }
+                $middleware = array_merge($defaultMiddleware['before'], $middleware, $defaultMiddleware['after']);
+            }
 
             foreach ($middleware as $middlewareItem) {
                 if (!$validationConfig['enabled'] && 'messenger.middleware.validation' === $middlewareItem['id']) {

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.xml

@@ -25,7 +25,6 @@
         </service>
 
         <!-- Middleware -->
-        <service id="messenger.middleware.allow_no_handler" class="Symfony\Component\Messenger\Middleware\AllowNoHandlerMiddleware" abstract="true" />
         <service id="messenger.middleware.call_message_handler" class="Symfony\Component\Messenger\Middleware\HandleMessageMiddleware" abstract="true">
             <argument /> <!-- Bus handler resolver -->
         </service>

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

@@ -331,6 +331,16 @@
         </xsd:sequence>
     </xsd:complexType>
 
+    <xsd:simpleType name="default_middleware">
+        <xsd:restriction base="xsd:string">
+            <xsd:enumeration value="true" />
+            <xsd:enumeration value="false" />
+            <xsd:enumeration value="1" />
+            <xsd:enumeration value="0" />
+            <xsd:enumeration value="allow_no_handlers" />
+        </xsd:restriction>
+    </xsd:simpleType>
+
     <xsd:simpleType name="cookie_secure">
         <xsd:restriction base="xsd:string">
             <xsd:enumeration value="true" />
@@ -408,7 +418,7 @@
             <xsd:element name="middleware" type="messenger_middleware" minOccurs="0" maxOccurs="unbounded" />
         </xsd:sequence>
         <xsd:attribute name="name" type="xsd:string" use="required"/>
-        <xsd:attribute name="default-middleware" type="xsd:boolean"/>
+        <xsd:attribute name="default-middleware" type="default_middleware"/>
     </xsd:complexType>
 
     <xsd:complexType name="messenger_middleware">

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/php/messenger_multiple_buses.php

@@ -8,14 +8,12 @@ $container->loadFromExtension('framework', array(
             'messenger.bus.events' => array(
                 'middleware' => array(
                     array('with_factory' => array('foo', true, array('bar' => 'baz'))),
-                    'allow_no_handler',
                 ),
             ),
             'messenger.bus.queries' => array(
                 'default_middleware' => false,
                 'middleware' => array(
                     'route_messages',
-                    'allow_no_handler',
                     'call_message_handler',
                 ),
             ),

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/xml/messenger_multiple_buses.xml

@@ -16,11 +16,9 @@
                         <framework:bar>baz</framework:bar>
                     </framework:argument>
                 </framework:middleware>
-                <framework:middleware id="allow_no_handler" />
             </framework:bus>
             <framework:bus name="messenger.bus.queries" default-middleware="false">
                 <framework:middleware id="route_messages" />
-                <framework:middleware id="allow_no_handler" />
                 <framework:middleware id="call_message_handler" />
             </framework:bus>
         </framework:messenger>

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/Fixtures/yml/messenger_multiple_buses.yml

@@ -6,10 +6,8 @@ framework:
             messenger.bus.events:
                 middleware:
                     - with_factory: [foo, true, { bar: baz }]
-                    - "allow_no_handler"
             messenger.bus.queries:
                 default_middleware: false
                 middleware:
                     - "route_messages"
-                    - "allow_no_handler"
                     - "call_message_handler"

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

@@ -627,7 +627,6 @@ abstract class FrameworkExtensionTest extends TestCase
         $this->assertEquals(array(
             array('id' => 'logging'),
             array('id' => 'with_factory', 'arguments' => array('foo', true, array('bar' => 'baz'))),
-            array('id' => 'allow_no_handler', 'arguments' => array()),
             array('id' => 'route_messages'),
             array('id' => 'call_message_handler'),
         ), $container->getParameter('messenger.bus.events.middleware'));
@@ -635,7 +634,6 @@ abstract class FrameworkExtensionTest extends TestCase
         $this->assertSame(array(), $container->getDefinition('messenger.bus.queries')->getArgument(0));
         $this->assertEquals(array(
             array('id' => 'route_messages', 'arguments' => array()),
-            array('id' => 'allow_no_handler', 'arguments' => array()),
             array('id' => 'call_message_handler', 'arguments' => array()),
         ), $container->getParameter('messenger.bus.queries.middleware'));
 
@@ -645,7 +643,7 @@ abstract class FrameworkExtensionTest extends TestCase
 
     /**
      * @expectedException \InvalidArgumentException
-     * @expectedExceptionMessage There is an error at path "framework.messenger" in one of the buses middleware definitions: expected a single entry for a middleware item config, with factory id as key and arguments as value. Got "{"foo":["qux"],"bar":["baz"]}"
+     * @expectedExceptionMessage Invalid middleware at path "framework.messenger": a map with a single factory id as key and its arguments as value was expected, {"foo":["qux"],"bar":["baz"]} given.
      */
     public function testMessengerMiddlewareFactoryErroneousFormat()
     {

src/Symfony/Component/Messenger/CHANGELOG.md

@@ -30,7 +30,7 @@ CHANGELOG
  * `SenderLocatorInterface::getSenderForMessage()` has been replaced by `getSender(Envelope $envelope)`
  * `MessengerDataCollector::getMessages()` returns an iterable, not just an array anymore
  * `AbstractHandlerLocator` is now internal
- * `HandlerLocatorInterface::resolve()` has been replaced by `getHandler(Envelope $envelope)`
+ * `HandlerLocatorInterface::resolve()` has been replaced by `getHandler(Envelope $envelope): ?callable` and shouldn't throw when no handlers are found
  * `SenderLocatorInterface::getSenderForMessage()` has been replaced by `getSender(Envelope $envelope)`
  * `SenderInterface::send()` returns `void`
  * Classes in the `Middleware\Enhancers` sub-namespace have been moved to the `Middleware` one
@@ -39,6 +39,7 @@ CHANGELOG
  * `SenderInterface` and `ChainSender` classes have been moved to the `Transport\Sender` sub-namespace
  * `ReceiverInterface` and its implementations have been moved to the `Transport\Receiver` sub-namespace
  * `ActivationMiddlewareDecorator` has been renamed `ActivationMiddleware`
+ * `AllowNoHandlerMiddleware` has been removed in favor of a new constructor argument on `HandleMessageMiddleware`
 
 4.1.0
 -----

src/Symfony/Component/Messenger/Handler/Locator/AbstractHandlerLocator.php

@@ -12,7 +12,6 @@
 namespace Symfony\Component\Messenger\Handler\Locator;
 
 use Symfony\Component\Messenger\Envelope;
-use Symfony\Component\Messenger\Exception\NoHandlerForMessageException;
 
 /**
  * @author Miha Vrhovnik <miha.vrhovnik@gmail.com>
@@ -22,7 +21,7 @@ use Symfony\Component\Messenger\Exception\NoHandlerForMessageException;
  */
 abstract class AbstractHandlerLocator implements HandlerLocatorInterface
 {
-    public function getHandler(Envelope $envelope): callable
+    public function getHandler(Envelope $envelope): ?callable
     {
         $class = \get_class($envelope->getMessage());
 
@@ -42,7 +41,7 @@ abstract class AbstractHandlerLocator implements HandlerLocatorInterface
             }
         }
 
-        throw new NoHandlerForMessageException(sprintf('No handler for message "%s".', $class));
+        return null;
     }
 
     abstract protected function getHandlerByName(string $name): ?callable;

src/Symfony/Component/Messenger/Handler/Locator/HandlerLocatorInterface.php

@@ -12,7 +12,6 @@
 namespace Symfony\Component\Messenger\Handler\Locator;
 
 use Symfony\Component\Messenger\Envelope;
-use Symfony\Component\Messenger\Exception\NoHandlerForMessageException;
 
 /**
  * @author Samuel Roze <samuel.roze@gmail.com>
@@ -21,8 +20,6 @@ interface HandlerLocatorInterface
 {
     /**
      * Returns the handler for the given message.
-     *
-     * @throws NoHandlerForMessageException When no handler is found
      */
-    public function getHandler(Envelope $envelope): callable;
+    public function getHandler(Envelope $envelope): ?callable;
 }

src/Symfony/Component/Messenger/Middleware/AllowNoHandlerMiddleware.php

@@ -1,33 +0,0 @@
-<?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\Middleware;
-
-use Symfony\Component\Messenger\Envelope;
-use Symfony\Component\Messenger\Exception\NoHandlerForMessageException;
-
-/**
- * @author Samuel Roze <samuel.roze@gmail.com>
- */
-class AllowNoHandlerMiddleware implements MiddlewareInterface
-{
-    /**
-     * {@inheritdoc}
-     */
-    public function handle(Envelope $envelope, callable $next): void
-    {
-        try {
-            $next($envelope);
-        } catch (NoHandlerForMessageException $e) {
-            // We allow not having a handler for this message.
-        }
-    }
-}

src/Symfony/Component/Messenger/Middleware/HandleMessageMiddleware.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\Messenger\Middleware;
 
 use Symfony\Component\Messenger\Envelope;
+use Symfony\Component\Messenger\Exception\NoHandlerForMessageException;
 use Symfony\Component\Messenger\Handler\Locator\HandlerLocatorInterface;
 
 /**
@@ -20,19 +21,26 @@ use Symfony\Component\Messenger\Handler\Locator\HandlerLocatorInterface;
 class HandleMessageMiddleware implements MiddlewareInterface
 {
     private $messageHandlerLocator;
+    private $allowNoHandlers;
 
-    public function __construct(HandlerLocatorInterface $messageHandlerLocator)
+    public function __construct(HandlerLocatorInterface $messageHandlerLocator, bool $allowNoHandlers = false)
     {
         $this->messageHandlerLocator = $messageHandlerLocator;
+        $this->allowNoHandlers = $allowNoHandlers;
     }
 
     /**
      * {@inheritdoc}
+     *
+     * @throws NoHandlerForMessageException When no handler is found and $allowNoHandlers is false
      */
     public function handle(Envelope $envelope, callable $next): void
     {
-        $handler = $this->messageHandlerLocator->getHandler($envelope);
-        $handler($envelope->getMessage());
-        $next($envelope);
+        if (null !== $handler = $this->messageHandlerLocator->getHandler($envelope)) {
+            $handler($envelope->getMessage());
+            $next($envelope);
+        } elseif (!$this->allowNoHandlers) {
+            throw new NoHandlerForMessageException(sprintf('No handler for message "%s".', \get_class($envelope->getMessage())));
+        }
     }
 }

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

@@ -27,7 +27,6 @@ use Symfony\Component\Messenger\Handler\ChainHandler;
 use Symfony\Component\Messenger\Handler\MessageHandlerInterface;
 use Symfony\Component\Messenger\Handler\MessageSubscriberInterface;
 use Symfony\Component\Messenger\MessageBusInterface;
-use Symfony\Component\Messenger\Middleware\AllowNoHandlerMiddleware;
 use Symfony\Component\Messenger\Middleware\HandleMessageMiddleware;
 use Symfony\Component\Messenger\Middleware\MiddlewareInterface;
 use Symfony\Component\Messenger\Tests\Fixtures\DummyCommand;
@@ -493,7 +492,6 @@ class MessengerPassTest extends TestCase
     public function testRegistersMiddlewareFromServices()
     {
         $container = $this->getContainerBuilder($fooBusId = 'messenger.bus.foo');
-        $container->register('messenger.middleware.allow_no_handler', AllowNoHandlerMiddleware::class)->setAbstract(true);
         $container->register('middleware_with_factory', UselessMiddleware::class)->addArgument('some_default')->setAbstract(true);
         $container->register('middleware_with_factory_using_default', UselessMiddleware::class)->addArgument('some_default')->setAbstract(true);
         $container->register(UselessMiddleware::class, UselessMiddleware::class);
@@ -502,14 +500,11 @@ class MessengerPassTest extends TestCase
             array('id' => UselessMiddleware::class),
             array('id' => 'middleware_with_factory', 'arguments' => array('foo', 'bar')),
             array('id' => 'middleware_with_factory_using_default'),
-            array('id' => 'allow_no_handler'),
         ));
 
         (new MessengerPass())->process($container);
         (new ResolveChildDefinitionsPass())->process($container);
 
-        $this->assertTrue($container->hasDefinition($childMiddlewareId = $fooBusId.'.middleware.allow_no_handler'));
-
         $this->assertTrue($container->hasDefinition($factoryChildMiddlewareId = $fooBusId.'.middleware.middleware_with_factory'));
         $this->assertEquals(
             array('foo', 'bar'),
@@ -528,7 +523,6 @@ class MessengerPassTest extends TestCase
             new Reference(UselessMiddleware::class),
             new Reference($factoryChildMiddlewareId),
             new Reference($factoryWithDefaultChildMiddlewareId),
-            new Reference($childMiddlewareId),
         ), $container->getDefinition($fooBusId)->getArgument(0));
         $this->assertFalse($container->hasParameter($middlewareParameter));
     }

src/Symfony/Component/Messenger/Tests/Handler/Locator/ContainerHandlerLocatorTest.php

@@ -25,14 +25,10 @@ class ContainerHandlerLocatorTest extends TestCase
         $this->assertSame($handler, $resolvedHandler);
     }
 
-    /**
-     * @expectedException \Symfony\Component\Messenger\Exception\NoHandlerForMessageException
-     * @expectedExceptionMessage No handler for message "Symfony\Component\Messenger\Tests\Fixtures\DummyMessage"
-     */
-    public function testThrowsNoHandlerException()
+    public function testNoHandlersReturnsNull()
     {
         $locator = new ContainerHandlerLocator(new Container());
-        $locator->getHandler(new Envelope(new DummyMessage('Hey')));
+        $this->assertNull($locator->getHandler(new Envelope(new DummyMessage('Hey'))));
     }
 
     public function testGetHandlerViaInterface()

src/Symfony/Component/Messenger/Tests/Middleware/AllowNoHandlerMiddlewareTest.php

@@ -1,56 +0,0 @@
-<?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\Tests\Middleware;
-
-use PHPUnit\Framework\TestCase;
-use Symfony\Component\Messenger\Envelope;
-use Symfony\Component\Messenger\Exception\NoHandlerForMessageException;
-use Symfony\Component\Messenger\Middleware\AllowNoHandlerMiddleware;
-use Symfony\Component\Messenger\Tests\Fixtures\DummyMessage;
-
-class AllowNoHandlerMiddlewareTest extends TestCase
-{
-    public function testItCallsNextMiddleware()
-    {
-        $message = new DummyMessage('Hey');
-        $envelope = new Envelope($message);
-
-        $next = $this->createPartialMock(\stdClass::class, array('__invoke'));
-        $next->expects($this->once())->method('__invoke')->with($envelope);
-
-        $middleware = new AllowNoHandlerMiddleware();
-        $middleware->handle($envelope, $next);
-    }
-
-    public function testItCatchesTheNoHandlerException()
-    {
-        $next = $this->createPartialMock(\stdClass::class, array('__invoke'));
-        $next->expects($this->once())->method('__invoke')->will($this->throwException(new NoHandlerForMessageException()));
-
-        $middleware = new AllowNoHandlerMiddleware();
-
-        $this->assertNull($middleware->handle(new Envelope(new DummyMessage('Hey')), $next));
-    }
-
-    /**
-     * @expectedException \RuntimeException
-     * @expectedExceptionMessage Something went wrong.
-     */
-    public function testItDoesNotCatchOtherExceptions()
-    {
-        $next = $this->createPartialMock(\stdClass::class, array('__invoke'));
-        $next->expects($this->once())->method('__invoke')->will($this->throwException(new \RuntimeException('Something went wrong.')));
-
-        $middleware = new AllowNoHandlerMiddleware();
-        $middleware->handle(new Envelope(new DummyMessage('Hey')), $next);
-    }
-}

src/Symfony/Component/Messenger/Tests/Middleware/HandleMessageMiddlewareTest.php

@@ -37,4 +37,22 @@ class HandleMessageMiddlewareTest extends TestCase
 
         $middleware->handle($envelope, $next);
     }
+
+    /**
+     * @expectedException \Symfony\Component\Messenger\Exception\NoHandlerForMessageException
+     * @expectedExceptionMessage No handler for message "Symfony\Component\Messenger\Tests\Fixtures\DummyMessage"
+     */
+    public function testThrowsNoHandlerException()
+    {
+        $middleware = new HandleMessageMiddleware(new HandlerLocator(array()));
+
+        $middleware->handle(new Envelope(new DummyMessage('Hey')), function () {});
+    }
+
+    public function testAllowNoHandlers()
+    {
+        $middleware = new HandleMessageMiddleware(new HandlerLocator(array()), true);
+
+        $this->assertNull($middleware->handle(new Envelope(new DummyMessage('Hey')), function () {}));
+    }
 }