-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Add a MessageHandlerInterface
(multiple messages + auto-configuration)
#26685
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
[Messenger] Add a MessageHandlerInterface
(multiple messages + auto-configuration)
#26685
Conversation
7dfabd0
to
a2f09cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Smaller PR than I would have expected. For me, it adds the autoconfiguration so that users can just create a message+handler & start working. I'm very satisfied!
* | ||
* @author Samuel Roze <samuel.roze@gmail.com> | ||
*/ | ||
interface HandlerInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MessageHandlerInterface
? It's just a bit more obvious when you see the interface by itself in code (e.g. FooHandler implements MessageHandlerInterface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would match the tag name, it makes sense.
* return [ | ||
* FirstMessage::class, | ||
* SecondMessage:class | ||
* ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the first example should just show the simplest & most common use-case (1 message):
return [SomeMessage::class];
|
||
/** | ||
* Handlers can implements this interface. This allows them to be auto-configured and to | ||
* handle multiple requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multiple messages
} | ||
|
||
if (!class_exists($messageClass)) { | ||
$messageClassLocation = isset($tag['handles']) ? 'declared in your tag attribute "handles"' : sprintf('used as argument type in method "%s::__invoke()"', $r->getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might not be worth trying to hack together a fix, but the second part of this message about __invoke()
is now not necessarily the case... with the new HandlerInterface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good idea.
ee39711
to
7f08bfb
Compare
Thanks @weaverryan. Updated based on your feedbacks 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I did a quick review. Just have one comment.
* | ||
* return [ | ||
* [FirstMessage::class, 0], | ||
* [SecondMessage::class, -10], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like priorities. But we've discussed that earlier...
However, what I do miss is the fact that a single handler could handle two different messages in two different functions.
return [
FirstMessage::class => 'myFunction',
SecondMessage::class => ['myOtherFunction', -10],
];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this potential use case of using different functions. But I don’t see a real value of adding this now to be honest. I believe that if a handler gets multiple messages they would be very similar and instanceof
s within the __invoke
method should be enough.
However, if we are many to believe this should be added I’m happy to update the PR.
Do we really need this functionality. SRP anyone? |
@mvrhov : But for other types of buses, I'd personally prefer explicit psr-4 wiring rather than using a marker interface for auto configuration (and for which the command_handlers:
namespace: App\Application\
resource: '%kernel.project_dir%/src/Application/**/Handler/*CommandHandler.php'
tags:
- { name: tactician.handler, typehints: true, bus: command } |
@ogizanagi which you can do already (except the multi-bus, for now) 🙃 services:
App\Message\CommandHandler\:
resource: '../src/Message/CommandHandler/*'
tags: ['messenger.message_handler'] Source: https://github.com/Nyholm/message-component-demo/blob/master/config/services.yaml#L27-L29. |
Of course! 😄 |
@@ -19,6 +19,7 @@ | |||
use Symfony\Component\DependencyInjection\Exception\RuntimeException; | |||
use Symfony\Component\DependencyInjection\Reference; | |||
use Symfony\Component\Messenger\Handler\ChainHandler; | |||
use Symfony\Component\Messenger\Handler\MessageHandlerInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there two different but similar interfaces or is this a typo? HandlerInterface
and MessageHandlerInterface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's definitely a typo while renaming HandlerInterface
to MessageHandlerInterface
. Updated 👍
namespace Symfony\Component\Messenger\Handler; | ||
|
||
/** | ||
* Handlers can implements this interface. This allows them to be auto-configured and to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implements
-> implement
Yea, the purpose of this is basically to support autoconfiguration. This particular interface (which is optional of course), is just the best way we could think of to make that happen :). We thought about doing a PSR-4 directory thing (like was suggested a few times, e.g. #26685 (comment)), but it gets a bit trickier, as we can't guarantee a user will have this in their |
My suggestion would be if we need the interface, then Create a blank one as a marker. Then for those who would like to also handle multiple messages in one handler add another interface which extends the marker one. |
We now have a pending PR for a Messenger recipe. So the PSR-4 directory loading could be added here. Otherwise, I'd also recommend a blank marker interface. |
Using psr4 directory loading is great. I’ve been using it for about 3 years with SimpleBus (not officially supported). However, I found it hard to explain and show for people consulting my project and for new developers. Having an alternative way of configure handlers that is very (!) similar to an existing pattern (EventSubsriberInterface) does make sense. Also, I’ve noticed that I many times found myself wanting to have multiple messages handled in the same handler. Say I have messages |
I'm not sure that there is any value of having an empty interface (well, except the fancyness of having it "autoconfigured", while PSR-4 discovery is doing the job). On the other hand, it's almost the only way to be able to listen to multiple messages from the same handler. A side effect is actually that now it is auto-configurable. I guess that taking this angle is much simpler to answer "is it necessary" and "should we do it this way". |
MessageHandlerInterface
(multiple messages + auto-configuration)
7f08bfb
to
004754b
Compare
I don't see why we should introduce the possiblity to handle different messages in the same handler, if we know, like @Nyholm said, the better solution is to have two handlers that share the same logic, e.g. with a service or a trait. So to solve autoconfiguration I would prefer an empty marker interface for now. And if we need multiple messages support, we can still introduce a new interface that extends from the marker interface later. |
“This also loses the possiblity to typehint the arguments of the __invoke method,
which to me was the main reason why handlers are just callables instead of
implementing an interface.”
The idea is to be able to use both of them, not to replace it or even
enforce the interface at all.
…On Sat, 31 Mar 2018 at 23:28, Tobias Schultze ***@***.***> wrote:
I don't see why we should introduce the possiblity to handle different
messages in the same handler, if we know, like @Nyholm
<https://github.com/Nyholm> said, the better solution is to have two
handlers that share the same logic, e.g. with a service or a trait.
This also loses the possiblity to typehint the arguments of the __invoke
method, which to me was the main reason why handlers are just callables
instead of implementing an interface.
So to solve, autoconfiguration I would prefer an empty marker interface
for now. And if we need the multiple messages support, we can still
introduce a new interface that extends from the marker interface later.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#26685 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAxHEd65wk8S6LsJWiYHBFQgdJP5mpfrks5tkAMFgaJpZM4S9TOY>
.
|
I know but as I pointed out the additional thing is worse. So no point in adding something that is inferior. |
I agree with @Tobion and @ogizanagi, 👍 for a marker only. |
namespace Symfony\Component\Messenger\Handler; | ||
|
||
/** | ||
* Handlers can implement this interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should say this is a marker interface somehow. Basically so it’s a bit more clear if you look in the interface, why it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolas-grekas had an opposite argument last time, so I suggest that we don't necessarily explicit why it's here (unless we have a lot of questions and then add it 👍)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented that we should not tell about "autoconfiguration", which isn't the incompatible with Ryan's comment :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sroze Can you add some more information here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀 #26786
d157183
to
07e6bc7
Compare
…messages + auto-configuration) (sroze) This PR was squashed before being merged into the 4.1-dev branch (closes #26685). Discussion ---------- [Messenger] Add a `MessageHandlerInterface` (multiple messages + auto-configuration) | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ø | License | MIT | Doc PR | ø Based on @chalasr's PR: #26672. This reduces the hassle of registering handlers: it allows the auto-configuration with a new interface `HandlerInterface`. At the same time, it allows a handler to handle multiple messages. Commits ------- 07e6bc7 [Messenger] Add a `MessageHandlerInterface` (multiple messages + auto-configuration)
This PR was merged into the 4.1-dev branch. Discussion ---------- [Messenger] Mention the interface is a marker | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26685 (comment) | License | MIT | Doc PR | ø Replace the interface's description to explicit it is a marker. Sentence based on [other](https://github.com/symfony/symfony/blob/5129c4cf7e294b1a5ea30d6fec6e89b75396dcd2/src/Symfony/Component/Security/Guard/Token/GuardTokenInterface.php#L17) [examples](https://github.com/symfony/symfony/blob/c88158a6da6474c5e52deaa7c77210689308d37e/src/Symfony/Component/Security/Core/Encoder/SelfSaltingEncoderInterface.php#L15) [in](https://github.com/symfony/symfony/blob/9fda6d3ee3ea7f32a00a13508f9748df01c2ecc7/src/Symfony/Component/PropertyAccess/Exception/ExceptionInterface.php#L15) [the](https://github.com/symfony/symfony/blob/9fda6d3ee3ea7f32a00a13508f9748df01c2ecc7/src/Symfony/Component/Process/Exception/ExceptionInterface.php#L15) [codebase](https://github.com/symfony/symfony/blob/9fda6d3ee3ea7f32a00a13508f9748df01c2ecc7/src/Symfony/Component/OptionsResolver/Exception/ExceptionInterface.php#L15). cc @weaverryan @nicolas-grekas Commits ------- ff9153e Mention the interface is a marker
Can somebody give a good reason for adding support for handling multiple messages again? I thought several core members already said that there is no need for this? |
@Nyholm has some use cases for it from his personal experience. But, it was certainly not a requirement - so we ended up at a compromise in this PR, with the empty interface + another optional one to support handling multiple messages. |
I just used this feature in a project yesterday: this example was a "process manager". The idea is to listen to multiple messages (in the case of an event bus) and send them somewhere more or less generic (on a given aggregate) to be handled. That was great just because of this interface. |
The argument wasn't that there is no use-case. It is that there is a better solution already so doesn't need to be solved at the framework level at all. |
Also I don't see why the interface does not allow to specify the callback methods for the messages like EventSubscriberInterface. This would actually add a feature and make it more consistent. |
I don't get your point. For this use-case, what would be "the better solution" then?
I don't believe this is actually required but I'd 👍 such a PR tbh.
That's very subjective 😄 Here's one of my real-life message subscriber that I'd quite happy about: class NotificationsManager implements MessageSubscriberInterface
{
// ...
public static function getHandledMessages(): array
{
return [
PictureUploaded::class,
TravelCreated::class,
];
}
public function __invoke(TravelEvent $event)
{
$travel = $this->travelRepository->find($event->getTravelUuid());
$events = $travel->apply($event);
foreach ($events as $raisedEvent) {
$this->eventStore->append(Stream::fromTravel($travel), $raisedEvent);
}
}
} |
In your case the handler processes messages that share a common parent (or interface), e.g. The other case is that different messages that don't share a parent or interface are handled in the same handler. For this the following would already be supported and solve it at the language level instead of the framework level: class NotificationMessageAHandler implements MessageHandlerInterface
{
use NotificationHandlerTrait;
public function __invoke(MessageA $msg)
{
$this->handle($msg); // whatever the trait exposes
}
}
class NotificationMessageBHandler implements MessageHandlerInterface
{
use NotificationHandlerTrait;
public function __invoke(MessageB $msg)
{
$this->handle($msg); // whatever the trait exposes
}
} |
or just explicit config 🙄 +1 for the trait example. |
I think this is actually a very good idea: the current design of |
But then we're back to something what's done already using individual handlers. Tend to like handlers being invokables from an architecture pov. Then again no real reason to force it i guess. |
Yes. Just the consistency argument is IMHO enough to add this.
Agree with you @Tobion.
That's where I believe that this two-class + trait approach is not ideal in terms of DX. It isn't a bad practice either, if you know what you are doing and don't handle all your messages in the same sort of "mass handler". |
I'm coming into this as a relative outsider - having only really jumped into this component from reviewing some PRs for symfony/docs. I was surprised that there was no autoconfiguration from the interface and that it didn't end up using an API similar to EventSubscriberInterface with I'm not sure how that would work to continue support for the current option of just returning an array of classes as that's not something the EventSubscriberInterface supports. |
Thank you for your feedback. It's not the first time it has been asked so here is the pull-request: #27034. |
What are thoughts on a "supports" method where we could put instance of checks? Using @sroze's example above: class NotificationsManager implements ???
{
// ...
public function supportsMessage($message): bool
{
return $message instanceof TravelEvent;
}
public function __invoke(TravelEvent $event)
{
$travel = $this->travelRepository->find($event->getTravelUuid());
$events = $travel->apply($event);
foreach ($events as $raisedEvent) {
$this->eventStore->append(Stream::fromTravel($travel), $raisedEvent);
}
}
} This would not require the handler to be aware of all possible types of message. |
I'd suggest you open an issue, commenting on a closed issue is not the best
for the visibility of your RFC :)
…On Wed, Apr 25, 2018 at 7:31 PM Kevin Bond ***@***.***> wrote:
What are thoughts on a "supports" method where we could put instance of
checks? Using @sroze <https://github.com/sroze>'s example above:
class NotificationsManager implements ???{ // ... public function supportsMessage($message): bool { return $message instanceof TravelEvent; } public function __invoke(TravelEvent $event) { $travel = $this->travelRepository->find($event->getTravelUuid()); $events = $travel->apply($event); foreach ($events as $raisedEvent) { $this->eventStore->append(Stream::fromTravel($travel), $raisedEvent); } }}
This would not require the handler to be aware of all possible types of
message.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26685 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAxHEfUQ9Tc6E3AtQEfFovRnsG2iMcB0ks5tsLLWgaJpZM4S9TOY>
.
|
…(sroze) This PR was merged into the 4.1 branch. Discussion ---------- [Messenger][DX] Uses custom method names for handlers | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26685 (comment) | License | MIT | Doc PR | ø This has been discussed mostly in the [`MessageHandlerInterface` pull-request](#26685). For consistency reasons and convenience, this PR adds the ability to configure the method to be used on handlers: ```php use Symfony\Component\Messenger\Handler\MessageSubscriberInterface; use Symfony\Component\Messenger\Handler\MessageSubscriberConfiguration; class CreateNumberMessageHandler implements MessageSubscriberInterface { /** * {@inheritdoc} */ public static function getHandledMessages(): array { return [ CreateNumber::class => ['createNumber', 10], AnotherMessage::class => 'anotherMethod', ]; } public function createNumber(CreateNumber $command) { // ... } } ``` Commits ------- 2461e51 [Messenger][DX] Uses custom method names for handlers
…(sroze) This PR was merged into the 4.1 branch. Discussion ---------- [Messenger][DX] Uses custom method names for handlers | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#26685 (comment) | License | MIT | Doc PR | ø This has been discussed mostly in the [`MessageHandlerInterface` pull-request](symfony/symfony#26685). For consistency reasons and convenience, this PR adds the ability to configure the method to be used on handlers: ```php use Symfony\Component\Messenger\Handler\MessageSubscriberInterface; use Symfony\Component\Messenger\Handler\MessageSubscriberConfiguration; class CreateNumberMessageHandler implements MessageSubscriberInterface { /** * {@inheritdoc} */ public static function getHandledMessages(): array { return [ CreateNumber::class => ['createNumber', 10], AnotherMessage::class => 'anotherMethod', ]; } public function createNumber(CreateNumber $command) { // ... } } ``` Commits ------- 2461e5119a [Messenger][DX] Uses custom method names for handlers
…(sroze) This PR was merged into the 4.1 branch. Discussion ---------- [Messenger][DX] Uses custom method names for handlers | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#26685 (comment) | License | MIT | Doc PR | ø This has been discussed mostly in the [`MessageHandlerInterface` pull-request](symfony/symfony#26685). For consistency reasons and convenience, this PR adds the ability to configure the method to be used on handlers: ```php use Symfony\Component\Messenger\Handler\MessageSubscriberInterface; use Symfony\Component\Messenger\Handler\MessageSubscriberConfiguration; class CreateNumberMessageHandler implements MessageSubscriberInterface { /** * {@inheritdoc} */ public static function getHandledMessages(): array { return [ CreateNumber::class => ['createNumber', 10], AnotherMessage::class => 'anotherMethod', ]; } public function createNumber(CreateNumber $command) { // ... } } ``` Commits ------- 2461e5119a [Messenger][DX] Uses custom method names for handlers
Based on @chalasr's PR: #26672.
This reduces the hassle of registering handlers: it allows the auto-configuration with a new interface
HandlerInterface
. At the same time, it allows a handler to handle multiple messages.