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

Maintain the selected panel when redirecting to another profile #20646

Merged
merged 1 commit into from
Dec 6, 2016

Conversation

javiereguiluz
Copy link
Member

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 -

Copy link
Contributor

@ro0NL ro0NL left a 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>)
Copy link
Contributor

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?

Copy link
Member Author

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!

Copy link
Member

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)

@nicolas-grekas nicolas-grekas added this to the 3.1 milestone Dec 6, 2016
@fabpot
Copy link
Member

fabpot commented Dec 6, 2016

Thank you @javiereguiluz.

@fabpot fabpot merged commit de7b326 into symfony:3.1 Dec 6, 2016
fabpot added a commit that referenced this pull request Dec 6, 2016
…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
javiereguiluz added a commit that referenced this pull request Dec 6, 2016
…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
This was referenced Dec 13, 2016
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
…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
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