[EventSubscriber] Infer subscribed events from listener parameters #30801
Conversation
bf864fa to
f014a0c
Compare
|
Are the drawbacks worth the explicitness downgrade? |
|
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:
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. |
f014a0c to
359870e
Compare
| * | ||
| * @return string | ||
| */ | ||
| protected function inferEventName($subscriber, $params): string |
There was a problem hiding this comment.
this should be in the pass, i.e. compile-time only
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…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.
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 …… like this …
Drawbacks:
__call()method.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.