8000 [WebProfilerBundle] Fixed error from unset twig variable by simonsargeant · Pull Request #18457 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[WebProfilerBundle] Fixed error from unset twig variable #18457

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
wants to merge 2 commits into from
Closed

[WebProfilerBundle] Fixed error from unset twig variable #18457

wants to merge 2 commits into from

Conversation

simonsargeant
Copy link
Contributor
Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Minor bug, fixes error from twig variable which was removed in 2.8
Here was where it was originally set https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/config.html.twig#L69

Replaced with expected class name

@javiereguiluz
Copy link
Member

@simonsargeant thanks for fixing this. It looks good to me 👍

However, there is a big problem with the code we use for this:

{% if 'n/a' != collector.debug %}
    ...
{% endif %}

collector.debug is a boolean value and we're comparing it with a string (n/a). In this case n/a is transformed to true and the comparison is if true != collector.debug. That's why we never see this information in the toolbar.

Should we fix that in this PR or create a new PR?

@stof
Copy link
Member
stof commented Apr 6, 2016

@javiereguiluz do we actually need the color here ?

@javiereguiluz
Copy link
Member

@stof I don't have a strong opinion about this. But the color may be consistent with other colored blocks in that panel and it doesn't hurt:

Color No Color
color no_color

@simonsargeant
Copy link
Contributor Author

@javiereguiluz I think this is now expected behavior as collector.debug could be boolean or 'n/a' https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/DataCollector/ConfigDataCollector.php#L69. Does not show the debug status if 'n/a', shows activated or deactivated if true or false.

@javiereguiluz
Copy link
Member

@simonsargeant yes, the collector can be true, false or 'n/a' ... but the problem is in the condition:

Env collector.debug 'n/a' != collector.debug Result
dev true false You don't see this in the toolbar
prod false true You see this in the toolbar ... but there is no toolbar because we are in prod
- n/a false You don't see this in the toolbar ... but I've never faced this condition

So the only scenario where this is true is in the prod environment, where the toolbar is not displayed

@simonsargeant
Copy link
Contributor Author

@javiereguiluz Sorry, I meant I had updated the PR so the behaviour is now:

collector.debug 'n/a' is not same as collector.debug Result
true true Shown in toolbar (green)
false true Shown in toolbar (red)
'n/a' false Not shown in toolbar

@javiereguiluz
Copy link
Member

👍 Thanks @simonsargeant

@stof
Copy link
Member
stof commented Apr 7, 2016

👍

@fabpot
Copy link
Member
fabpot commented Apr 7, 2016

Thank you @simonsargeant.

fabpot added a commit that referenced this pull request Apr 7, 2016
@fabpot
bug #18457 [WebProfilerBundle] Fixed error from unset twig variable (…
1db0925 
…simonsargeant)

This PR was squashed before being merged into the 2.8 branch (closes #18457).

Discussion
----------

[WebProfilerBundle] Fixed error from unset twig variable

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Minor bug, fixes error from twig variable which was removed in 2.8
Here was where it was originally set https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/config.html.twig#L69

Replaced with expected class name

Commits
-------

3e2c4c9 [WebProfilerBundle] Fixed error from unset twig variable
@fabpot fabpot closed this Apr 7, 2016
This was referenced Apr 29, 2016
Merged
Merged
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
@fabpot
bug symfony#18457 [WebProfilerBundle] Fixed error from unset twig var…
840dd80 
…iable (simonsargeant)

This PR was squashed before being merged into the 2.8 branch (closes symfony#18457).

Discussion
----------

[WebProfilerBundle] Fixed error from unset twig variable

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Minor bug, fixes error from twig variable which was removed in 2.8
Here was where it was originally set https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/WebProfilerBundle/Resources/views/Collector/config.html.twig#L69

Replaced with expected class name

Commits
-------

3e2c4c9 [WebProfilerBundle] Fixed error from unset twig variable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug Status: Reviewed WebProfilerBundle
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@simonsargeant @javiereguiluz @stof @fabpot @carsonbot
0