Deprecate the special SYMFONY__ environment variables #20100

javiereguiluz · 2016-09-30T07:51:22Z

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	-

nicolas-grekas · 2016-09-30T08:13:01Z

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.

See http://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?

nicolas-grekas · 2016-09-30T08:33:20Z

If the answer is yes (see above challenge), shouldn't we deprecate EnvParametersResource and getEnvParameters also?

linaori · 2016-09-30T08:34:20Z

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

nicolas-grekas · 2016-09-30T08:37:18Z

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 to pull from the env.
SYMFONY__ to push to parameter's defaults.
Different directions, different use cases to me.

ro0NL · 2016-10-01T09:07:07Z

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

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

wouterj · 2016-10-01T11:04:26Z

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...

nicolas-grekas · 2016-11-24T11:21:04Z

I propose #20618 as an alternative to using SYMFONY__ 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 · 2016-12-09T11:34:15Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -537,6 +537,7 @@ protected function getEnvParameters()
        $parameters = array();
        foreach ($_SERVER as $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);


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

nicolas-grekas · 2016-12-09T11:35:09Z

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

…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 · 2016-12-19T06:32:25Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -537,6 +537,7 @@ protected function getEnvParameters()
        $parameters = array();
        foreach ($_SERVER as $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);


xabbuh · 2016-12-19T06:33:01Z

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

+        $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) {


You now have to use the annotation instead.

nicolas-grekas · 2017-01-06T14:34:23Z

don't forget this one @javiereguiluz

javiereguiluz · 2017-01-06T16:11:50Z

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

xabbuh · 2017-01-06T17:33:52Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -537,6 +537,7 @@ protected function getEnvParameters()
        $parameters = array();
        foreach ($_SERVER as $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);


I would move "instead" after "syntax".

and will be removed in 4.0

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

xabbuh · 2017-01-06T17:34:36Z

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

@@ -719,6 +719,22 @@ public function testTerminateDelegatesTerminationOnlyForTerminableInterface()
    }

    /**
+     * @group legacy
+     * @expectedExceptionMessage The special environment variables that start with SYMFONY__ (such as "SYMFONY__FOO__BAR") 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.


@expectedDeprecation

ro0NL · 2017-01-06T17:40:55Z

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 an SYMFONY__ 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 a SYMFONY__ 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?

nicolas-grekas · 2017-01-06T17:45:38Z

Let's be pragmatic here: if any SYMFONY__ env var exists, it's because we asked for them. Which means it's OK to deprecate the env vars. MHO.

ro0NL · 2017-01-06T17:46:40Z

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

chalasr · 2017-01-26T21:43:15Z

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 the Kernel::getEnvParameters() method be deprecated?

nicolas-grekas · 2017-02-14T13:57:01Z

rebase needed - to make tests green at least

javiereguiluz · 2017-03-06T09:42:55Z

The rebase wen't wrong ... closing it in favor of #21889.

javiereguiluz added HttpKernel Deprecation labels Sep 30, 2016

carsonbot added the Status: Needs Review label Sep 30, 2016

nicolas-grekas mentioned this pull request Nov 24, 2016

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

Merged

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

nicolas-grekas reviewed Dec 9, 2016

View reviewed changes

xabbuh requested changes Dec 19, 2016

View reviewed changes

xabbuh requested changes Jan 6, 2017

View reviewed changes

HeahDude mentioned this pull request Jan 26, 2017

[DI] Unable to resolve env() #21420

Closed

nicolas-grekas mentioned this pull request Feb 14, 2017

[DI] Unable to use env() on params passed to ReplaceAliasByActualDefinitionPass #21609

Closed

javiereguiluz added 4 commits March 6, 2017 10:36

Deprecate the special SYMFONY__ environment variables

59d6330

Include the name of the env variable in the deprecation message

cea85ea

Use the expectedDeprecation annotation

4d390bd

Changed suggested by reviewers

75f847a

javiereguiluz force-pushed the fix_20089 branch from c77317c to 75f847a Compare March 6, 2017 09:38

javiereguiluz closed this Mar 6, 2017

chalasr mentioned this pull request Mar 6, 2017

Deprecate the special SYMFONY__ environment variables #21889

Closed

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate the special SYMFONY__ environment variables #20100

Deprecate the special SYMFONY__ environment variables #20100

javiereguiluz commented Sep 30, 2016

nicolas-grekas commented Sep 30, 2016 •

edited

Loading

nicolas-grekas commented Sep 30, 2016

linaori commented Sep 30, 2016

nicolas-grekas commented Sep 30, 2016 •

edited

Loading

ro0NL commented Oct 1, 2016

wouterj commented Oct 1, 2016

nicolas-grekas commented Nov 24, 2016 •

edited

Loading

nicolas-grekas Dec 9, 2016

nicolas-grekas commented Dec 9, 2016

xabbuh Dec 19, 2016

xabbuh Dec 19, 2016

nicolas-grekas commented Jan 6, 2017

javiereguiluz commented Jan 6, 2017

xabbuh Jan 6, 2017

ro0NL Jan 6, 2017 •

edited

Loading

xabbuh Jan 6, 2017

ro0NL commented Jan 6, 2017 •

edited

Loading

nicolas-grekas commented Jan 6, 2017 •

edited

Loading

ro0NL commented Jan 6, 2017

chalasr commented Jan 26, 2017

nicolas-grekas commented Feb 14, 2017

javiereguiluz commented Mar 6, 2017

Deprecate the special SYMFONY__ environment variables #20100

Deprecate the special SYMFONY__ environment variables #20100

Conversation

javiereguiluz commented Sep 30, 2016

nicolas-grekas commented Sep 30, 2016 • edited Loading

nicolas-grekas commented Sep 30, 2016

linaori commented Sep 30, 2016

nicolas-grekas commented Sep 30, 2016 • edited Loading

ro0NL commented Oct 1, 2016

wouterj commented Oct 1, 2016

nicolas-grekas commented Nov 24, 2016 • edited Loading

nicolas-grekas Dec 9, 2016

Choose a reason for hiding this comment

nicolas-grekas commented Dec 9, 2016

xabbuh Dec 19, 2016

Choose a reason for hiding this comment

xabbuh Dec 19, 2016

Choose a reason for hiding this comment

nicolas-grekas commented Jan 6, 2017

javiereguiluz commented Jan 6, 2017

xabbuh Jan 6, 2017

Choose a reason for hiding this comment

ro0NL Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

xabbuh Jan 6, 2017

Choose a reason for hiding this comment

ro0NL commented Jan 6, 2017 • edited Loading

nicolas-grekas commented Jan 6, 2017 • edited Loading

ro0NL commented Jan 6, 2017

chalasr commented Jan 26, 2017

nicolas-grekas commented Feb 14, 2017

javiereguiluz commented Mar 6, 2017

nicolas-grekas commented Sep 30, 2016 •

edited

Loading

nicolas-grekas commented Sep 30, 2016 •

edited

Loading

nicolas-grekas commented Nov 24, 2016 •

edited

Loading

ro0NL Jan 6, 2017 •

edited

Loading

ro0NL commented Jan 6, 2017 •

edited

Loading

nicolas-grekas commented Jan 6, 2017 •

edited

Loading