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

Change priority of KernelEvents::RESPONSE subscriber #36789

Merged
merged 1 commit into from May 14, 2020

Conversation

marcw
Copy link
Contributor

@marcw marcw commented May 12, 2020

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
License MIT

This PR changes the priority of the KernelEvents::RESPONSE subscriber of the ProfilerListener so that it is the penultimate to be executed (just before StreamedResponseListener).

The reason is that other listeners that were executed after this one CAN change the response (such as SessionListener for example). This creates a headache when debugging, with a discrepancy between what is shown in a curl command, and by the Symfony profiler.

@fabpot fabpot force-pushed the make-profiler-listener-penultimate branch from a379912 to 6ed624a Compare May 14, 2020 09:30
@fabpot
Copy link
Member

fabpot commented May 14, 2020

Thank you @marcw.

@fabpot fabpot merged commit e5c82c5 into symfony:4.4 May 14, 2020
@fabpot fabpot mentioned this pull request May 16, 2020
@derrabus
Copy link
Member

Looks like this PR has caused a regression: #36836.

fabpot added a commit that referenced this pull request May 18, 2020
This PR was merged into the 4.4 branch.

Discussion
----------

[HttpKernel] Bring back the debug toolbar

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #36836
| License       | MIT
| Doc PR        | N/A

This PR effectively reverts #36789 in order to fix a regression caused by that PR.

Commits
-------

9f8d225 Revert "Change priority of KernelEvents::RESPONSE subscriber"
This was referenced May 31, 2020
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.

None yet

7 participants