-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[EventDispatcher][FrameworkBundle] Rework union types on #[AsEventListener]
#62184
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
Conversation
| if ($reflector instanceof \ReflectionMethod) { | ||
| if (isset($tagAttributes['method'])) { | ||
| throw new LogicException(\sprintf('AsEventListener attribute cannot declare a method on "%s::%s()".', $reflector->class, $reflector->name)); | ||
| } | ||
| $tagAttributes['method'] = $reflector->getName(); | ||
| } | ||
| $definition->addTag('kernel.event_listener', $tagAttributes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just reverts the changes made in #61252.
| /** | ||
| * @return string[] | ||
| */ | ||
| private function getEventFromTypeDeclaration(ContainerBuilder $container, string $id, string $method): array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the actual new implementation.
| $container->registerAttributeForAutoconfiguration(AsEventListener::class, | ||
| static function (ChildDefinition $definition, AsEventListener $attribute, \ReflectionClass|\ReflectionMethod $reflector): void { | ||
| $tagAttributes = get_object_vars($attribute); | ||
| if ($reflector instanceof \ReflectionMethod) { | ||
| $tagAttributes['method'] = $reflector->getName(); | ||
| } | ||
| $definition->addTag('kernel.event_listener', $tagAttributes); | ||
| } | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the reason the tests passed when they shouldn't have.
src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php
Outdated
Show resolved
Hide resolved
| "symfony/expression-language": "^6.4|^7.0|^8.0", | ||
| "symfony/config": "^6.4|^7.0|^8.0", | ||
| "symfony/error-handler": "^6.4|^7.0|^8.0", | ||
| "symfony/framework-bundle": "^6.4|^7.0|^8.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we do without this require? it feels reverse to have a component require a bundle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I felt the same way, but the reason the previous PR didn't break the existing tests was that it didn't update the registerAttributeForAutoconfiguration() closure in the tests. I'm not sure how to ensure this doesn't happen again.
73479c4 to
8ea7196
Compare
|
Thank you @HypeMC. |
This is a reimplementation of #61252.
The previous PR introduced a backward compatibility break.
Consider the following listener:
In earlier versions, this worked fine, but now it throws:
Interestingly, there was a test for this scenario, but since each test method re-defines the
registerAttributeForAutoconfiguration()closure (which wasn't updated everywhere), the tests still passed.Additionally, the implementation was added to the
FrameworkExtension, even though similar logic already existed inRegisterListenersPass::getEventFromTypeDeclaration(), resulting in a decentralized implementation.This PR reverts the changes in
FrameworkExtensionand re-implements the feature inRegisterListenersPass.The tests now reuse the closure from
FrameworkExtensionto make them more robust and consistent with the actual implementation.