-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Maintain the selected panel when redirecting to another profile #20646
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
Conversation
javiereguiluz
commented
Nov 26, 2016
Q | A |
---|---|
Branch? | 3.1 |
Bug fix? | no |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #20637 |
License | MIT |
Doc PR | - |
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.
👍
@@ -39,7 +39,7 @@ | |||
{%- else -%} | |||
{{ redirect_route }} | |||
{%- endif %} | |||
(<a href="{{ path('_profiler', { token: redirect.token }) }}">{{ redirect.token }}</a>) | |||
(<a href="{{ path('_profiler', { token: redirect.token, panel: app.request.query.get('panel', 'request') }) }}">{{ redirect.token }}</a>) |
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.
Not sure it should hardcode the default panel name (request
). It's not needed right?
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.
In case it's null I'd prefer to set the default panel name instead of relying of auto-redirection to some panel. But your proposal is correct too. They are just different opinions on the same subject!
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.
@javiereguiluz please use request
, not app.request
. The WebProfilerBundle does not depend on the app
global variable before your change, making it compatible with Silex (where the app
global variable is the Silex application itself, and so no compatible with app.request
)
Thank you @javiereguiluz. |
…ofile (javiereguiluz) This PR was merged into the 3.1 branch. Discussion ---------- Maintain the selected panel when redirecting to another profile | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20637 | License | MIT | Doc PR | - Commits ------- de7b326 Maintain the selected panel when redirecting to another profile
…iereguiluz) This PR was merged into the 3.1 branch. Discussion ---------- Don't use the "app" global variable in the profiler | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - The use of the `app` variable makes this incompatible with projects like Silex (as pointed by @stof in #20646 (comment)) Commits ------- a777618 Don't use the "app" global variable in the profiler
…ther profile (javiereguiluz) This PR was merged into the 3.1 branch. Discussion ---------- Maintain the selected panel when redirecting to another profile | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#20637 | License | MIT | Doc PR | - Commits ------- de7b326 Maintain the selected panel when redirecting to another profile