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

[FrameworkBundle] Fix unresolved parameters from default configs in debug:config #20714

Merged
merged 1 commit into from Dec 13, 2016

Conversation

Projects
None yet
6 participants
@chalasr
Member

chalasr commented Dec 1, 2016

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

When using the debug:config command, if the dumped configuration is explicitly defined by the user, then parameters are properly resolved in the output. If it is not, and values come from the bundle default configuration directly, they are not.

Steps to reproduce:

  • Checkout the symfony demo
  • Run debug:config twig
  • Look at the debug key, it is the kernel.debug parameter properly resolved: true
  • Look at the cache key, it is not resolved: '%kernel.cache_dir%/twig'

This fixes it by resolving configs once again after processing the configuration.
ping @weaverryan

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Dec 1, 2016

👍

Status: Reviewed

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 1, 2016

Maybe not what you'd expect, yet the current behavior on 2.7 is consistent, not buggy.
If we need to reconsider this, we should do it on 3.3 IMHO.

@chalasr

This comment has been minimized.

Member

chalasr commented Dec 1, 2016

@nicolas-grekas Not sure to understand. Why should parameters be resolved only for explicitly defined configuration?

On the current 2.7, I can get both %kernel.root_dir% for a key and its value for another, imho it's not a consistent behavior. Whether we dump placeholders or values, but not both.

@chalasr

This comment has been minimized.

Member

chalasr commented Dec 1, 2016

Maybe it's not clear what I mean.

config:

twig:
    debug:            "%kernel.debug%"
    strict_variables: "%kernel.debug%"
    form_themes:
        - "bootstrap_3_layout.html.twig"
        - "form/fields.html.twig"

output before:
before

output after:
after

I find the debug:config command especially useful to be aware of what is configured implicitly (i.e. default bundle configurations), which is no more true if the output doesn't contain real values, and quite confusing if one key contains the real value and the second doesn't.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Dec 6, 2016

@chalasr

This comment has been minimized.

Member

chalasr commented Dec 9, 2016

So isn't this the expected behavior? Is there any blocker for merging this as a bugfix?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 12, 2016

👍

1 similar comment
@stof

This comment has been minimized.

Member

stof commented Dec 12, 2016

👍

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 13, 2016

Thanks for fixing this bug @chalasr.

@fabpot fabpot merged commit 26f588a into symfony:2.7 Dec 13, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Dec 13, 2016

bug #20714 [FrameworkBundle] Fix unresolved parameters from default c…
…onfigs in debug:config (chalasr)

This PR was merged into the 2.7 branch.

Discussion
----------

[FrameworkBundle] Fix unresolved parameters from default configs in debug:config

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

When using the `debug:config` command, if the dumped configuration is explicitly defined by the user, then parameters are properly resolved in the output. If it is not, and values come from the bundle default configuration directly, they are not.

Steps to reproduce:
- Checkout the symfony demo
- Run `debug:config twig`
- Look at the `debug` key, it is the `kernel.debug` parameter properly resolved: `true`
- Look at the `cache` key, it is not resolved: `'%kernel.cache_dir%/twig'`

This fixes it by resolving the configs once again after processing the configuration.
ping @weaverryan

Commits
-------

26f588a Fix unresolved parameters from default bundle configs in debug:config

@chalasr chalasr deleted the chalasr:debug-config-resolve-defaults branch Dec 13, 2016

fabpot added a commit that referenced this pull request Dec 14, 2016

bug #20909 Fix misresolved parameters in debug:config on 3.2 (chalasr)
This PR was merged into the 3.2 branch.

Discussion
----------

Fix misresolved parameters in debug:config on 3.2

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

This fixes parameters resolution (classic and env ones) in `debug:config`, again.
Merging #20714 broke the fix resolving env parameters made in #20688, and anyway it was mismerged (#20714 was not applied when using the `path` argument, my bad, I should have prevented it).

This adds a test which prevents regressions so I hope this is is the last PR on this subject.
The buggy output is unfortunately part of the last 3.2 release... It can easily be confirmed by running `debug:config doctrine` on a fresh symfony-demo project

Commits
-------

c88bc89 Fix misresolved parameters in debug:config on 3.2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment