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

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 18, 2019

Conversation

wskorodecki
Copy link
Contributor

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'

@xabbuh
Copy link
Member

xabbuh 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 bugfix/wrapped-listener branch 6 times, most recently from 6878bf7 to 067a900 Compare May 14, 2019 07:36
@wskorodecki
Copy link
Contributor Author

@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 bugfix/wrapped-listener branch from 067a900 to 1093078 Compare May 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 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
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 use Closure::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.

@wskorodecki wskorodecki force-pushed the bugfix/wrapped-listener branch from 1093078 to 20c587f Compare May 17, 2019 13:27
@wskorodecki
Copy link
Contributor Author

I've updated the patch.

@fabpot
Copy link
Member

fabpot commented May 18, 2019

Thank you @wskorodecki.

@fabpot fabpot merged commit 20c587f into symfony:4.3 May 18, 2019
fabpot added a commit that referenced this pull request May 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

```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 mentioned this pull request May 22, 2019
fabpot added a commit that referenced this pull request May 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/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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.