8000 [Messenger] Adding support for record messages by Nyholm · Pull Request #27844 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Messenger] Adding support for record messages #27844

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
11 changes: 11 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.xml
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@
<argument type="service" id="validator" />
</service>

<service id="messenger.middleware.record_messages" class="Symfony\Component\Messenger\Middleware\HandleRecordedMessageMiddleware" abstract="true">
<argument /> <!-- Message bus -->
<argument type="service" id="messenger.recorder" />
</service>

<!-- Logging -->
<service id="messenger.middleware.logging" class="Symfony\Component\Messenger\Middleware\LoggingMiddleware" abstract="true">
<argument type="service" id="logger" />
Expand Down Expand Up @@ -62,5 +67,11 @@

<tag name="messenger.transport_factory" />
</service>

<!-- Message recorder -->
<service id="messenger.recorder" class="Symfony\Component\Messenger\MessageRecorder">
<tag name="kernel.reset" method="reset" />
</service>
<service id="Symfony\Component\Messenger\MessageRecorderInterface" alias="messenger.recorder" />
</services>
</container>
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<?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;

/**
* When handling messages, some handlers caused an exception. This exception
* contains all those handler exceptions.
*
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
class MessageHandlingException extends \RuntimeException implements ExceptionInterface
{
private $exceptions = array();

public function __construct(array $exceptions)
{
$message = sprintf(
"Some handlers for recorded messages threw an exception. Their messages were: \n\n%s",
implode(", \n", array_map(function (\Throwable $e) {
return $e->getMessage();
}, $exceptions))
);

$this->exceptions = $exceptions;
parent::__construct($message);
}

public function getExceptions(): array

Choose a reason for hiding this comment

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

toArray would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, Im not sure

{
return $this->exceptions;
}
}
55 changes: 55 additions & 0 deletions src/Symfony/Component/Messenger/MessageRecorder.php
< 8000 /tr> 9E88
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?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;

use Symfony\Contracts\Service\ResetInterface;

/**
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
* @author Matthias Noback <matthiasnoback@gmail.com>
*/
class MessageRecorder implements MessageRecorderInterface, RecordedMessageCollectionInterface, ResetInterface
{
private $messages = array();

/**
* {@inheritdoc}
*/
public function getRecordedMessages(): array

Choose a reason for hiding this comment

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

I am not fan of getter, it does not follow the tell, don't ask principle. IMOH recordedMessages is enough/better :)

Copy link
Member Author
@Nyholm Nyholm Sep 9, 2018

Choose a reason for hiding this comment

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

As far as I know, this is the "symfony way". Im not sure if I can apply "tell, don't ask principle", even though we may like it more at the moment.

Choose a reason for hiding this comment

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

ok I understand :(

{
return $this->messages;
}

/**
* {@inheritdoc}
*/
public function resetRecordedMessages(): void
{
$this->reset();
}

/**
* {@inheritdoc}
*/
public function reset()

Choose a reason for hiding this comment

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

Why do you keep this method? You should remove it. Like I said, resetdoes not mean anything for a domain object.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. But I need this service to implement ResettableInterface. You do not need to implement this method in your domain objects. I removed it from the interface. =)

Choose a reason for hiding this comment

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

ok 👍

{
$this->messages = array();
}

/**
* {@inheritdoc}
*/
public function record($message): void
{
$this->messages[] = $message;
}
}
26 changes: 26 additions & 0 deletions src/Symfony/Component/Messenger/MessageRecorderInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?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;

/**
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
* @author Matthias Noback <matthiasnoback@gmail.com>
*/
interface MessageRecorderInterface
{
/**
* Record a message.
*
* @param object $message
*/
public function record($message);

Choose a reason for hiding this comment

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

Add : void as function signature (because of interface).

Copy link
Contributor
@sroze sroze Jul 20, 2018

Choose a reason for hiding this comment

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

I don't see any value in adding : void tbh. It might be an issue if some recorders want to return something? (which might be needed in some case?)

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?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\MessageHandlingException;
use Symfony\Component\Messenger\MessageBusInterface;
use Symfony\Component\Messenger\RecordedMessageCollectionInterface;

/**
* A middleware that takes all recorded messages and dispatch them to the bus.
*
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
* @author Matthias Noback <matthiasnoback@gmail.com>
*/
class HandleRecordedMessageMiddleware implements MiddlewareInterface
{
private $messageRecorder;
private $messageBus;

public function __construct(MessageBusInterface $messageBus, RecordedMessageCollectionInterface $messageRecorder)
{
$this->messageRecorder = $messageRecorder;
$this->messageBus = $messageBus;
}

public function handle($message, callable $next)
{
// Make sure the recorder is empty before we begin
$this->messageRecorder->resetRecordedMessages();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's imagine the following example:

  1. Controller dispatches a FirstMessage message.
  2. FirstMessageHandler gets called and it does the following:
    $recorder->record(new SecondMessage);
    $bus->dispatch(new SaveToSecondaryDatabase);
    $recorder->record(new ThirdMessage);

The dispatch will call the middleware and the just recorded message will be erased, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is correct IF the current bus and $bus is using the same MessageRecorder.

I think I should remove this line. Are you okey with that @ogizanagi? It was added by your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be fine removing it, but the case described above is a more complex topic then.
For instance, if there is any issue with FirstMessageHandler after SaveToSecondaryDatabase, SecondMessage
(and possibly those recorded by SaveToSecondaryDatabase) would still have been executed while I would expect it does only if FirstMessageHandler is fully successful.
Actually, to me recorded messages should be treated if and only if the highest dispatch call is successful. Possibly by using an internal flag inside the middleware? (Thus, we could even keep the reset for safety, only when the flag is off).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Which is equivalent to the FinishesHandlingMessageBeforeHandlingNext middleware of SimpleBus. (I prefer SimpleBus' naming in here, it's more explicit)


try {
$returnData = $next($message);
} catch (\Throwable $exception) {
$this->messageRecorder->resetRecordedMessages();

throw $exception;
}

$exceptions = array();
while (!empty($recordedMessages = $this->messageRecorder->getRecordedMessages())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reasoning of get + reset instead of pop?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to make sure nobody runs $this->messageRecorder->resetRecordedMessages(); while I'm handling the recorded messages.

$this->messageRecorder->resetRecordedMessages();
// Assert: The message recorder is empty, all messages are in $recordedMessages

foreach ($recordedMessages as $recordedMessage) {
try {
$this->messageBus->dispatch($recordedMessage);
} catch (\Throwable $exception) {
$exceptions[] = $exception;
}
}
}

if (!empty($exceptions)) {
if (1 === \count($exceptions)) {
throw $exceptions[0];
}
throw new MessageHandlingException($exceptions);
}

return $returnData;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?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;

/**
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
* @author Matthias Noback <matthiasnoback@gmail.com>
*/
interface RecordedMessageCollectionInterface
{
/**
* Fetch recorded messages.
*
* @return object[]
*/
public function getRecordedMessages(): array;

/**
* Remove all recorded messages.
*/
public function resetRecordedMessages(): void;
}
Loading
0