Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedMay 13, 2019
Wouldn't it make more sense to let the compiler pass throw a meaningful error when such a listener is registered? |
6878bf7 to067a900Comparewskorodecki commentedMay 14, 2019
@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 thecallable type hint, which should remain. |
067a900 to1093078Comparenicolas-grekas commentedMay 14, 2019
We prefer avoiding such compile time validation, because it slows down the compilation, thus DX. |
derrabus commentedMay 16, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
nicolas-grekas commentedMay 16, 2019
Let's go back to the initial patch, isn't it? |
derrabus commentedMay 16, 2019
@nicolas-grekas: You did this in#29245 because you wanted to use |
nicolas-grekas commentedMay 16, 2019
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 to20c587fComparewskorodecki commentedMay 18, 2019
I've updated the patch. |
fabpot commentedMay 18, 2019
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```phppublic 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 reproduce1. Register some listener with a method, which could not be auto-guessed by Symfony2. Do not create the `__invoke` method in the listener3. Skip the `method` attribute in the listener configurationExample:```phpclass SomeListener{ public function myListenerMethod(SomeEvent $event) { // ... }}``````yamlApp\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/AThis 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
Thecallable 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 givenThe 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_callablemethod if you need a proof.How to reproduce
__invokemethod in the listenermethodattribute 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'