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 misresolved parameters in debug:config #20909

Merged
merged 1 commit into from
Dec 14, 2016

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Dec 13, 2016

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

@chalasr
Copy link
Member Author

chalasr commented Dec 13, 2016

The patch proposed by fabbot is not related to this PR thus cannot be applied.


private static function resolveParametersValues(ContainerBuilder $builder, $configs)
{
return $builder->resolveEnvPlaceholders($builder->getParameterBag()->resolveValue($configs));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the need for this method

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it could ease updating this command again after #20618, removed

@nicolas-grekas
Copy link
Member

Why that? I think we can apply it, wouldn't be the first time.

@chalasr chalasr force-pushed the fix/debug-config-resolve-params branch 3 times, most recently from 5f1b505 to 1d17ae2 Compare December 13, 2016 19:07
@chalasr
Copy link
Member Author

chalasr commented Dec 13, 2016

From our contributing guidelines:

Never fix coding standards in some existing code as it makes the code review more difficult;

Applied! :)

@nicolas-grekas
Copy link
Member

👍

@chalasr chalasr force-pushed the fix/debug-config-resolve-params branch from 1d17ae2 to c88bc89 Compare December 13, 2016 19:36
@chalasr chalasr changed the title Fix misresolved parameters in debug:config on 3.2 [FrameworkBundle] Fix misresolved parameters in debug:config Dec 13, 2016
@fabpot
Copy link
Member

fabpot commented Dec 14, 2016

Thank you @chalasr.

@fabpot fabpot merged commit c88bc89 into symfony:3.2 Dec 14, 2016
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
@chalasr chalasr deleted the fix/debug-config-resolve-params branch December 14, 2016 21:49
@fabpot fabpot mentioned this pull request Jan 12, 2017
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.

4 participants