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] Split events across requests#29312
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
now called listeners fixed. It's definitely better from the profiler pov.
The UI solves itself btw :) great.
(main request)
(sub request)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| $this->called[$eventName]->attach($listener); | ||
| $this->callStack->attach($listener,array($eventName,$this->currentRequestHash)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
same issue with keys; grouping by event name breaks the dispatch order
Uh oh!
There was an error while loading.Please reload this page.
ro0NL commentedNov 25, 2018
Last but not least, i think we should append eachwrapped listener to the call stack duringpre process and lazy verify if it was called yes/no. Currently it's added to the call stack during post-process which affects the order. I.e. the main kernel.exception is called before the sub kernel.reques, but we only collect that after the sub request finished. |
ro0NL commentedNov 25, 2018
Ive updated above screenshots afterc737944 |
This PR was merged into the 4.1 branch.Discussion----------[WebProfilerBundle] Fix title case| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | yes| New feature? | no| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Split from#29312 for 4.1 👼Commits-------3e16e25 [WebProfilerBundle] Fix title case
This PR was merged into the 3.4 branch.Discussion----------[EventDispatcher] Revers event tracing order| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Split from#29312 for 3.4This traces events in dispatch order.Before:After:Here we see it also collects "terminate" events (both kernel & console for not-called listeners). In case of exception page the fix is even better:#29312 (review)Which now correctly shows the kernel.exception event of the main request is dispatched _before_ the kernel.request event of the sub-request.Moreover, it de-duplicates events. So we actually see the sub-request events 🎉_Havent looked at tests yet._Commits-------2570d6f [EventDispatcher] Revers event tracing order
This PR was merged into the 3.4 branch.Discussion----------[EventDispatcher] Revers event tracing order| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Split from #29312 for 3.4This traces events in dispatch order.Before:After:Here we see it also collects "terminate" events (both kernel & console for not-called listeners). In case of exception page the fix is even better:symfony/symfony#29312 (review)Which now correctly shows the kernel.exception event of the main request is dispatched _before_ the kernel.request event of the sub-request.Moreover, it de-duplicates events. So we actually see the sub-request events 🎉_Havent looked at tests yet._Commits-------2570d6f877 [EventDispatcher] Revers event tracing order
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about making the class@final? (if you agree, don't forget updating the changelog file)
- rebase needed btw
Uh oh!
There was an error while loading.Please reload this page.
fabpot commentedMar 31, 2019
@ro0NL Do you think you can finish this one for 4.3? |
ro0NL commentedApr 2, 2019
@fabpot done. |
fabpot commentedApr 2, 2019
@ro0NL Apparently, tests do not pass. Can you have a look please? |
ro0NL commentedApr 2, 2019
Let's see now :) i think it's a shortcoming of sf/debug not accounting for |
nicolas-grekas commentedApr 2, 2019
It's not a bug it's a feature. You must have to change your code when a lower layer decides to deprecate something. This is how. |
fabpot commentedApr 2, 2019
Thank you@ro0NL. |
This PR was squashed before being merged into the 4.3-dev branch (closes#29312).Discussion----------[EventDispatcher] Split events across requests| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes <!-- please add some, will be required by reviewers -->| Fixed tickets |#24275| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->Split events per request, as currently a profiled sub-request includes all events. Follows same approach how logs are split in#23659.Commits-------c3477ba [EventDispatcher] Split events across requests
Uh oh!
There was an error while loading.Please reload this page.
Split events per request, as currently a profiled sub-request includes all events. Follows same approach how logs are split in#23659.