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

Merged
merged 1 commit into from Apr 3, 2018

Conversation

@sroze
Member

sroze commented Mar 27, 2018

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.

@weaverryan

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

This comment has been minimized.

@weaverryan

weaverryan Mar 27, 2018

Member

MessageHandlerInterface? It's just a bit more obvious when you see the interface by itself in code (e.g. FooHandler implements MessageHandlerInterface

This comment has been minimized.

@sroze

sroze Mar 27, 2018

Member

It would match the tag name, it makes sense.

* return [
* FirstMessage::class,
* SecondMessage:class
* ];

This comment has been minimized.

@weaverryan

weaverryan Mar 27, 2018

Member

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.

This comment has been minimized.

@weaverryan

weaverryan Mar 27, 2018

Member

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());

This comment has been minimized.

@weaverryan

weaverryan Mar 27, 2018

Member

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

This comment has been minimized.

@sroze

sroze Mar 27, 2018

Member

I think it's a good idea.

@sroze

This comment has been minimized.

Member

sroze commented Mar 27, 2018

Thanks @weaverryan. Updated based on your feedbacks 👍

@Nyholm

Great. I did a quick review. Just have one comment.

*
* return [
* [FirstMessage::class, 0],
* [SecondMessage::class, -10],

This comment has been minimized.

@Nyholm

Nyholm Mar 27, 2018

Member

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],
];

This comment has been minimized.

@sroze

sroze Mar 27, 2018

Member

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

@mvrhov

This comment has been minimized.

Contributor

mvrhov commented Mar 28, 2018

Do we really need this functionality. SRP anyone?

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Mar 28, 2018

@mvrhov : I'm not found of it either, but this can be useful for event buses. -> I was more thinking about the priority, not really handling multiple messages types in one handler. Sorry.

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 getHandledMessages would basically be useless), like I'm already used to with tactician:

    command_handlers:
        namespace: App\Application\
        resource: '%kernel.project_dir%/src/Application/**/Handler/*CommandHandler.php'
        tags:
            - { name: tactician.handler, typehints: true, bus: command }
@sroze

This comment has been minimized.

Member

sroze commented Mar 28, 2018

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

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Mar 28, 2018

Of course! 😄
Just saying I find this interface too much constraining if you want to use it for autoconfiguration without psr-4 wiring, as it requires implementing a method that wouldn't be useful for most buses (and even for event buses, handling multiple event types in the same handler does not looks so nice to me. I was more referring to the priority in my previous comment actually).
Hence, I find the intent here not very clear.

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

This comment has been minimized.

@javiereguiluz

javiereguiluz Mar 28, 2018

Member

Are there two different but similar interfaces or is this a typo? HandlerInterface and MessageHandlerInterface

This comment has been minimized.

@sroze

sroze Mar 29, 2018

Member

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

This comment has been minimized.

@javiereguiluz

javiereguiluz Mar 28, 2018

Member

implements -> implement

@javiereguiluz javiereguiluz added this to the 4.1 milestone Mar 28, 2018

@weaverryan

This comment has been minimized.

Member

weaverryan commented Mar 28, 2018

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 services.yaml, unless we add it there always in the FrameworkBundle recipe... which is kinda weird since the config isn't relevant unless you install the component.

@mvrhov

This comment has been minimized.

Contributor

mvrhov commented Mar 28, 2018

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.

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Mar 29, 2018

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.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Mar 29, 2018

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 UploadedCoverImage and UploadedProfileImage should both be processed by the CleanMetadataHandler. Sure the “correct” way is to create a service and is used by CleanProfileMetadataHandler and CleanCoverMetadataHandler. But you know... using the same handler is more convenient and simple if the alternative is to create two empty handlers.

@sroze

This comment has been minimized.

Member

sroze commented Mar 29, 2018

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

@sroze sroze changed the title from [Messenger] Add handlers auto-configuration to [Messenger] Add a `MessageHandlerInterface` (multiple messages + auto-configuration) Mar 29, 2018

@Tobion

This comment has been minimized.

Member

Tobion commented Mar 31, 2018

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.
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 multiple messages support, we can still introduce a new interface that extends from the marker interface later.

@sroze

This comment has been minimized.

Member

sroze commented Apr 1, 2018

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 1, 2018

I know but as I pointed out the additional thing is worse. So no point in adding something that is inferior.

@chalasr

This comment has been minimized.

Member

chalasr commented Apr 1, 2018

I agree with @Tobion and @ogizanagi, 👍 for a marker only.

@Nyholm

This comment has been minimized.

Member

Nyholm commented Apr 1, 2018

Reading all the feedback on this PR, I suggest two interfaces where the interface in this PR extends a “marker only” interface.

(Of course the interfaces should be named differently but that is a later discussion)

@nicolas-grekas

having two interfaces look better to me also: MessageHandlerInterface to add its type to any callable class that is intended to be used as a message handler
and MessageSubscriberInterface for handlers that deal with multiple types.

$messageClassLocation = isset($tag['handles']) ? 'declared in your tag attribute "handles"' : sprintf('used as argument type in method "%s::__invoke()"', $r->getName());
throw new RuntimeException(sprintf('Invalid handler service "%s": message class "%s" %s does not exist.', $serviceId, $handles, $messageClassLocation));
$handles = $tag['handles'] ?? $this->guessHandledClasses($r = $container->getReflectionClass($container->getParameterBag()->resolveValue($container->getDefinition($serviceId)->getClass())), $serviceId);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 2, 2018

Member

resolveValue is already handled inside getReflectionClass

throw new RuntimeException(sprintf('Invalid handler service "%s": message class "%s" %s does not exist.', $serviceId, $handles, $messageClassLocation));
$handles = $tag['handles'] ?? $this->guessHandledClasses($r = $container->getReflectionClass($container->getParameterBag()->resolveValue($container->getDefinition($serviceId)->getClass())), $serviceId);
if (empty($handles)) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 2, 2018

Member

no need for empty()
if (!$handles) {

$messagePriority = $priority;
}
if (!class_exists($messageClass)) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 2, 2018

Member

do we really need this check?
we usually don't validate DI configurations at compile time, because this adds overhead to the compilation process

This comment has been minimized.

@sroze

sroze Apr 3, 2018

Member

I believe it would definitely helps the DX if there is a typo in the class name or missing use.

namespace Symfony\Component\Messenger\Handler;
/**
* Handlers can implement this interface. This allows them to be auto-configured and to

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 2, 2018

Member

I would remove the mention of autoconfiguration.
The interface is useful on its own to add semantic to an invokable class.
THEN, yes, autoconfiguration can leverage this added semantic. But that's independent.

This comment has been minimized.

@sroze

sroze Apr 3, 2018

Member

Changed 👍

}
}
class PrioritisedHandler implements MessageHandlerInterface

This comment has been minimized.

@xabbuh

xabbuh Apr 2, 2018

Member

PrioritizedHandler

This comment has been minimized.

@sroze

sroze Apr 3, 2018

Member

Changed 👍

$handlerReference = (string) $handlerMapping['handler.'.SecondMessage::class]->getValues()[0];
$definition = $container->getDefinition($handlerReference);
$this->assertEquals(ChainHandler::class, $definition->getClass());

This comment has been minimized.

@xabbuh

xabbuh Apr 2, 2018

Member

assertSame()

@sroze

This comment has been minimized.

Member

sroze commented Apr 3, 2018

Fair enough, I've split it into two interfaces 🙃

namespace Symfony\Component\Messenger\Handler;
/**
* Handlers can implement this interface.

This comment has been minimized.

@weaverryan

weaverryan Apr 3, 2018

Member

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.

This comment has been minimized.

@sroze

sroze Apr 3, 2018

Member

@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 👍)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 3, 2018

Member

I commented that we should not tell about "autoconfiguration", which isn't the incompatible with Ryan's comment :)

This comment has been minimized.

@fabpot

fabpot Apr 4, 2018

Member

@sroze Can you add some more information here then?

This comment has been minimized.

@sroze

@sroze sroze merged commit 07e6bc7 into symfony:master Apr 3, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

sroze added a commit that referenced this pull request Apr 3, 2018

feature #26685 [Messenger] Add a `MessageHandlerInterface` (multiple …
…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)

@sroze sroze deleted the sroze:messenger-handlers-autoconfiguration branch Apr 3, 2018

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 9, 2018

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?

@weaverryan

This comment has been minimized.

Member

weaverryan commented Apr 9, 2018

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

@sroze

This comment has been minimized.

Member

sroze commented Apr 9, 2018

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.

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 9, 2018

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.

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 9, 2018

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.
Currently it just adds an inferior solution for a bad architectural design where you need to expose public methods without typing the arguments.

@sroze

This comment has been minimized.

Member

sroze commented Apr 9, 2018

It is that there is a better solution already so doesn't need to be solved at the framework level at all.

I don't get your point. For this use-case, what would be "the better solution" then?

Also I don't see why the interface does not allow to specify the callback methods for the messages like EventSubscriberInterface

I don't believe this is actually required but I'd 👍 such a PR tbh.

Currently it just adds an inferior solution for a bad architectural design where you need to expose public methods without typing the arguments.

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);
        }
    }
}
@Tobion

This comment has been minimized.

Member

Tobion commented Apr 9, 2018

In your case the handler processes messages that share a common parent (or interface), e.g. TravelEvent. I would suggest the messages should be routed to the handler automatically based on the type in __invoke. So getHandledMessages should not be necessary at all. So I think routing with instanceof should be supported in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Messenger/HandlerLocator.php or a different implementation of HandlerLocatorInterface

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
    }
}
@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 9, 2018

I would suggest the messages should be routed to the handler automatically based on the type in __invoke. So getHandledMessages should not be necessary at all. So I think routing with instanceof should be supported

or just explicit config 🙄

+1 for the trait example.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 9, 2018

allow specifying the callback methods for the messages like EventSubscriberInterface

I think this is actually a very good idea: the current design of MessageSubscriberInterface forces using __invoke while allowing several methods would encourage proper type-hinting (instead of having to drop type hints entirely when using __invoke.)

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Apr 9, 2018

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.

@sroze

This comment has been minimized.

Member

sroze commented Apr 9, 2018

allow specifying the callback methods for the messages like EventSubscriberInterface

Yes. Just the consistency argument is IMHO enough to add this.

So I think routing with instanceof should be supported

Agree with you @Tobion.

The other case is that different messages that don't share a parent or interface are handled in the same handler

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

@tomtomau

This comment has been minimized.

tomtomau commented Apr 24, 2018

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 getSubscribedEvents() returning something like: array('eventName' => array(array('methodName1', $priority), array('methodName2')). I would 👍 towards something like this for consistency!

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.

@sroze

This comment has been minimized.

Member

sroze commented Apr 24, 2018

Thank you for your feedback. It's not the first time it has been asked so here is the pull-request: #27034.

@kbond

This comment has been minimized.

Contributor

kbond commented Apr 25, 2018

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.

@sroze

This comment has been minimized.

Member

sroze commented Apr 25, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

sroze added a commit that referenced this pull request May 11, 2018

feature #27034 [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 | #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

symfony-splitter pushed a commit to symfony/messenger that referenced this pull request May 11, 2018

feature #27034 [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment