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

Deprecate the special SYMFONY__ environment variables#20100

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

Closed

javiereguiluz wants to merge4 commits intosymfony:masterfromjaviereguiluz:fix_20089

Closed

Deprecate the special SYMFONY__ environment variables#20100

javiereguiluz wants to merge4 commits intosymfony:masterfromjaviereguiluz:fix_20089

Conversation

Copy link

Member

javiereguiluz commentedSep 30, 2016

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

javiereguiluz added HttpKernel Deprecation labels

Sep 30, 2016

carsonbot added the Status: Needs Review label

Sep 30, 2016

Copy link

Member

nicolas-grekas commentedSep 30, 2016•
edited
Loading

Just challenging this one: the pending doc PR on this topic now says:

You can also define the default value of any existing parameters using special environment variables named after their corresponding parameter prefixed with SYMFONY__ after replacing dots by double underscores (e.g. SYMFONY__KERNEL__CHARSET to set the default value of the kernel.charset parameter). These default values are resolved when compiling the service container and won't change at runtime once dumped.

Seehttp://pr-6918-6qmocelev2lwe.eu.platform.sh/configuration/external_parameters.html#environment-variables

To me, the use case is different.env() parameters do not allow this (setting the default value of parameter at compile time through env vars).
Do we really consider this feature as something we don't want to support anymore?

Copy link

Member

nicolas-grekas commentedSep 30, 2016

If the answer is yes (see above challenge), shouldn't we deprecateEnvParametersResource andgetEnvParameters also?

Copy link

Contributor

linaori commentedSep 30, 2016

@nicolas-grekas ifSYMFONY__ indicated that it was a value only used to build parameters and stores the calculated version, maybe something likebuild_env(...) could solve the problem?

Copy link

Member

nicolas-grekas commentedSep 30, 2016•
edited
Loading

It's also a matter of who owns the relationship between the env and the config.
env() or any such alternatives are the way to go topull from the env.
SYMFONY__ topush to parameter's defaults.
Different directions, different use cases to me.

Copy link

Contributor

ro0NL commentedOct 1, 2016

It's also a matter of who owns the relationship between the env and the config.

Deprecating the magicSYMFONY__ vars brings back the ownership to the config.. imo that's a good thing.

Copy link

Member

wouterj commentedOct 1, 2016

If deprecation is coming, I don't agree with the implementation of the deprecation. I can imagine people want to keep theSYMFONY__ parameters and just instead request them withenv('SYMFONY__') (to ease migration). This will currently still result in the deprecation...

nicolas-grekas mentioned this pull request

Nov 24, 2016

[DI] Make ContainerBuilder::resolveEnvPlaceholders() able to inline the values of referenced env vars.#20618

Merged

Copy link

Member

nicolas-grekas commentedNov 24, 2016•
edited
Loading

I propose#20618 as an alternative to usingSYMFONY__ env vars for compile time env vars resolution. Which means I'm now 👍 to deprecate them, thus unlocking this PR on my side.

nicolas-grekas added this to the3.x milestone

Dec 6, 2016

nicolas-grekas reviewed

Dec 9, 2016

View reviewed changes

src/Symfony/Component/HttpKernel/Kernel.php Outdated

		$parameters =array();
		foreach ($_SERVERas$key =>$value) {
		if (0 ===strpos($key,'SYMFONY__')) {
		@trigger_error('SYMFONY__ special environment variables are deprecated as of 3.2 and will be removed in 4.0. Use instead the %env()% syntax to get the value of environment variables in configuration files.',E_USER_DEPRECATED);

Copy link

Member

nicolas-grekasDec 9, 2016

Choose a reason for hiding this comment

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

IMHO, we should either add the full $key in the message, or move the deprecation outside of the loop.

Copy link

Member

nicolas-grekas commentedDec 9, 2016

@wouterj can you think of another way? IMO, it looks sensible the way it is now.

fabpot added a commit that referenced this pull request

Dec 17, 2016

feature#20618[DI] Make ContainerBuilder::resolveEnvPlaceholders() a…

4a597bd

…ble to inline the values of referenced env vars. (nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Make ContainerBuilder::resolveEnvPlaceholders() able to inline the values of referenced env vars.| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Being able to resolve environment variables at compile time as a replacement for `SYMFONY__` special env vars, unlocking their deprecation (see#20100).Commits-------713b081 [DI] Make ContainerBuilder::resolveEnvPlaceholders() able to inline the values of referenced env vars.

xabbuh requested changes

Dec 19, 2016

View reviewed changes

src/Symfony/Component/HttpKernel/Kernel.php Outdated

		$parameters =array();
		foreach ($_SERVERas$key =>$value) {
		if (0 ===strpos($key,'SYMFONY__')) {
		@trigger_error(sprintf('The special environment variables that start with SYMFONY__ (such as "%s") are deprecated as of 3.2 and will be removed in 4.0. Use instead the %env()% syntax to get the value of environment variables in configuration files.',$key),E_USER_DEPRECATED);

Copy link

Member

xabbuhDec 19, 2016

Choose a reason for hiding this comment

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

3.3

src/Symfony/Component/HttpKernel/Tests/KernelTest.php Outdated

		$deprecationMessage ='SYMFONY__ special environment variables are deprecated as of 3.2 and will be removed in 4.0. Use instead the %env()% syntax to get the value of environment variables in configuration files.';
		$expectedDeprecations =array($deprecationMessage,$deprecationMessage);

		ErrorAssert::assertDeprecationsAreTriggered($expectedDeprecations,function ()use ($kernel,$method) {

Copy link

Member

xabbuhDec 19, 2016

Choose a reason for hiding this comment

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

You now have to use the annotation instead.

Copy link

Member

nicolas-grekas commentedJan 6, 2017

don't forget this one@javiereguiluz

Copy link

MemberAuthor

javiereguiluz commentedJan 6, 2017

@nicolas-grekas updated with the fixes suggested by reviewers.

xabbuh requested changes

Jan 6, 2017

View reviewed changes

src/Symfony/Component/HttpKernel/Kernel.php Outdated

		$parameters =array();
		foreach ($_SERVERas$key =>$value) {
		if (0 ===strpos($key,'SYMFONY__')) {
		@trigger_error(sprintf('The special environment variables that start with SYMFONY__ (such as "%s") are deprecated as of 3.3 and will be removed in 4.0. Use instead the %env()% syntax to get the value of environment variables in configuration files.',$key),E_USER_DEPRECATED);

Copy link

Member

xabbuhJan 6, 2017

Choose a reason for hiding this comment

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

I would move "instead" after "syntax".

Copy link

Contributor

ro0NLJan 6, 2017•
edited
Loading

Choose a reason for hiding this comment

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

and will be removed in 4.0

We cant really remove user env vars ^^ i'd go withand are not supported anymore in 4.0.

src/Symfony/Component/HttpKernel/Tests/KernelTest.php

		}

		/**
		* @group legacy

Copy link

Member

xabbuhJan 6, 2017

Choose a reason for hiding this comment

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

@expectedDeprecation

Copy link

Contributor

ro0NL commentedJan 6, 2017•
edited
Loading

If deprecation is coming, I don't agree with the implementation of the deprecation. I can imagine people want to keep the SYMFONY__ parameters and just instead request them with env('SYMFONY__') (to ease migration). This will currently still result in the deprecation...

This is true. Defining anSYMFONY__ env var is not deprecated, only the magic part where it's set as a parameter.

I also dont really agree on tests here.. using reflection smells.

I think roughly the path is

Remove$_SERVER implementation
Trigger deprecation if aSYMFONY__ var is defined, but not configured as a parameter, ie;

'database_name':'...'

Otherwise set the default value along with deprecation, ie;

'database_name':'%env(SYMFONY__DATABASE_NAME)%'

Should work out right?

Copy link

Member

nicolas-grekas commentedJan 6, 2017•
edited
Loading

Let's be pragmatic here: if anySYMFONY__ env var exists, it's becausewe asked for them. Which means it's OK to deprecatethe env vars. MHO.

Copy link

Contributor

ro0NL commentedJan 6, 2017

👍 ok then. Already thought something like that.. just wanted to make sure we did not overlooked it :)

HeahDude mentioned this pull request

Jan 26, 2017

[DI] Unable to resolve env()#21420

Closed

Copy link

Member

chalasr commentedJan 26, 2017

Instead of triggering the deprecation when registering the parameter, couldn't we trigger it when getting the parameter (reversing the key transformation and trigger if we find it in $_SERVER)?

Also, shouldn't theKernel::getEnvParameters() method be deprecated?