Skip to content

Conversation

@HypeMC
Copy link
Member

@HypeMC HypeMC commented Oct 28, 2025

Q A
Branch? 7.4
Bug fix? yes
New feature? no
Deprecations? no
Issues -
License MIT

This is a reimplementation of #61252.
The previous PR introduced a backward compatibility break.

Consider the following listener:

final class TestListener
{
    #[AsEventListener(event: RequestEvent::class)]
    public function onRequestEvent(): void
    {
        // ...
    }
}

In earlier versions, this worked fine, but now it throws:

AsEventListener attribute requires the first argument of "App\EventListener\TestListener::onRequestEvent()" to be an event object.

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 in RegisterListenersPass::getEventFromTypeDeclaration(), resulting in a decentralized implementation.

This PR reverts the changes in FrameworkExtension and re-implements the feature in RegisterListenersPass.
The tests now reuse the closure from FrameworkExtension to make them more robust and consistent with the actual implementation.

Comment on lines +782 to +788
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);
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 just reverts the changes made in #61252.

/**
* @return string[]
*/
private function getEventFromTypeDeclaration(ContainerBuilder $container, string $id, string $method): array
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 is the actual new implementation.

Comment on lines -306 to -314
$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);
}
);
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 is the reason the tests passed when they shouldn't have.

"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",
Copy link
Member

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

Copy link
Member Author

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.

@nicolas-grekas
Copy link
Member

Thank you @HypeMC.

@nicolas-grekas nicolas-grekas merged commit 5c76029 into symfony:7.4 Oct 29, 2025
11 of 12 checks passed
@HypeMC HypeMC deleted the fix-union-events branch October 29, 2025 14:15
This was referenced Nov 2, 2025
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.

4 participants