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

[EventSubscriber] Infer subscribed events from listener parameters #30801

Closed

Conversation

derrabus
Copy link
Member

@derrabus derrabus commented Mar 31, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT
Doc PR N/A

Since event names are FQCN now, I though maybe we could reduce the verbosity of event subscribers a bit by omitting the keys in the array returned by getSubscribedEvents(). This PR allows to rewrite this subscriber …

final class MySubscriber implements EventSubscriberInterface
{
    public static function getSubscribedEvents()
    {
        return [RequestEvent::class => 'onRequest'];
    }

    public function onRequest(RequestEvent $event): void
    {
    }
}

… like this …

final class MySubscriber implements EventSubscriberInterface
{
    public static function getSubscribedEvents()
    {
        return ['onRequest'];
    }

    public function onRequest(RequestEvent $event): void
    {
    }
}

Drawbacks:

  • The referenced method has to be defined on the subscriber. When using the event name as a key, the subscriber could handle the event within the magic __call() method.
  • This approach does not work on parameter-less listener methods for obvious reasons.
  • Reflection usage comes with a small performance penalty. When using the dispatcher standalone, the addSubscriber() call will be slightly slower on a subscriber without event names as keys. When using DI, only the container compilation will be slightly slower.

@nicolas-grekas
Copy link
Member

Are the drawbacks worth the explicitness downgrade?

@derrabus
Copy link
Member Author

I'd say yes. I like the reduced redundancy. The type-hint on the method parameter is explicit enough if event name === FQCN. Regarding the drawbacks, if I look at the event subscribers in my current applications:

  • I never use __call() to handle an event.
  • The method usually has a properly type-hinted parameter or no parameter at all.
  • I don't care much if container compilation takes a few µs more as long as the runtime performance is not impacted.

Also, I wouldn't deprecate or change the current layout of the array. You can even mix both styles. So, if you have a parameter-less listener method, you can still fall back to providing the event name as key.

@derrabus derrabus force-pushed the feature/implicit-event-subscribers branch from f014a0c to 359870e Compare March 31, 2019 22:01
*
* @return string
*/
protected function inferEventName($subscriber, $params): string
Copy link
Member

Choose a reason for hiding this comment

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

this should be in the pass, i.e. compile-time only

Copy link
Member

Choose a reason for hiding this comment

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

Hum, but this would mean getSubscribedEvents wouldn't by that decoupled anymore as the pass would be a requirement to allow bypassing the event name.
And on the other side, I don't like having the reflection-related logic in the main object.
I think I'm 👎 on this idea sorry.

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 should be in the pass, i.e. compile-time only

The logic is executed on compile-time only – in the full-stack framework scenario. If I moved the reflection logic into the pass, this feature would become a DI-only feature as it would not work anymore when using the dispatcher standalone.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly what I meant also.
We just closed #30760 to save the code from doing more Reflection, even at compile time.
It would be consistent to close this one also, sorry again.

Copy link
Member Author

Choose a reason for hiding this comment

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

All right.

@derrabus derrabus closed this Apr 3, 2019
@derrabus derrabus deleted the feature/implicit-event-subscribers branch April 3, 2019 15:45
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
nicolas-grekas added a commit that referenced this pull request Oct 9, 2019
…gistering listeners (derrabus)

This PR was merged into the 4.4 branch.

Discussion
----------

[EventDispatcher] Allow to omit the event name when registering listeners

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | #33453 (kind of)
| License       | MIT
| Doc PR        | TODO

After #30801 and #33485, this is another attempt at taking advantage of FQCN events for simplifying the registration of event listeners by inferring the event name from the parameter type declaration of the listener. This is my last attempt, I promise. 🙈

This time, I'd like to make the `event` attribute of the `kernel.event_listener` tag optional. This would allow us to build listeners like the following one without adding any attributes to the `kernel.event_listener` tag.

```php
namespace App\EventListener;

final class MyRequestListener
{
    public function __invoke(RequestEvent $event): void
    {
        // do something
    }
}
```

This in turn allows us to register a whole namespace of such listeners without having to configure each listener individually:

```YAML
services:
    App\EventListener\:
        resource: ../src/EventListener/*
        tags: [kernel.event_listener]
```

Commits
-------

6f32584 [EventDispatcher] Allow to omit the event name when registering listeners.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants