Skip to content
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] Removed "callable" type hint from WrappedListener constructor #31493

Merged
merged 1 commit into from May 18, 2019

Conversation

Projects
None yet
6 participants
@wskorodecki
Copy link
Contributor

commented May 13, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
License MIT

The problem

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:

class SomeListener
{
    public function myListenerMethod(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'

@wskorodecki wskorodecki marked this pull request as ready for review May 13, 2019

@xabbuh

This comment has been minimized.

Copy link
Member

commented May 13, 2019

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

@wskorodecki wskorodecki force-pushed the wskorodecki:bugfix/wrapped-listener branch 6 times, most recently from 6878bf7 to 067a900 May 14, 2019

@wskorodecki

This comment has been minimized.

Copy link
Contributor Author

commented May 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 the callable type hint, which should remain.
I've updated the RegisterListenersPass and RegisterListenersPassTest.

@wskorodecki wskorodecki force-pushed the wskorodecki:bugfix/wrapped-listener branch from 067a900 to 1093078 May 14, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 14, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

We ran into this issue as well. In our case, the fix was to delete the corresponding addListener() 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:41
Stack 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 to WrappedListener 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

This comment has been minimized.

Copy link
Member

commented May 16, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

Did anyone check why we added the type btw?

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 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.

@wskorodecki wskorodecki force-pushed the wskorodecki:bugfix/wrapped-listener branch from 1093078 to 20c587f May 17, 2019

@wskorodecki

This comment has been minimized.

Copy link
Contributor Author

commented May 18, 2019

I've updated the patch.

@fabpot

fabpot approved these changes May 18, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented May 18, 2019

Thank you @wskorodecki.

@fabpot fabpot merged commit 20c587f into symfony:4.3 May 18, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request May 18, 2019

bug #31493 [EventDispatcher] Removed "callable" type hint from Wrappe…
…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

@fabpot fabpot referenced this pull request May 22, 2019

Merged

Release v4.3.0-BETA2 #31577

fabpot added a commit that referenced this pull request May 25, 2019

bug #31615 Allow WrappedListener to describe uncallable listeners (de…
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.