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

[DI] Fix accepting null as default env param value#20512

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:env-no-null

Nov 15, 2016

Merged

[DI] Fix accepting null as default env param value#20512

fabpot merged 1 commit intosymfony:masterfromnicolas-grekas:env-no-null

Nov 15, 2016

Conversation

Copy link

Member

nicolas-grekas commentedNov 14, 2016•
edited
Loading

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

carsonbot added Status: Needs Review Bug labels

Nov 14, 2016

Copy link

Member

stof commentedNov 14, 2016

but shouldn't we allow it ? It would be useful in case some config treats null as being "don't enable this feature", which is the case for the database_url in Doctrine (allowing to configure with username/password/host locally when not setting the URL)

Copy link

MemberAuthor

nicolas-grekas commentedNov 14, 2016

Could be, but should be allowed for all parameters then (for consistency, and because we use the same code paths for all params, env or not)

Copy link

Member

stof commentedNov 14, 2016

normal parameters (non-env-based) allow to storenull (they also allow to store arrays, contrary to env parameters)

nicolas-grekas force-pushed theenv-no-null branch 2 times, most recently from9b424a1 toc78e090Compare

November 14, 2016 11:10

Copy link

MemberAuthor

nicolas-grekas commentedNov 14, 2016•
edited
Loading

Indeed, I messed up here :) Fixed!

nicolas-grekas changed the title~~[DI] Fix misleading exception message~~[DI] Fix accepting null as default env param value

Nov 14, 2016

javiereguiluz reviewed

Nov 14, 2016

View reviewed changes

src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php Outdated

		publicfunctiontestResolveEnvAllowsNull()
		{
		$bag =newEnvPlaceholderParameterBag();
		$bag->get('env(NULL)');

Copy link

Member

javiereguiluzNov 14, 2016

Choose a reason for hiding this comment

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

It's a bit confusing in this test to useNULL as the var name. What about replacing it byNULL_VAR ? (andNull_Var in the example below)

Copy link

MemberAuthor

nicolas-grekasNov 15, 2016

Choose a reason for hiding this comment

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

updated

Copy link

Member

dunglas commentedNov 14, 2016

👍

xabbuh added the DependencyInjection label

Nov 14, 2016

nicolas-grekas force-pushed theenv-no-null branch fromc78e090 to98c1401Compare

November 15, 2016 12:04

[DI] Fix accepting null as default env param value

7625166

nicolas-grekas force-pushed theenv-no-null branch from98c1401 to7625166Compare

November 15, 2016 12:20

Copy link

Member

xabbuh commentedNov 15, 2016

👍

Status: Reviewed

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Nov 15, 2016

Copy link

Member

fabpot commentedNov 15, 2016

Thank you@nicolas-grekas.

fabpot merged commit7625166 intosymfony:master

Nov 15, 2016

fabpot added a commit that referenced this pull request

Nov 15, 2016

bug#20512[DI] Fix accepting null as default env param value (nicola…

f0aa663

…s-grekas)This PR was merged into the 3.2-dev branch.Discussion----------[DI] Fix accepting null as default env param value| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#20447| License       | MIT| Doc PR        | -Commits-------7625166 [DI] Fix accepting null as default env param value

nicolas-grekas deleted the env-no-null branch

November 16, 2016 10:15

fabpot mentioned this pull request

Nov 17, 2016

Release v3.2.0-RC1#20552

Merged