8000 [WebProfilerBundle] Some listeners don't display their priority · Issue #22520 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[WebProfilerBundle] Some listeners don't display their priority #22520

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.

Sign up for GitHub

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

Jump to bottom
Closed
javiereguiluz opened this issue Apr 25, 2017 · 12 comments
Closed

[WebProfilerBundle] Some listeners don't display their priority #22520

javiereguiluz opened this issue Apr 25, 2017 · 12 comments
Labels
Bug Status: Needs Review WebProfilerBundle

Comments

@javiereguiluz
Copy link
Member
javiereguiluz commented Apr 25, 2017 •
edited
Loading
Q A
Bug report? yes
Feature request? no
BC Break report? no
Symfony version all

In the profiler, the listeners related to the kernel.terminate event don't display the priority:

undefined-priority

However, those services define their priority:

defined-priority

But this is somehow lost and the priority is null in the profiler:

null-priority
@stof
Copy link
Member
stof commented Apr 25, 2017

I suspect this is because the collection of data about listeners is done during the kernel.terminate event. And so, during this event, the dispatcher does not contain the actual listeners but wrapped versions of them.

The TraceableEventDispatcher probably need to unwrap the listener before forwarding the call, as done for removeListener (except that it should not remove the wrapper of course)

@stof
Copy link
Member
stof commented Apr 26, 2017

/cc @nicolas-grekas

@dmaicher
Copy link
Contributor

On my Symfony 2.8 app I can see the priority for the kernel.terminate listener (there is only one listed):

listener_prio

@javiereguiluz
Copy link
Member Author
javiereguiluz commented Apr 26, 2017 •
edited
Loading

I used the Symfony Demo app, so Symfony 3.2.5 for me.

@stof
Copy link
Member
stof commented Apr 26, 2017 •
edited
Loading

@dmaicher the implementation in 2.8 is totally different (and it shows the wrapper instead of the actual listener)

@dmaicher
Copy link
Contributor

@stof ok thats what I thought but above it says Symfony version: all 😋

@stof
Copy link
Member
stof commented Apr 26, 2017

Well, I think it is a mistake.

The 2.8 version has actually a different issue. It has the right priority, but not the right callable

@dmaicher dmaicher mentioned this issue Apr 26, 2017
Merged
@dmaicher
Copy link
Contributor

I fixed the problem for 2.8 in this PR #22541.

I will also try to have a look on 3.2.

fabpot added a commit that referenced this issue Apr 26, 2017
@fabpot
bug #22541 [EventDispatcher] fix: unwrap listeners for correct info (…
e9e4c79 
…dmaicher)

This PR was merged into the 2.8 branch.

Discussion
----------

[EventDispatcher] fix: unwrap listeners for correct info

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | related to #22520
| License       | MIT
| Doc PR        | -

Fixes problem where listeners were displayed as `Symfony\Component\EventDispatcher\Debug\WrappedListener::__invoke() ` in the profiler. It now shows the correct callable.

Commits
-------

d8c5869 [EventDispatcher] fix: unwrap listeners for correct info
@dmaicher
Copy link
Contributor

@javiereguiluz can you try this diff for 3.2? I currently don't have an app running with it 😋

diff --git a/src/Symfony/Component/EventDispatcher/Debug/WrappedListener.php b/src/Symfony/Component/EventDispatcher/Debug/WrappedListener.php
index 45208a19b2..59afdc0f71 100644
--- a/src/Symfony/Component/EventDispatcher/Debug/WrappedListener.php
+++ b/src/Symfony/Component/EventDispatcher/Debug/WrappedListener.php
@@ -89,9 +89,18 @@ class WrappedListener
             $this->data = false !== self::$cloner ? self::$cloner->cloneVar(array(new ClassStub($this->pretty.'()', $this->listener)))->seek(0) : $this->pretty;
         }
 
+        $priority = null;
+        if (null !== $this->dispatcher) {
+            $priority = $this->dispatcher->getListenerPriority($eventName, $this->listener);
+            // if we try to get the priority while an event is dispatched the wrapper is registered so fallback here
+            if (null === $priority) {
+                $priority = $this->dispatcher->getListenerPriority($eventName, $this);
+            }
+        }
+
         return array(
             'event' => $eventName,
-            'priority' => null !== $this->dispatcher ? $this->dispatcher->getListenerPriority($eventName, $this->listener) : null,
+            'priority' => $priority,
             'pretty' => $this->pretty,
             'data' => $this->data,
         );

I think the problem is indeed that during dispatching the wrapped versions are registered and so getting the priority for the original listener fails as @stof also suspected.

@javiereguiluz
Copy link
Member Author

@dmaicher I've just tested your solution and I can confirm that it fixes the problem to me. Thanks!

By the way, if you need a Symfony 3.2 app to do quick tests you can use the Symfony Demo app which can be installed in just a few seconds.

@dmaicher
Copy link
Contributor

@javiereguiluz ok cool 😊 Then I will prepare a PR later today.

True next time I will just use the demo app indeed 👍 Thanks for the tip.

@dmaicher dmaicher mentioned this issue Apr 28, 2017
Merged
@dmaicher
Copy link
Contributor

Here is the fix: #22568

nicolas-grekas added a commit that referenced this issue Apr 28, 2017
@nicolas-grekas
bug #22568 [EventDispatcher] fix getting priorities of listeners duri…
c590e85 
…ng dispatch (dmaicher)

This PR was merged into the 3.2 branch.

Discussion
----------

[EventDispatcher] fix getting priorities of listeners during dispatch

| Q             | A
| ------------- | ---
| Branch?       | 3.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22520
| License       | MIT
| Doc PR        | -

The WebProfiler collects listener info during dispatching the `kernel.terminate` event which caused issues while retrieving priorities as the wrappers are registered instead of the real listeners.

Commits
-------

79b71c0 [EventDispatcher] fix getting priorities of listeners during dispatch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review WebProfilerBundle
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@javiereguiluz @nicolas-grekas @stof @dmaicher @carsonbot
0