Skip to content

[Messenger] Add middleware allowing just for a single handler #28716

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@
<service id="messenger.middleware.call_message_handler" class="Symfony\Component\Messenger\Middleware\HandleMessageMiddleware" abstract="true">
<argument /> <!-- Bus handler resolver -->
</service>
<service id="messenger.middleware.allow_single_handler" class="Symfony\Component\Messenger\Middleware\AllowSingleHandlerMiddleware" abstract="true">
<argument /> <!-- Bus handler resolver -->
</service>

<service id="messenger.middleware.validation" class="Symfony\Component\Messenger\Middleware\ValidationMiddleware" abstract="true">
<argument type="service" id="validator" />
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Component/Messenger/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ CHANGELOG
* [BC BREAK] The `EncoderInterface` and `DecoderInterface` have been replaced by a unified `Symfony\Component\Messenger\Transport\Serialization\SerializerInterface`.
* [BC BREAK] The locator passed to `ContainerHandlerLocator` should not prefix its keys by "handler." anymore
* [BC BREAK] The `AbstractHandlerLocator::getHandler()` method uses `?callable` as return type
* Added `AllowSingleHandlerMiddleware` for message buses which behave like command buses.

4.1.0
-----
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?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\Exception;

/**
* @author Kamil Kokot <kamil@kokot.me>
*/
class MoreThanOneHandlerForMessageException extends \LogicException implements ExceptionInterface
{
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?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\Exception\MoreThanOneHandlerForMessageException;
use Symfony\Component\Messenger\Handler\ChainHandler;
use Symfony\Component\Messenger\Handler\Locator\HandlerLocatorInterface;

/**
* @author Kamil Kokot <kamil@kokot.me>
*/
class AllowSingleHandlerMiddleware implements MiddlewareInterface
{
private $messageHandlerResolver;

public function __construct(HandlerLocatorInterface $messageHandlerResolver)
{
$this->messageHandlerResolver = $messageHandlerResolver;
}

/**
* {@inheritdoc}
*/
public function handle($message, callable $next)
{
$handler = $this->messageHandlerResolver->resolve($message);

if ($handler instanceof ChainHandler) {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this is tight coupling to a specific implementation: any other handler could implement a chained behavior, isn't it?
Why do we want to restrict the possibilities here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is coupled with the FrameworkBundle implementation. Unfortunately, I don't have any idea on how to make it better (I don't think that sharing the YAML configuration is better either). @nicolas-grekas do you have an idea? 🤔

Copy link
Contributor Author

@pamil pamil Oct 6, 2018

Choose a reason for hiding this comment

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

Yeah, I also don't think this is the best solution, but I couldn't think of any other way (apart from introducing such a constraint into FrameworkBundle itself).

We want to restrict it only to one handler to prevent any issues with having two handlers handling the same command (which could happen by accident, especially if using autowiring and autoconfiguration).

throw new MoreThanOneHandlerForMessageException(sprintf('More than one handler for message "%s".', \get_class($message)));
Copy link
Contributor

Choose a reason for hiding this comment

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

The other "problem" (or challenge) with the current implementation is that we can't display which are the implementations. It would massively help the developer, isn't it?

}

return $next($message);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?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\Handler\ChainHandler;
use Symfony\Component\Messenger\Handler\Locator\HandlerLocator;
use Symfony\Component\Messenger\Middleware\AllowSingleHandlerMiddleware;
use Symfony\Component\Messenger\Tests\Fixtures\DummyMessage;

class AllowSingleHandlerMiddlewareTest extends TestCase
{
public function testItCallsNextMiddlewareAndReturnsItsResult(): void
{
$message = new DummyMessage('Hey');

$handler = $this->createPartialMock(\stdClass::class, array('__invoke'));
$handler->method('__invoke')->willReturn('Hello');

$next = $this->createPartialMock(\stdClass::class, array('__invoke'));
$next->expects($this->once())->method('__invoke')->with($message)->willReturn('Foo');

$middleware = new AllowSingleHandlerMiddleware(new HandlerLocator(array(
DummyMessage::class => $handler,
)));

$this->assertSame('Foo', $middleware->handle($message, $next));
}

/**
* @expectedException \Symfony\Component\Messenger\Exception\MoreThanOneHandlerForMessageException
* @expectedExceptionMessage More than one handler for message "Symfony\Component\Messenger\Tests\Fixtures\DummyMessage".
*/
public function testItThrowsAnExceptionIfHandlerIsChainHandler(): void
{
$next = $this->createPartialMock(\stdClass::class, array('__invoke'));

$middleware = new AllowSingleHandlerMiddleware(new HandlerLocator(array(
DummyMessage::class => new ChainHandler(array(new \stdClass(), new \stdClass())),
)));

$middleware->handle(new DummyMessage('Hey'), $next);
}
}