feature #28909 [Messenger] made dispatch() and handle() return void (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Messenger] made dispatch() and handle() return void

| 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        | -

Middlewares and dispatchers should not return any value. Forwarding back the results from handlers breaks the scope of the component. The canonical example of some bad design this can lead to is `ChainHandler`: how can the caller know that there is one in the chain and that it should expect an array as a return value? More generally, how can a caller know what to expect back from a call to dispatch()? I think we should not allow such broken designs.

Instead, we should favor east-oriented design: if one needs a command bus, one would have to dispatch a proper command object - and if a result is expected back, it should be done via a setter on the dispatched command - or better, a callback set on the command object. This way we play *by the rules* of the type-system, not against.

Commits
-------

f942ffcb1b [Messenger] made dispatch() and handle() return void
This commit is contained in:
Samuel ROZE 2018-10-21 13:42:19 +01:00
commit d901c6d846
28 changed files with 71 additions and 173 deletions

View File

@ -31,7 +31,7 @@ class DoctrineTransactionMiddleware implements MiddlewareInterface
$this->entityManagerName = $entityManagerName;
}
public function handle($message, callable $next)
public function handle($message, callable $next): void
{
$entityManager = $this->managerRegistry->getManager($this->entityManagerName);
@ -41,7 +41,7 @@ class DoctrineTransactionMiddleware implements MiddlewareInterface
$entityManager->getConnection()->beginTransaction();
try {
$result = $next($message);
$next($message);
$entityManager->flush();
$entityManager->getConnection()->commit();
} catch (\Throwable $exception) {
@ -49,7 +49,5 @@ class DoctrineTransactionMiddleware implements MiddlewareInterface
throw $exception;
}
return $result;
}
}

View File

@ -168,16 +168,6 @@
{% endfor %}
</td>
</tr>
<tr>
<td class="text-bold">Result</td>
<td>
{% if dispatchCall.result is defined %}
{{ profiler_dump(dispatchCall.result.seek('value'), maxDepth=2) }}
{% elseif dispatchCall.exception is defined %}
<span class="text-danger">No result as an exception occurred</span>
{% endif %}
</td>
</tr>
{% if dispatchCall.exception is defined %}
<tr>
<td class="text-bold">Exception</td>

View File

@ -38,11 +38,13 @@ class SendMessageMiddleware implements MiddlewareInterface, EnvelopeAwareInterfa
*
* {@inheritdoc}
*/
public function handle($envelope, callable $next)
public function handle($envelope, callable $next): void
{
if ($envelope->get(ReceivedStamp::class)) {
// It's a received message. Do not send it back:
return $next($envelope);
$next($envelope);
return;
}
$sender = $this->senderLocator->getSenderForMessage($envelope->getMessage());
@ -55,7 +57,7 @@ class SendMessageMiddleware implements MiddlewareInterface, EnvelopeAwareInterfa
}
}
return $next($envelope);
$next($envelope);
}
private function mustSendAndHandle($message): bool

View File

@ -6,6 +6,7 @@ CHANGELOG
* The component is not experimental anymore
* All the changes below are BC BREAKS
* `MessageBusInterface::dispatch()` and `MiddlewareInterface::handle()` now return `void`
* The signature of `Amqp*` classes changed to take a `Connection` as a first argument and an optional
`Serializer` as a second argument.
* `SenderLocator` has been renamed to `ContainerSenderLocator`

View File

@ -98,14 +98,6 @@ class MessengerDataCollector extends DataCollector implements LateDataCollectorI
'caller' => $tracedMessage['caller'],
);
if (array_key_exists('result', $tracedMessage)) {
$result = $tracedMessage['result'];
$debugRepresentation['result'] = array(
'type' => \is_object($result) ? \get_class($result) : \gettype($result),
'value' => $result,
);
}
if (isset($tracedMessage['exception'])) {
$exception = $tracedMessage['exception'];

View File

@ -39,12 +39,8 @@ class ChainHandler
public function __invoke($message)
{
$results = array();
foreach ($this->handlers as $handler) {
$results[] = $handler($message);
$handler($message);
}
return $results;
}
}

View File

@ -38,13 +38,13 @@ class MessageBus implements MessageBusInterface
/**
* {@inheritdoc}
*/
public function dispatch($message)
public function dispatch($message): void
{
if (!\is_object($message)) {
throw new InvalidArgumentException(sprintf('Invalid type for message argument. Expected object, but got "%s".', \gettype($message)));
}
return \call_user_func($this->callableForNextMiddleware(0, Envelope::wrap($message)), $message);
\call_user_func($this->callableForNextMiddleware(0, Envelope::wrap($message)), $message);
}
private function callableForNextMiddleware(int $index, Envelope $currentEnvelope): callable
@ -71,7 +71,7 @@ class MessageBus implements MessageBusInterface
$message = $message->getMessage();
}
return $middleware->handle($message, $this->callableForNextMiddleware($index + 1, $currentEnvelope));
$middleware->handle($message, $this->callableForNextMiddleware($index + 1, $currentEnvelope));
};
}
}

View File

@ -19,11 +19,7 @@ interface MessageBusInterface
/**
* Dispatches the given message.
*
* The bus can return a value coming from handlers, but is not required to do so.
*
* @param object|Envelope $message The message or the message pre-wrapped in an envelope
*
* @return mixed
*/
public function dispatch($message);
public function dispatch($message): void;
}

View File

@ -18,10 +18,10 @@ use Symfony\Component\Messenger\Exception\NoHandlerForMessageException;
*/
class AllowNoHandlerMiddleware implements MiddlewareInterface
{
public function handle($message, callable $next)
public function handle($message, callable $next): void
{
try {
return $next($message);
$next($message);
} catch (NoHandlerForMessageException $e) {
// We allow not having a handler for this message.
}

View File

@ -37,12 +37,12 @@ class ActivationMiddlewareDecorator implements MiddlewareInterface, EnvelopeAwar
/**
* @param Envelope $envelope
*/
public function handle($envelope, callable $next)
public function handle($envelope, callable $next): void
{
if (\is_callable($this->activated) ? ($this->activated)($envelope) : $this->activated) {
return $this->inner->handle($envelope->getMessageFor($this->inner), $next);
$this->inner->handle($envelope->getMessageFor($this->inner), $next);
} else {
$next($envelope);
}
return $next($envelope);
}
}

View File

@ -39,7 +39,7 @@ class TraceableMiddleware implements MiddlewareInterface, EnvelopeAwareInterface
/**
* @param Envelope $envelope
*/
public function handle($envelope, callable $next)
public function handle($envelope, callable $next): void
{
$class = \get_class($this->inner);
$eventName = 'c' === $class[0] && 0 === strpos($class, "class@anonymous\0") ? get_parent_class($class).'@anonymous' : $class;
@ -51,19 +51,15 @@ class TraceableMiddleware implements MiddlewareInterface, EnvelopeAwareInterface
$this->stopwatch->start($eventName, $this->eventCategory);
try {
$result = $this->inner->handle($envelope->getMessageFor($this->inner), function ($message) use ($next, $eventName) {
$this->inner->handle($envelope->getMessageFor($this->inner), function ($message) use ($next, $eventName) {
$this->stopwatch->stop($eventName);
$result = $next($message);
$next($message);
$this->stopwatch->start($eventName, $this->eventCategory);
return $result;
});
} finally {
if ($this->stopwatch->isStarted($eventName)) {
$this->stopwatch->stop($eventName);
}
}
return $result;
}
}

View File

@ -28,13 +28,11 @@ class HandleMessageMiddleware implements MiddlewareInterface
/**
* {@inheritdoc}
*/
public function handle($message, callable $next)
public function handle($message, callable $next): void
{
$handler = $this->messageHandlerResolver->resolve($message);
$result = $handler($message);
$handler($message);
$next($message);
return $result;
}
}

View File

@ -28,12 +28,12 @@ class LoggingMiddleware implements MiddlewareInterface
/**
* {@inheritdoc}
*/
public function handle($message, callable $next)
public function handle($message, callable $next): void
{
$this->logger->debug('Starting handling message {class}', $this->createContext($message));
try {
$result = $next($message);
$next($message);
} catch (\Throwable $e) {
$this->logger->warning('An exception occurred while handling message {class}', array_merge(
$this->createContext($message),
@ -44,8 +44,6 @@ class LoggingMiddleware implements MiddlewareInterface
}
$this->logger->debug('Finished handling message {class}', $this->createContext($message));
return $result;
}
private function createContext($message): array

View File

@ -18,8 +18,6 @@ interface MiddlewareInterface
{
/**
* @param object $message
*
* @return mixed
*/
public function handle($message, callable $next);
public function handle($message, callable $next): void;
}

View File

@ -32,7 +32,7 @@ class ValidationMiddleware implements MiddlewareInterface, EnvelopeAwareInterfac
/**
* @param Envelope $envelope
*/
public function handle($envelope, callable $next)
public function handle($envelope, callable $next): void
{
$message = $envelope->getMessage();
$groups = null;
@ -46,6 +46,6 @@ class ValidationMiddleware implements MiddlewareInterface, EnvelopeAwareInterfac
throw new ValidationFailedException($message, $violations);
}
return $next($envelope);
$next($envelope);
}
}

View File

@ -33,15 +33,12 @@ class MessengerDataCollectorTest extends TestCase
$this->dumper->setColors(false);
}
/**
* @dataProvider getHandleTestData
*/
public function testHandle($returnedValue, $expected)
public function testHandle()
{
$message = new DummyMessage('dummy message');
$bus = $this->getMockBuilder(MessageBusInterface::class)->getMock();
$bus->method('dispatch')->with($message)->willReturn($returnedValue);
$bus->method('dispatch')->with($message);
$bus = new TraceableMessageBus($bus);
$collector = new MessengerDataCollector();
@ -54,13 +51,9 @@ class MessengerDataCollectorTest extends TestCase
$messages = $collector->getMessages();
$this->assertCount(1, $messages);
$this->assertStringMatchesFormat($expected, $this->getDataAsString($messages[0]));
}
public function getHandleTestData()
{
$file = __FILE__;
$messageDump = <<<DUMP
$expected = <<<DUMP
array:4 [
"bus" => "default"
"stamps" => null
"message" => array:2 [
@ -74,48 +67,10 @@ class MessengerDataCollectorTest extends TestCase
"file" => "$file"
"line" => %d
]
]
DUMP;
yield 'no returned value' => array(
null,
<<<DUMP
array:5 [
$messageDump
"result" => array:2 [
"type" => "NULL"
"value" => null
]
]
DUMP
);
yield 'scalar returned value' => array(
'returned value',
<<<DUMP
array:5 [
$messageDump
"result" => array:2 [
"type" => "string"
"value" => "returned value"
]
]
DUMP
);
yield 'array returned value' => array(
array('returned value'),
<<<DUMP
array:5 [
$messageDump
"result" => array:2 [
"type" => "array"
"value" => array:1 [
0 => "returned value"
]
]
]
DUMP
);
$this->assertStringMatchesFormat($expected, $this->getDataAsString($messages[0]));
}
public function testHandleWithException()

View File

@ -852,8 +852,8 @@ class HandlerOnUndefinedBus implements MessageSubscriberInterface
class UselessMiddleware implements MiddlewareInterface
{
public function handle($message, callable $next)
public function handle($message, callable $next): void
{
return $next($message);
$next($message);
}
}

View File

@ -17,7 +17,7 @@ use Symfony\Component\Messenger\Tests\Fixtures\DummyMessage;
class ChainHandlerTest extends TestCase
{
public function testItCallsTheHandlersAndReturnsAllResults()
public function testItCallsTheHandlers()
{
$message = new DummyMessage('Hey');
@ -26,19 +26,15 @@ class ChainHandlerTest extends TestCase
->expects($this->once())
->method('__invoke')
->with($message)
->willReturn('Hello')
;
$handler2 = $this->createPartialMock(\stdClass::class, array('__invoke'));
$handler2
->expects($this->once())
->method('__invoke')
->with($message)
->willReturn('World')
;
$results = (new ChainHandler(array($handler1, $handler2)))($message);
$this->assertSame(array('Hello', 'World'), $results);
(new ChainHandler(array($handler1, $handler2)))($message);
}
/**

View File

@ -39,7 +39,7 @@ class MessageBusTest extends TestCase
(new MessageBus())->dispatch('wrong');
}
public function testItCallsMiddlewareAndChainTheReturnValue()
public function testItCallsMiddleware()
{
$message = new DummyMessage('Hello');
$responseFromDepthMiddleware = 1234;
@ -49,21 +49,21 @@ class MessageBusTest extends TestCase
->method('handle')
->with($message, $this->anything())
->will($this->returnCallback(function ($message, $next) {
return $next($message);
$next($message);
}));
$secondMiddleware = $this->getMockBuilder(MiddlewareInterface::class)->getMock();
$secondMiddleware->expects($this->once())
->method('handle')
->with($message, $this->anything())
->willReturn($responseFromDepthMiddleware);
;
$bus = new MessageBus(array(
$firstMiddleware,
$secondMiddleware,
));
$this->assertEquals($responseFromDepthMiddleware, $bus->dispatch($message));
$bus->dispatch($message);
}
public function testItKeepsTheEnvelopeEvenThroughAMiddlewareThatIsNotEnvelopeAware()
@ -76,7 +76,7 @@ class MessageBusTest extends TestCase
->method('handle')
->with($message, $this->anything())
->will($this->returnCallback(function ($message, $next) {
return $next($message);
$next($message);
}));
$secondMiddleware = $this->getMockBuilder(array(MiddlewareInterface::class, EnvelopeAwareInterface::class))->getMock();
@ -104,7 +104,7 @@ class MessageBusTest extends TestCase
->method('handle')
->with($envelope, $this->anything())
->will($this->returnCallback(function ($message, $next) {
return $next($message->with(new AnEnvelopeStamp()));
$next($message->with(new AnEnvelopeStamp()));
}));
$secondMiddleware = $this->getMockBuilder(MiddlewareInterface::class)->getMock();
@ -112,7 +112,7 @@ class MessageBusTest extends TestCase
->method('handle')
->with($message, $this->anything())
->will($this->returnCallback(function ($message, $next) {
return $next($message);
$next($message);
}));
$thirdMiddleware = $this->getMockBuilder(array(MiddlewareInterface::class, EnvelopeAwareInterface::class))->getMock();
@ -143,7 +143,7 @@ class MessageBusTest extends TestCase
->method('handle')
->with($message, $this->anything())
->will($this->returnCallback(function ($message, $next) use ($changedMessage) {
return $next($changedMessage);
$next($changedMessage);
}));
$secondMiddleware = $this->getMockBuilder(array(MiddlewareInterface::class, EnvelopeAwareInterface::class))->getMock();

View File

@ -18,15 +18,15 @@ use Symfony\Component\Messenger\Tests\Fixtures\DummyMessage;
class AllowNoHandlerMiddlewareTest extends TestCase
{
public function testItCallsNextMiddlewareAndReturnsItsResult()
public function testItCallsNextMiddleware()
{
$message = new DummyMessage('Hey');
$next = $this->createPartialMock(\stdClass::class, array('__invoke'));
$next->expects($this->once())->method('__invoke')->with($message)->willReturn('Foo');
$next->expects($this->once())->method('__invoke')->with($message);
$middleware = new AllowNoHandlerMiddleware();
$this->assertSame('Foo', $middleware->handle($message, $next));
$middleware->handle($message, $next);
}
public function testItCatchesTheNoHandlerException()

View File

@ -32,11 +32,11 @@ class ActivationMiddlewareDecoratorTest extends TestCase
$next->expects($this->never())->method('__invoke');
$middleware = $this->createMock(MiddlewareInterface::class);
$middleware->expects($this->once())->method('handle')->with($message, $next)->willReturn('Hello from middleware');
$middleware->expects($this->once())->method('handle')->with($message, $next);
$decorator = new ActivationMiddlewareDecorator($middleware, true);
$this->assertSame('Hello from middleware', $decorator->handle($envelope, $next));
$decorator->handle($envelope, $next);
}
public function testExecuteMiddlewareOnActivatedWithCallable()
@ -51,11 +51,11 @@ class ActivationMiddlewareDecoratorTest extends TestCase
$next->expects($this->never())->method('__invoke');
$middleware = $this->createMock(MiddlewareInterface::class);
$middleware->expects($this->once())->method('handle')->with($message, $next)->willReturn('Hello from middleware');
$middleware->expects($this->once())->method('handle')->with($message, $next);
$decorator = new ActivationMiddlewareDecorator($middleware, $activated);
$this->assertSame('Hello from middleware', $decorator->handle($envelope, $next));
$decorator->handle($envelope, $next);
}
public function testExecuteEnvelopeAwareMiddlewareWithEnvelope()
@ -67,11 +67,11 @@ class ActivationMiddlewareDecoratorTest extends TestCase
$next->expects($this->never())->method('__invoke');
$middleware = $this->createMock(array(MiddlewareInterface::class, EnvelopeAwareInterface::class));
$middleware->expects($this->once())->method('handle')->with($envelope, $next)->willReturn('Hello from middleware');
$middleware->expects($this->once())->method('handle')->with($envelope, $next);
$decorator = new ActivationMiddlewareDecorator($middleware, true);
$this->assertSame('Hello from middleware', $decorator->handle($envelope, $next));
$decorator->handle($envelope, $next);
}
public function testExecuteMiddlewareOnDeactivated()
@ -80,13 +80,13 @@ class ActivationMiddlewareDecoratorTest extends TestCase
$envelope = Envelope::wrap($message);
$next = $this->createPartialMock(\stdClass::class, array('__invoke'));
$next->expects($this->once())->method('__invoke')->with($envelope)->willReturn('Hello from $next');
$next->expects($this->once())->method('__invoke')->with($envelope);
$middleware = $this->createMock(MiddlewareInterface::class);
$middleware->expects($this->never())->method('handle');
$decorator = new ActivationMiddlewareDecorator($middleware, false);
$this->assertSame('Hello from $next', $decorator->handle($envelope, $next));
$decorator->handle($envelope, $next);
}
}

View File

@ -33,7 +33,7 @@ class TraceableMiddlewareTest extends TestCase
->method('handle')
->with($message, $this->anything())
->will($this->returnCallback(function ($message, callable $next) {
return $next($message);
$next($message);
}))
;
@ -42,7 +42,6 @@ class TraceableMiddlewareTest extends TestCase
->expects($this->once())
->method('__invoke')
->with($message)
->willReturn($expectedReturnedValue = 'Hello')
;
$stopwatch = $this->createMock(Stopwatch::class);
@ -58,7 +57,7 @@ class TraceableMiddlewareTest extends TestCase
$traced = new TraceableMiddleware($middleware, $stopwatch, $busId);
$this->assertSame($expectedReturnedValue, $traced->handle($envelope, $next));
$traced->handle($envelope, $next);
}
/**
@ -75,7 +74,7 @@ class TraceableMiddlewareTest extends TestCase
->method('handle')
->with($message, $this->anything())
->will($this->returnCallback(function ($message, callable $next) {
return $next($message);
$next($message);
}))
;

View File

@ -23,7 +23,6 @@ class HandleMessageMiddlewareTest extends TestCase
$message = new DummyMessage('Hey');
$handler = $this->createPartialMock(\stdClass::class, array('__invoke'));
$handler->method('__invoke')->willReturn('Hello');
$next = $this->createPartialMock(\stdClass::class, array('__invoke'));

View File

@ -32,12 +32,9 @@ class LoggingMiddlewareTest extends TestCase
->expects($this->once())
->method('__invoke')
->with($message)
->willReturn('Hello')
;
$result = (new LoggingMiddleware($logger))->handle($message, $next);
$this->assertSame('Hello', $result);
(new LoggingMiddleware($logger))->handle($message, $next);
}
/**

View File

@ -38,12 +38,9 @@ class ValidationMiddlewareTest extends TestCase
->expects($this->once())
->method('__invoke')
->with($envelope)
->willReturn('Hello')
;
$result = (new ValidationMiddleware($validator))->handle($envelope, $next);
$this->assertSame('Hello', $result);
(new ValidationMiddleware($validator))->handle($envelope, $next);
}
public function testValidateWithStampAndNextMiddleware()
@ -61,12 +58,9 @@ class ValidationMiddlewareTest extends TestCase
->expects($this->once())
->method('__invoke')
->with($envelope)
->willReturn('Hello')
;
$result = (new ValidationMiddleware($validator))->handle($envelope, $next);
$this->assertSame('Hello', $result);
(new ValidationMiddleware($validator))->handle($envelope, $next);
}
/**

View File

@ -20,20 +20,18 @@ use Symfony\Component\Messenger\TraceableMessageBus;
class TraceableMessageBusTest extends TestCase
{
public function testItTracesResult()
public function testItTracesDispatch()
{
$message = new DummyMessage('Hello');
$bus = $this->getMockBuilder(MessageBusInterface::class)->getMock();
$bus->expects($this->once())->method('dispatch')->with($message)->willReturn($result = array('foo' => 'bar'));
$traceableBus = new TraceableMessageBus($bus);
$line = __LINE__ + 1;
$this->assertSame($result, $traceableBus->dispatch($message));
$this->assertNull($traceableBus->dispatch($message));
$this->assertCount(1, $tracedMessages = $traceableBus->getDispatchedMessages());
$this->assertArraySubset(array(
'message' => $message,
'result' => $result,
'stamps' => null,
'caller' => array(
'name' => 'TraceableMessageBusTest.php',
@ -43,20 +41,18 @@ class TraceableMessageBusTest extends TestCase
), $tracedMessages[0], true);
}
public function testItTracesResultWithEnvelope()
public function testItTracesDispatchWithEnvelope()
{
$envelope = Envelope::wrap($message = new DummyMessage('Hello'))->with($stamp = new AnEnvelopeStamp());
$bus = $this->getMockBuilder(MessageBusInterface::class)->getMock();
$bus->expects($this->once())->method('dispatch')->with($envelope)->willReturn($result = array('foo' => 'bar'));
$traceableBus = new TraceableMessageBus($bus);
$line = __LINE__ + 1;
$this->assertSame($result, $traceableBus->dispatch($envelope));
$this->assertNull($traceableBus->dispatch($envelope));
$this->assertCount(1, $tracedMessages = $traceableBus->getDispatchedMessages());
$this->assertArraySubset(array(
'message' => $message,
'result' => $result,
'stamps' => array($stamp),
'caller' => array(
'name' => 'TraceableMessageBusTest.php',

View File

@ -29,7 +29,7 @@ $connection = Connection::fromDsn(getenv('DSN'));
$receiver = new AmqpReceiver($connection, $serializer);
$worker = new Worker($receiver, new class() implements MessageBusInterface {
public function dispatch($envelope)
public function dispatch($envelope): void
{
echo 'Get envelope with message: '.get_class($envelope->getMessage())."\n";
echo sprintf("with stamps: %s\n", json_encode(array_keys($envelope->all()), JSON_PRETTY_PRINT));

View File

@ -27,7 +27,7 @@ class TraceableMessageBus implements MessageBusInterface
/**
* {@inheritdoc}
*/
public function dispatch($message)
public function dispatch($message): void
{
$caller = $this->getCaller();
$callTime = microtime(true);
@ -35,17 +35,14 @@ class TraceableMessageBus implements MessageBusInterface
$stamps = $message instanceof Envelope ? array_values($message->all()) : null;
try {
$result = $this->decoratedBus->dispatch($message);
$this->decoratedBus->dispatch($message);
$this->dispatchedMessages[] = array(
'stamps' => $stamps,
'message' => $messageToTrace,
'result' => $result,
'callTime' => $callTime,
'caller' => $caller,
);
return $result;
} catch (\Throwable $e) {
$this->dispatchedMessages[] = array(
'stamps' => $stamps,