Skip to content
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

[EventDispatcher] Add InvokableEventSubscriberTrait helper trait for single-event subscription #33485

Open
wants to merge 3 commits into
base: 4.4
from

Conversation

@yceruto
Copy link
Member

commented Sep 6, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #33453
License MIT
Doc PR TODO

Everything is in the linked issue.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

I'm sorry but I don't think this is an idea we should pursue.
It forces tight coupling to the component by inheritance and relies on pure conventions that happen to be what I call dead-ends: as soon as you want to listen to many events, you need to unlearn what you had to learn to use this class, you then need to learn about subscribers, and best of all, you are forced to change the inheritance chain - thus break BC.

[edit: this comment is now obsolete]

@derrabus
Copy link
Contributor

left a comment

Good idea. I would use that class instantly. 👍

Tests for the error cases are missing:

  • Missing event parameter
  • Missing/invalid type declaration
  • Missing __invoke()
@derrabus

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

It forces tight coupling to the component by inheritance and relies on pure conventions that happen to be what I call dead-ends: as soon as you want to listen to many events, you need to unlearn what you had to learn to use this class, you then need to learn about subscribers, and best of all, you are forced to change the inheritance chain - thus break BC.

For the same reasons, we could ban the abstract Voter class from the security component for instance. Yet, classes like these remove the need for boilerplate that looks the same in 80% of the cases.

Also, developers can look into this class and learn how they can build subscribers without it. The reflection part aside, it's pretty straightforward.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Voter doesn't use reflection. It's just a base abstract class.
The proposed one provides conventions by reflection.

All this is exactly the same as writing:

    final public static function getSubscribedEvents(): array
    {
        return [MyEvent::class => ['__invoke', 0]];
    }

Saving writing this is not worth the drawbacks to me.
If you want to save writing this, a maker would do it without the drawbacks.

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

What about a new interface for single-event subscription?

interface EventListenerInterface
{
    public static function getEventName();
    public static function getMethodName();
    public static function getPriority();
}

and with auto-configuration would make the current event listener approach even easier maybe?

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

Related to #33485 (comment), the getMethodName could also be useless if the event listeners already support invokable classes.

@yceruto yceruto force-pushed the yceruto:event_listener branch 2 times, most recently from c20950c to d347385 Sep 7, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

Talking with @nicolas-grekas on Slack, their architectural objections can be removed using traits, done!, so let's focus on the teachability objections now :)

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

I will add more tests when the objections have been removed, I hope that :)

@yceruto yceruto force-pushed the yceruto:event_listener branch 2 times, most recently from 59ade0a to badd322 Sep 7, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

If you're ok with my last review, we could name the trait InvokableEventSubscriberTrait

@yceruto yceruto force-pushed the yceruto:event_listener branch 4 times, most recently from 1a83a39 to 6031c96 Sep 7, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

@nicolas-grekas comments addressed where possible, how does it look to you now?

@yceruto yceruto force-pushed the yceruto:event_listener branch from 6031c96 to a0c0feb Sep 7, 2019

@yceruto yceruto force-pushed the yceruto:event_listener branch 2 times, most recently from d17bfa8 to 4875a6d Sep 7, 2019

@yceruto yceruto force-pushed the yceruto:event_listener branch from 4875a6d to 3e72b32 Sep 9, 2019

@yceruto yceruto changed the title [EventDispatcher] Add helper class for single-event subscription [EventDispatcher] Add InvokableEventSubscriberTrait helper trait for single-event subscription Sep 9, 2019

@yceruto yceruto force-pushed the yceruto:event_listener branch 2 times, most recently from a020d1f to 3936532 Sep 9, 2019

@yceruto

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

Update

Removed getEventName and getPriority methods, so you'll use this trait for one exclusive purpose, the most common on app side:

class MyEventSubscriber implements EventSubscriberInterface
{
    use InvokableEventSubscriberTrait;

    public function __invoke(MyEvent $event): void
    {
        // ...
    }
}

If it does not fit your needs, because the event name is not the FQCN or it has a different priority, then you should not use this feature and would implement the getSubscribedEvents() method directly.

@yceruto yceruto force-pushed the yceruto:event_listener branch from 3936532 to 39f2088 Sep 9, 2019

@yceruto yceruto force-pushed the yceruto:event_listener branch from 39f2088 to 401e7e4 Sep 9, 2019

@yceruto yceruto force-pushed the yceruto:event_listener branch from 401e7e4 to 61af258 Sep 9, 2019

@yceruto yceruto force-pushed the yceruto:event_listener branch from b25aa93 to c3e9e91 Sep 9, 2019

@nicolas-grekas
Copy link
Member

left a comment

(with a request to improve the description in the docblock)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.