-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[EventDispatcher] Removed "callable" type hint from WrappedListener constructor #31493
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
Wouldn't it make more sense to let the compiler pass throw a meaningful error when such a listener is registered? |
6878bf7
to
067a900
Compare
@xabbuh Yes, you're right. After a bit of thinking, I realized that my first solution was just a hotfix, but you suggested a much better approach, which not only solves the problem, but also does not force the removal of the callable type hint, which should remain. |
067a900
to
1093078
Compare
We prefer avoiding such compile time validation, because it slows down the compilation, thus DX. |
We ran into this issue as well. In our case, the fix was to delete the corresponding However, what I found problematic was that this issue broke the profiler for us and it was not really obvious why. The bar just displayed "An error occurred while loading the web debug toolbar." and no profile was logged for the corresponding request. The only evidence was an entry in the server's error log.
I had to add breakpoints to Until Symfony 4.2 it was apparently possible to register an event listener that is not (yet) callable. So the question is: Do we want to keep this "feature"? If we do, removing the type-hint would be the correct solution. If not, we should fail nicer than we currently do. Either way, adding compile-time checks would not solve the issue for us since the broken listeners were added during runtime in our case. |
Let's go back to the initial patch, isn't it? |
@nicolas-grekas: You did this in #29245 because you wanted to use |
OK thanks - If the error message that is thrown when removing the type hint is more informative, we should definitely go with it and remove the type hint. |
1093078
to
20c587f
Compare
I've updated the patch. |
Thank you @wskorodecki. |
…dListener constructor (wskorodecki) This PR was merged into the 4.3 branch. Discussion ---------- [EventDispatcher] Removed "callable" type hint from WrappedListener constructor | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT ### The problem ```php public function __construct(callable $listener, ?string $name, Stopwatch $stopwatch, EventDispatcherInterface $dispatcher = null) ``` The *callable* type hint will cause the following exception if you register a listener class with a method, which could not be auto-guessed by the Symfony: `Argument 1 passed to Symfony\Component\EventDispatcher\Debug\WrappedListener::__construct() must be callable, array given` The debug toolbar will not be displayed in this case. This is because PHP is checking the array first before making a decision if this is a valid callable or not. So if the second array element does not represent an existing method in object from the first element - PHP will treat this as a common array, not callable. Use `is_callable` method if you need a proof. ### How to reproduce 1. Register some listener with a method, which could not be auto-guessed by Symfony 2. Do not create the `__invoke` method in the listener 3. Skip the `method` attribute in the listener configuration Example: ```php class SomeListener { public function myListenerMethod(SomeEvent $event) { // ... } } ``` ```yaml App\EventListener\SomeListener: tags: - name: kernel.event_listener # Symfony will look for "onSomeEvent" method which does not exists. event: 'some.event' #method: 'myListenerMethod' # Skip this. ``` ### Solution: Removing the type hint will cause the \Closure::fromCallable() method to throw a proper exception, for example: `Symfony\Component\Debug\Exception\FatalThrowableError(code: 0): Failed to create closure from callable: class 'App\EventListener\SomeListener' does not have a method 'onSomeEvent'` Commits ------- 20c587f [EventDispatcher] Removed "callable" type hint from WrappedListener constructor
…rrabus) This PR was merged into the 4.3 branch. Discussion ---------- Allow WrappedListener to describe uncallable listeners | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #31493 | License | MIT | Doc PR | N/A This is a follow-up to #31493. The previous PR did not fix the problem completely. We also need to make sure that a listener that is not callable is not passed to `Closure::fromCallable()`. Note: It would probably be a good idea to give the developer a hint about uncallable listeners. Currently, such a listener causes an exception at the worst possible point of time: when collecting the profile information. This breaks the toolbar without any helpful feedback, as I've described [here](#31493 (comment)). This PR allows the `WrappedListener` class to describe a listener for the profiler even if the listener is not callable, which was the behavior in Symfony 4.2. Commits ------- bc3f598 Allow WrappedListener to describe uncallable listeners.
The problem
The callable type hint will cause the following exception if you register a listener class with a method, which could not be auto-guessed by the Symfony:
Argument 1 passed to Symfony\Component\EventDispatcher\Debug\WrappedListener::__construct() must be callable, array given
The debug toolbar will not be displayed in this case.
This is because PHP is checking the array first before making a decision if this is a valid callable or not. So if the second array element does not represent an existing method in object from the first element - PHP will treat this as a common array, not callable. Use
is_callable
method if you need a proof.How to reproduce
__invoke
method in the listenermethod
attribute in the listener configurationExample:
Solution:
Removing the type hint will cause the \Closure::fromCallable() method to throw a proper exception, for example:
Symfony\Component\Debug\Exception\FatalThrowableError(code: 0): Failed to create closure from callable: class 'App\EventListener\SomeListener' does not have a method 'onSomeEvent'