Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Merged
fabpot merged 1 commit intosymfony:4.3fromwskorodecki:bugfix/wrapped-listener
May 18, 2019

Conversation

@wskorodecki
Copy link
Contributor

QA
Branch?4.3
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
LicenseMIT

The problem

publicfunction __construct(callable$listener, ?string$name,Stopwatch$stopwatch,EventDispatcherInterface$dispatcher =null)

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 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. Useis_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 themethod attribute in the listener configuration

Example:

class SomeListener{publicfunctionmyListenerMethod(SomeEvent$event)    {// ...    }}
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'

@xabbuh
Copy link
Member

Wouldn't it make more sense to let the compiler pass throw a meaningful error when such a listener is registered?

@wskorodeckiwskorodeckiforce-pushed thebugfix/wrapped-listener branch 6 times, most recently from6878bf7 to067a900CompareMay 14, 2019 07:36
@wskorodecki
Copy link
ContributorAuthor

@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.
I've updated theRegisterListenersPass andRegisterListenersPassTest.

@wskorodeckiwskorodeckiforce-pushed thebugfix/wrapped-listener branch from067a900 to1093078CompareMay 14, 2019 08:48
@nicolas-grekas
Copy link
Member

We prefer avoiding such compile time validation, because it slows down the compilation, thus DX.
I know we added more of it in recent months but I'm not sure it's for the best.
Let's think twice here please.

@derrabus
Copy link
Member

derrabus commentedMay 16, 2019
edited
Loading

We ran into this issue as well. In our case, the fix was to delete the correspondingaddListener() calls because the arrays that were passed referenced classes that have been deleted a while ago. This of course was a mistake on our side that was uncovered during the Symfony upgrade.

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.

PHP Fatal error:  Uncaught Symfony\Component\Debug\Exception\FatalThrowableError: Argument 1 passed to Symfony\Component\EventDispatcher\Debug\WrappedListener::__construct() must be callable, array given, called in /path/to/project/vendor/symfony/event-dispatcher/Debug/TraceableEventDispatcher.php on line 244 in /path/to/project/vendor/symfony/event-dispatcher/Debug/WrappedListener.php:41Stack trace: #0 /path/to/project/vendor/symfony/event-dispatcher/Debug/TraceableEventDispatcher.php(244): Symfony\Component\EventDispatcher\Debug\WrappedListener->__construct(Array, NULL, Object(Symfony\Component\Stopwatch\Stopwatch), Object(Symfony\Component\HttpKernel\Debug\TraceableEventDispatcher)) #1 /path/to/project/vendor/symfony/http-kernel/DataCollector/EventDataCollector.php(65): Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher->getNotCalledListeners(Object(Symfony\Component\HttpFoundation\Request)) #2 /path/to/project/vendor/symfony/http-kernel/Profile in /path/to/project/vendor/symfony/event-dispatcher/Debug/WrappedListener.php on line 41

I had to add breakpoints toWrappedListener to find the broken listener registrations. Since forgetting to remove a listener registration is a mistake that might happen from time to time, we should make it easier to debug this.

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

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?
Did anyone check why we added the type btw?

@derrabus
Copy link
Member

Did anyone check why we added the type btw?

@nicolas-grekas: You did this in#29245 because you wanted to useClosure::fromCallable().

@nicolas-grekas
Copy link
Member

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.

@wskorodeckiwskorodeckiforce-pushed thebugfix/wrapped-listener branch from1093078 to20c587fCompareMay 17, 2019 13:27
@wskorodecki
Copy link
ContributorAuthor

I've updated the patch.

@fabpot
Copy link
Member

Thank you@wskorodecki.

@fabpotfabpot merged commit20c587f intosymfony:4.3May 18, 2019
fabpot added a commit that referenced this pull requestMay 18, 2019
…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
@fabpotfabpot mentioned this pull requestMay 22, 2019
fabpot added a commit that referenced this pull requestMay 25, 2019
…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.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wskorodecki@xabbuh@nicolas-grekas@derrabus@fabpot@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp