NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[DI] ContainerBuilder::compile() can optionally resolve env vars in parameter bag#21460

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromnicolas-grekas:di-compile-env

Feb 9, 2017

Merged

[DI] ContainerBuilder::compile() can optionally resolve env vars in parameter bag#21460

fabpot merged 1 commit intosymfony:masterfromnicolas-grekas:di-compile-env

Feb 9, 2017

Conversation

Copy link

Member

nicolas-grekas commentedJan 30, 2017•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets	#21420
License	MIT
Doc PR	-

Here is a new feature allowing one to do$container->compile(true) and get the env vars resolved using the current env.

carsonbot added Status: Needs Review DependencyInjection Feature labels

Jan 30, 2017

nicolas-grekas mentioned this pull request

Jan 30, 2017

[DI] Unable to resolve env()#21420

Closed

nicolas-grekas added this to the3.3 milestone

Jan 30, 2017

Copy link

Member

chalasr commentedJan 30, 2017•
edited
Loading

👍

olvlvl reviewed

Jan 30, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/ContainerBuilder.php Outdated

		* * Extension loading is disabled.
		*/
		publicfunctioncompile()
		publicfunctioncompile(/* $resolveEnvPlaceholders = false */)

Copy link

Contributor

olvlvlJan 30, 2017•
edited
Loading

Choose a reason for hiding this comment

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

How about using a flag (e.g.self::RESOLVE_ENV_PLACEHOLDERS) instead of a boolean, so that additional features may be introduced without requiring additional parameters?

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

IMHO, we shouldn't plan for additional unplanned features :) This can be changed easily in the futureif needed.

javiereguiluz reviewed

Jan 30, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php

		$_ENV['DUMMY_ENV_VAR'] ='du%%y';

		$container =newContainerBuilder();
		$container->setParameter('bar','%% %env(DUMMY_ENV_VAR)%');

Copy link

Member

Choose a reason for hiding this comment

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

What will happen in the other two cases: 1) undefined env var; 2) undefined env var, but default value defined for it. Should we add those cases to this test too?

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

same as usual, this is exactly the same code/logic

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

tests added

nicolas-grekas force-pushed thedi-compile-env branch from8b278b2 to13ea687Compare

January 30, 2017 16:37

Copy link

Member

fabpot commentedJan 30, 2017

Can you add some docs in the phpdocs?

nicolas-grekas force-pushed thedi-compile-env branch from13ea687 toff4c1f0Compare

January 30, 2017 20:37

Copy link

MemberAuthor

nicolas-grekas commentedJan 30, 2017

phpdoc added

nicolas-grekas force-pushed thedi-compile-env branch fromff4c1f0 to1a2b1cbCompare

January 31, 2017 12:20

fabpot reviewed

Feb 1, 2017

View reviewed changes

src/Symfony/Component/DependencyInjection/ContainerBuilder.php Outdated

		* * Extension loading is disabled.
		*
		* @param bool $resolveEnvPlaceholders Whether %env()% parameters should be resolved using the current
		* env vars or be replaced by uniquely identifiable placeholders

Copy link

Member

Choose a reason for hiding this comment

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

That describes what it does, but I'd like to understand alsowhen I should passtrue here.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

comment updated

nicolas-grekas force-pushed thedi-compile-env branch 3 times, most recently froma735b0e to6ea3a24Compare

February 3, 2017 12:26

[DI] ContainerBuilder::compile() can optionally resolve env vars in p…

a3fd512

…arameter bag

nicolas-grekas force-pushed thedi-compile-env branch from6ea3a24 toa3fd512Compare

February 9, 2017 15:17

Copy link

Member

fabpot commentedFeb 9, 2017

Thank you@nicolas-grekas.

fabpot merged commita3fd512 intosymfony:master

Feb 9, 2017

fabpot added a commit that referenced this pull request

Feb 9, 2017

feature#21460[DI] ContainerBuilder::compile() can optionally resolv…

10c3fc2

…e env vars in parameter bag (nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[DI] ContainerBuilder::compile() can optionally resolve env vars in parameter bag| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#21420| License       | MIT| Doc PR        | -Here is a new feature allowing one to do `$container->compile(true)` and get the env vars resolved using the current env.Commits-------a3fd512 [DI] ContainerBuilder::compile() can optionally resolve env vars in parameter bag

nicolas-grekas deleted the di-compile-env branch

February 14, 2017 16:21

fabpot mentioned this pull request

May 1, 2017

Release v3.3.0-BETA1#22603

Merged

Copy link

zippy1981 commentedMay 18, 2017

I tried to capture how to do thisin a stackoverflow answer.

drzraf mentioned this pull request

Jun 9, 2017

issue #982: support environment variable inside behat.ymlBehat/Behat#1048

Merged

Copy link

@zippy1981 Thanks for that SO answer, however I can't seem to get it working. If I replace$container->compile(); with$container->compile(true); in theinitializeContainer() method ofSymfony\Component\HttpKernel\Kernel then everything works fine, however if I add$container->compile(true); to my own bundle then nothing happens. Where exactly should I add this line?

Copy link

Member

stof commentedJun 15, 2017

@pwm why would you want to use this argument in your bundle ?

Copy link

pwm commentedJun 16, 2017

@stof I'm trying to get env vars work as monolog config values, pretty much exactly like the SO entry above, but I get anIncompatible use of dynamic environment variables... error. If I understand it correctly this PR is resolving this issue.