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][DX] Uses custom method names for handlers #27034

Merged
merged 1 commit into from May 11, 2018

Conversation

@sroze
Member

sroze commented Apr 24, 2018

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. For consistency reasons and convenience, this PR adds the ability to configure the method to be used on handlers:

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)
    {
        // ...
    }
}
@ogizanagi

MessageSubscriberInterface::getHandledMessages docblock misses an update

/**
* Used to transform an object and a method to a callable handler.
*
* @author Samuel Roze <samuel.roze@gmail.com>

This comment has been minimized.

@ogizanagi

ogizanagi Apr 24, 2018

Member

@internal ?

This comment has been minimized.

@sroze

sroze Apr 24, 2018

Member

Good point, added 👍

$handlersByMessage = array();
foreach ($container->findTaggedServiceIds($this->handlerTag, true) as $serviceId => $tags) {
foreach ($tags as $tag) {
$handles = isset($tag['handles']) ? array($tag['handles']) : $this->guessHandledClasses($r = $container->getReflectionClass($container->getDefinition($serviceId)->getClass()), $serviceId);
if (isset($tag['handles'])) {
$handles = isset($tag['method']) ? array($tag['handles'] => $tag['method']) : array($tag['handles']);

This comment has been minimized.

@ogizanagi

ogizanagi Apr 24, 2018

Member

Should it check the method exists to warn about typos at compile time?

$priority = $tag['priority'] ?? 0;
foreach ($handles as $messageClass) {
foreach ($handles as $messageClass => $method) {
if (is_int($messageClass)) {

This comment has been minimized.

@chalasr

chalasr Apr 24, 2018

Member

let's add a \ in front of is_* calls to be consistent with the existing one below

public function __construct($object, string $method)
{
if (!is_object($object)) {
throw new \InvalidArgumentException(sprintf('Expected an object as argument but got %s', gettype($object)));

This comment has been minimized.

@chalasr

chalasr Apr 24, 2018

Member

\is_object, \gettype (no, I'm not a bot :))

@chalasr chalasr added this to the 4.1 milestone Apr 24, 2018

if ('__invoke' !== $method) {
$wrapperDefinition = (new Definition(MethodOnObjectHandler::class))->setArguments(array(new Reference($serviceId), $method));
$definitions[$serviceId = hash('sha1', $messageClass.':'.$messagePriority.':'.$serviceId)] = $wrapperDefinition;

This comment has been minimized.

@Tobion

Tobion Apr 24, 2018

Member

It looks like it does not work for

public static function getHandledMessages(): array
    {
        return [
            CreateNumber::class => 'method',
            CreateNumber::class => 'anotherMethod',
        ];
    }

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 25, 2018

Member

also, please use ContainerBuilder::hash instead of sha1

This comment has been minimized.

@stof

stof Apr 25, 2018

Member

@Tobion Your array does not work. It has a single item in it as PHP does not allow duplicate keys.

however, the hash should indeed include the method name, as you can have multiple methods.

and the final service id should not be only the hash. It should have a prefix before it to make it easier to identify services in error message (messenger.method_object_handler.<hash> could make sense)

This comment has been minimized.

@sroze

sroze Apr 25, 2018

Member

Good point, method should be in the hash. Updated 👍

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 27, 2018

From my experience the EventSubscriber return value to configure callbacks has a really bad DX. It's not clear what array structure can be returned without reading the doc. Repeating that in the MessageSubscriberInterface seems bad to me.

How about using a value object like

    public static function getHandledMessages(): array
    {
        return [
            new MessageCallback(CreateNumber::class, 'createNumber', 10),
            new MessageCallback(AnotherMessage::class, 'anotherMethod'),
        ];
    }
class MessageCallback
{
    public function __construct(string $messageClass, string $method = '__invoke', int $priority = 0)
    {
    }
}

This way the code is self-explaining and you don't have to deal with mixed returns like array of array or array of string or non-associative array.

@sroze

This comment has been minimized.

Member

sroze commented Apr 27, 2018

I like it!

@sroze

This comment has been minimized.

Member

sroze commented Apr 27, 2018

But the point of this PR is actually to make it consistent with the EventSubscriber to improve the developer experience 🙃

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 27, 2018

Learning from mistakes and improving them seems more important to me. Also the EventSubscriber with it's array structure is probably not gonna make it like this into the EventDispatcher PSR that got revived. But something like my propal is really flexible and clear. Just make MessageCallback an interface with getters and voila.
I would also suggest to make the return value iterable instead of array.
I could imagine an implementation that just configures all public methods of the subscriber as handlers with the typehints (similar to the __invoke autowiring but for several methods).

    public static function getHandledMessages(): iterable
    {
        return new PublicMethodsAsHandlers(self::class);
    }
@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 27, 2018

public static function getHandledMessages(): HandledMessagesDescription
return (new HandledMessagesDescription())->someFluentApi()...?

but this looks like a lot of code infrastructure, which means this works only with a compiled container, because when not compiled the overhead of building this configuration can become significant, especially compared to the array approach, isn't it?

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 27, 2018

I don't see where the overhead is when using a value object or a fluent api? This is just setters and getters.

@sroze

This comment has been minimized.

Member

sroze commented Apr 27, 2018

@nicolas-grekas I prefer the array approach (with the MessageCallback - or similar name - value object) than the fluent API as it's much simpler for everybody, to use, understand and maintain. There isn't anything complex to build here that requires a fluent API I believe.

I like the return new PublicMethodsAsHandlers(self::class); example also.

@stof

This comment has been minimized.

Member

stof commented Apr 27, 2018

@sroze the array of EventSubscriberInteface is very simple when you have to register a single listener. But when you need to register 2 listeners for the same event, I admit that I get it wrong half of the time (doing array('foo', 'bar') instead of array(array('foo'), array('bar')). I even went as far as writing a unit test taking all my subscribers and validating the structure being returned.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 27, 2018

Would it make sense to use the static method as a configurator?

static function configureHandledMessages(HandledMessagesDescriptor $config)
{
    $config->...
}

BUT on the other end, any objects we put here tightly couples the interface to the descriptor.
It could be a blocker when taking interoperability into account: by forcing an object wrapper, we forbid any custom extension of the return value. An array doesn't have this drawback.
(just thinking loud :) )

@Tobion

This comment has been minimized.

Member

Tobion commented Apr 27, 2018

One more idea that closes the circle in my eyes:

Remove https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Messenger/Handler/MessageHandlerInterface.php and instead offer a trait/abstract class that implements

public static function getHandledMessages(): iterable
{
    return [
        new ReflectedMethodCallback(self::class, '__invoke'),
    ];
}

This is self-explaining, does not require more code than implementing the marker interface and does not require a marker interface at all. A marker interface without methods is usually just a workaround anyway.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 27, 2018

I think this is building infrastructure on top of conventions. We should design first and implement after IMHO.
Another issue with using objects is that it will make it much harder for IDEs to use the return value to provide auto-completion.
On a DX perspective, I really much prefer injecting the descriptor than have a "new" in the body of the method. The reason is that "new" forces to know what type to create; while passing by argument is just perfect for providing autocompletion.

Disclaimer: the arguments I'm giving here are done while taking the existing static method descriptions we already have. @Tobion uses the word "mistake" above, I'm really not sure this was a mistake at all. And more importantly, I want to be sure whatever new system we use is actually better.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 29, 2018

As @Tobion spotted, using an injected argument enforces a single style of configuration (the DX is bound to the style provided byHandledMessagesDescriptor in my example above). While this can be nice for autocompletion, I don't think we can make a universal enough style (that could eg fit a FIG PSR - which is not a goal here, but still nice for guiding the thought).

I'm also not really convinced by returning objects, for similar reasons: a locally created object forces also a style, or if we make it an interface, the interface could only provide a way to retrieve the state of the configuration, without providing any contractual guarantee (unlike the injected argument btw).

For this reasons, I now think this method should return an array.
I also think we can provide a builder for this array if we want to, but this builder should be an (optional) implementation detail used in some implementations of getHandledMessages().

@kbond

This comment has been minimized.

Contributor

kbond commented Apr 29, 2018

For this reasons, I now think this method should return an array.

Why not iterable?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 29, 2018

Or iterable, just low-level data structure :)

@sroze

This comment has been minimized.

Member

sroze commented May 1, 2018

I've introduced a MessageSubscriberConfiguration object that can be used to specify the priority and/or method name. To reduce the complexity, I've removed the ability to have priority or method name from the tags.

And updated the PR description :)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented May 3, 2018

In #27034 (comment) I gave my reasoning explaining why I think returning object is not a good idea. Instead, I suggest to use a builder:
return (new FooBuilder())->add(...)->add()->toArray().
or
return [HandledMessageDescription::build(...), etc], with build() returning an array.

Basically, the getter on MessageSubscriberConfiguration should not be exposed as they have no meaning (they are just techinal requirements to acces the state) and thus they should not be part of the subscriber interfaces.

@sroze

This comment has been minimized.

Member

sroze commented May 3, 2018

@nicolas-grekas well, we can have both :)

@Tobion

This comment has been minimized.

Member

Tobion commented May 3, 2018

if we make it an interface, the interface could only provide a way to retrieve the state of the configuration, without providing any contractual guarantee

I don't understand what you mean? Could you clarify?
So you propose to return some random array structure again similar to EventSubscriber?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented May 3, 2018

I don't understand what you mean? Could you clarify?

if the static method in the interface describes a way to return VO, then the interface of the VO becomes part of the abstraction (the getters on the VO are bound to the interface).
At at the abstraction level, describing the handled messages does not require accessing any state - only setters are needed inside the static method. But since we need a technical means to access the state of the VO on the outside, we must add getters on it. And this is a leak in the abstraction.

The Config component does it correctly: it provides a way to describe a hierarchical structure using OOP, but doesn't leak in the resulting data structure.

My previous proposal (injecting a configurator) does not have this issue BTW. But instead of binding the getters to the abstraction, it binds the setters. A bit better, but not much as you spotted.

So you propose to return some random an array structure again similar to EventSubscriber?

yes - to the same extent any hierarchical formats like yaml and xml are "random".

@sroze we cannot have both, because that'd still be a leak at the abstraction level.

@Tobion

This comment has been minimized.

Member

Tobion commented May 3, 2018

But since we need a technical means to access the state of the VO on the outside, we must add getters on it. And this is a leak in the abstraction.

I don't see the difference between getters and an array with keys. Both leak information in the same way. I'm not sure what you are trying to achieve here.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented May 3, 2018

I'm seeking for a solution that improves the situation. And that could be generic, so that we could reuse it in other similar situations. With the reasoning I have given, I believe we didn't find any. I'm 👎.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented May 3, 2018

@sroze if you want to move forward here, I suggest to isolate things that are unrelated to the return format of the static method in another PR if that makes sense.

@sroze

This comment has been minimized.

Member

sroze commented May 3, 2018

@nicolas-grekas i.e. revert to the array-notation, right?

@sroze

This comment has been minimized.

Member

sroze commented May 4, 2018

Alright, that's fair that there are different point of views and it's a tricky question, therefore let's make that in two steps: I reverted the introduction of the VO and kept the array EventDispatcher-like syntax (while changing the getHandledMessages() signature to return a iterable rather than an array).

@sroze

This comment has been minimized.

Member

sroze commented May 5, 2018

Shall we go ahead with this one?

@ogizanagi

Yes, makes sense to me as is. Let's move forward 👍

@@ -23,6 +23,7 @@
use Symfony\Component\Messenger\Handler\MessageHandlerInterface;
use Symfony\Component\Messenger\Handler\MessageSubscriberInterface;
use Symfony\Component\Messenger\TraceableMessageBus;
use Symfony\Component\Messenger\Handler\MethodOnObjectHandler;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 5, 2018

Member

Alpha order

This comment has been minimized.

@sroze

sroze May 6, 2018

Member

updated 👍

if ('__invoke' !== $method) {
$wrapperDefinition = (new Definition(MethodOnObjectHandler::class))->setArguments(array(new Reference($serviceId), $method));
$definitions[$serviceId = 'messenger.method_on_object_handler.'.ContainerBuilder::hash($messageClass.':'.$messagePriority.':'.$serviceId.':'.$method)] = $wrapperDefinition;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 5, 2018

Member

Missing dot at start of service is.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 5, 2018

Member

But can't we pass array($def, $method) instead of using this handler?

This comment has been minimized.

@sroze

sroze May 6, 2018

Member

Not the way it's done now because we simply wire the handlers (i.e. callables). That way is also simpler :)

}
if ('__invoke' !== $method) {
$wrapperDefinition = (new Definition(MethodOnObjectHandler::class))->setArguments(array(new Reference($serviceId), $method));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 6, 2018

Member

$wrapperDefinition = (new Definition('callable'))->addArgument(array(new Reference($serviceId), $method))->setFactory('Closure::fromCallable');
cheers :)

This comment has been minimized.

@sroze

sroze May 7, 2018

Member

:dance:

$messageClass = $method;
$method = '__invoke';
}
if (\is_array($messageClass)) {

This comment has been minimized.

@fabpot

fabpot May 7, 2018

Member

could be else if here, right?

This comment has been minimized.

@sroze

sroze May 7, 2018

Member

Changed with elseif.

This comment has been minimized.

@sroze

sroze May 8, 2018

Member

@fabpot actually can't because we are setting $messagePriority as well.

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.1 May 7, 2018

@sroze

This comment has been minimized.

Member

sroze commented May 8, 2018

Status: Needs review

@sroze

This comment has been minimized.

Member

sroze commented May 9, 2018

@fabpot could you update your review please?

@fabpot

fabpot approved these changes May 11, 2018

@sroze sroze merged commit 2461e51 into symfony:4.1 May 11, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

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

@fabpot fabpot referenced this pull request May 21, 2018

Merged

Release v4.1.0-BETA2 #27331

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment