-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[EventDispatcher] Validate existence of event listener classes #60277
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
[EventDispatcher] Validate existence of event listener classes #60277
Conversation
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
As per my understanding, failed tests are unrelated to my change. |
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 something that could potentially be useful for all the classes in the container, not just the event listener. As a last step before dumping the container, we can check the classes used for all services.
This was proposed 10 years ago, but declined: #11315 (comment). This decision may be reviewed.
src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/EventDispatcher/DependencyInjection/RegisterListenersPass.php
Outdated
Show resolved
Hide resolved
2b5b852
to
7a8e232
Compare
…ggest close matches during container compilation
7a8e232
to
282dc16
Compare
This comment has been minimized.
This comment has been minimized.
@@ -172,10 +172,13 @@ public function process(ContainerBuilder $container) | |||
|
|||
private function getEventFromTypeDeclaration(ContainerBuilder $container, string $id, string $method): string | |||
{ | |||
$class = $container->getDefinition($id)->getClass(); | |||
if (!$r = $container->getReflectionClass($class)) { | |||
throw new InvalidArgumentException(sprintf('Class "%s" used for service "%s" cannot be found.', $class, $id)); |
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.
An exception is already thrown by ContainerBuilder::getReflectionClass
:
symfony/src/Symfony/Component/DependencyInjection/ContainerBuilder.php
Lines 379 to 386 in fbff277
/** | |
* Retrieves the requested reflection class and registers it for resource tracking. | |
* | |
* @throws \ReflectionException when a parent class/interface/trait is not found and $throw is true | |
* | |
* @final | |
*/ | |
public function getReflectionClass(?string $class, bool $throw = true): ?\ReflectionClass |
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.
I could not reproduce this exception with the test in the PR. The ContainerBuilder::getReflectionClass
returns null
instead of throwing.
I changed the target branch as this is a behavior change and not a bug. |
👎 for the current implementation but looking at the linked issue, I'm wondering if we shouldn't add the check when the class is not defined for a service (and thus when the id becomes the class). Up for digging this idea @ashutoshagrawal1010? |
Closing meanwhile. Thanks for submitting. |
This PR improves the developer experience for event listener services.
kernel.event_listener
tag actually exists.InvalidArgumentException
instead of failing later at runtime.