[EventDispatcher] Add events attribute of the kernel.event_listener tag
#61222
Conversation
santysisi
force-pushed
the
feature/kernel-event_listener-events-attribute
branch
2 times, most recently
from
6513db5 to
f801995
Compare
santysisi
force-pushed
the
feature/kernel-event_listener-events-attribute
branch
from
f801995 to
61e8043
Compare
| "php": ">=8.2", | ||
| "symfony/event-dispatcher-contracts": "^2.5|^3" | ||
| "symfony/event-dispatcher-contracts": "^2.5|^3", | ||
| "symfony/deprecation-contracts": "^2.5|^3" |
There was a problem hiding this comment.
I added this dependency to the component because I'm using the trigger_deprecation() function.
| "symfony/deprecation-contracts": "^2.5|^3", | ||
| "symfony/error-handler": "^7.3|^8.0", | ||
| "symfony/event-dispatcher": "^6.4|^7.0|^8.0", | ||
| "symfony/event-dispatcher": "^7.4|^8.0", |
There was a problem hiding this comment.
I'm bumping the version to 7.4 in order to use the new events attribute instead of event.
Please let me know if you'd prefer a different strategy or approach. I'm happy to adjust and avoid the version bump if needed 😄
| "symfony/dependency-injection": "^6.4.11|^7.1.4|^8.0", | ||
| "symfony/deprecation-contracts": "^2.5|^3", | ||
| "symfony/event-dispatcher": "^6.4|^7.0|^8.0", | ||
| "symfony/event-dispatcher": "^7.4|^8.0", |
|
Deprecating and renaming the "event" parameter of the tag has too much impact (as shown by the diff in tests). That will cause a lot of maintenance burden for projects that define event listeners in configuration directly. Instead, I think the event listener tag can be repeated for each listened event. |
|
I'm not sure that's the right approach. Can't we add multiple kernel.event_listener tags instead? That should be already supported. |
|
|
||
| EventDispatcher | ||
| --------------- | ||
| * Deprecate attribute `event` of the `kernel.event_listener` tag in favor of `events` attribute |
There was a problem hiding this comment.
| * Deprecate attribute `event` of the `kernel.event_listener` tag in favor of `events` attribute | |
| * Deprecate attribute `event` of the `kernel.event_listener` tag in favor of `events` attribute |
There was a problem hiding this comment.
There are also two spaces between "tag" and "in".
There was a problem hiding this comment.
This deprecation must be reverted.
|
|
||
| 7.4 | ||
| --- | ||
| * Add `events` attribute of the `kernel.event_listener` tag |
There was a problem hiding this comment.
| * Add `events` attribute of the `kernel.event_listener` tag | |
| * Add `events` attribute of the `kernel.event_listener` tag |
|
We should keep tests using also the old |
alex-dev
left a comment
alex-dev
left a comment
There was a problem hiding this comment.
Looking at how much changes this implementation for a minor QoL improvement bring, it is clear we should consider a less invasive approach.
I see no reason to go with supporting multiple events in the same tag. Considering a service can already have multiple kernel.event_listener targetting the same method, keeping the nesting flat and having a single event per tag ensure compatibility with everything without any breaking change.
It even matches better with the final runtime implementation: the listener method is registered multiple time in the dispatcher.
|
Hi everyone 👋, Thank you all for taking the time to review this PR and explain why this approach might not be ideal. I understand and agree with your points, so I’ll go ahead and close this PR. Once again, I really appreciate you all for sharing your perspectives ❤️ |
…EventListener]` (Fan2Shrek) This PR was merged into the 7.4 branch. Discussion ---------- [FrameworkBundle] Add support for union types on `#[AsEventListener]` | Q | A | ------------- | --- | Branch? | 7.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | Fix #61218 | License | MIT This is an alternative implementation to support union types, related to issue #61218 and PR #61222. It duplicates `kernel.event_listener` tags. I'm not sure how to add tests for this — could you advise? Commits ------- 5d7b14f Add support for union types on AsEventListener
7 participants
Added
eventsattribute in thekernel.event_listenertag, enabling registration of a listener for multiple events at once.Deprecated
eventattribute inkernel.event_listeneris now deprecated and will be removed in Symfony 8.0.eventattribute will trigger a deprecation warning.eventandeventswill throw anInvalidArgumentExceptionto avoid configuration conflicts.