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

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

Merged
merged 1 commit into from Dec 13, 2016

Conversation

chalasr
Copy link
Member

@chalasr 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
Copy link
Member

👍

Status: Reviewed

@nicolas-grekas
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

chalasr commented Dec 9, 2016

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

@nicolas-grekas
Copy link
Member

👍

1 similar comment
@stof
Copy link
Member

stof commented Dec 12, 2016

👍

@fabpot
Copy link
Member

fabpot commented Dec 13, 2016

Thanks for fixing this bug @chalasr.

@fabpot fabpot merged commit 26f588a into symfony:2.7 Dec 13, 2016
fabpot added a commit that referenced this pull request Dec 13, 2016
…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 debug-config-resolve-defaults branch December 13, 2016 08:15
fabpot added a commit that referenced this pull request Dec 14, 2016
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants