Skip to content

[DI] Fix merging of env vars in configs #23903

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
Merged
merged 1 commit into from
Aug 18, 2017
Merged

[DI] Fix merging of env vars in configs #23903

merged 1 commit into from
Aug 18, 2017

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Aug 16, 2017 •
edited
Loading

Q A
Branch? 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22561, #23520
License MIT
Doc PR -

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Aug 16, 2017
This was referenced Aug 16, 2017
Closed
Closed
@nicolas-grekas nicolas-grekas force-pushed the di-merge-env branch 2 times, most recently from 822cb89 to 1c4af6b Compare August 17, 2017 12:56
@nicolas-grekas
Copy link
Member Author

Because processing configuration is done internally in Extension::load(), we have no proper way to get the processed configuration from the outside.

I didn't find any better implementation than the one in this PR, which consist of storing processed configs when Extension::processConfiguration() is being called, then fetching these with a new Extension::getProcessedConfigs() method.

This works only if extensions do call processConfiguration() of course.
I'd expect this to be a common enough situation to consider the issue fixed.
If the expectation is not met, the outcome is that overridden env vars are tracked again, which is not a huge issue anyway, and is just the current situation.

Decoupling config processing from loading is another possibility, but that'd be asking for big changes for all bundle authors. Not worth it IMHO.

PR is ready (failures are false positives.)

fabpot
fabpot requested changes Aug 18, 2017
View reviewed changes
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

I think we need copious comments so that we can still understand the logic in 2 years :)

src/Symfony/Component/DependencyInjection/Extension/Extension.php Outdated
return $this->processedConfigs[] = $processor->processConfiguration($configuration, $configs);
}

final public function getProcessedConfigs()
Copy link
Member

Choose a reason for hiding this comment

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

/** @internal */?

Copy link
Member Author

Choose a reason for hiding this comment

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

so it is

@nicolas-grekas
Copy link
Member Author

comments added, should be ready

Taluu
Taluu reviewed Aug 18, 2017
View reviewed changes
src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php
@@ -106,11 +106,11 @@ public function resolve()
/**
* Replaces "%env(FOO)%" references by their placeholder, keeping regular "%parameters%" references as is.
*/
public function resolveEnvReferences()
public function resolveEnvReferences(array $value)
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this a BC break ?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is not released yet, 3.3@dev only

@nicolas-grekas
Copy link
Member Author

Merging to unlock another PR to come fixing a bug in 3.3 also.
If you have further comments please still add them.

@nicolas-grekas nicolas-grekas merged commit 00b9273 into symfony:3.3 Aug 18, 2017
nicolas-grekas added a commit that referenced this pull request Aug 18, 2017
@nicolas-grekas
bug #23903 [DI] Fix merging of env vars in configs (nicolas-grekas)
1732cc8 
This PR was merged into the 3.3 branch.

Discussion
----------

[DI] Fix merging of env vars in configs

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22561, #23520
| License       | MIT
| Doc PR        | -

Commits
-------

00b9273 [DI] Fix merging of env vars in configs
@nicolas-grekas nicolas-grekas deleted the di-merge-env branch August 18, 2017 14:06
@fabpot fabpot mentioned this pull request Aug 28, 2017
Merged
@PhilETaylor PhilETaylor mentioned this pull request Aug 28, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Taluu Taluu Taluu left review comments

@fabpot fabpot fabpot requested changes

Assignees
No one assigned
Labels
Bug DependencyInjection Status: Needs Review
Projects
None yet
Milestone
3.3
Development

Successfully merging this pull request may close these issues.
4 participants
@nicolas-grekas @fabpot @Taluu @carsonbot