Deprecate the special SYMFONY__ environment variables #21889

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

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	-

chalasr · 2017-03-06T18:17:14Z

I surely miss something but #20100 (comment):

shouldn't the Kernel::getEnvParameters() method be deprecated?

Or do we plan to trigger this deprecation forever?

robfrawley · 2017-03-06T18:27:18Z

Vastly prefer the implementation described in #20100 (comment). I was going to comment on this earlier, but couldn't find that comment by @chalasr.

javiereguiluz · 2017-03-12T17:31:58Z

@chalasr about your comment, I don't agree on reporting the deprecation when using the env var instead of when setting it (if you set it, it's because you have defined it ... so you should get the deprecation).

About the other proposal ... I don't know what to say ... but others think it's much better, so if you can implement it, I guess we can close this PR in favor of your PR. Thanks!

robfrawley · 2017-03-12T20:37:45Z

The reason I believe we shouldn't get any deprecations until an environment variable is actually used is because otherwise, we are restricting the use of external environment variables without knowing that they are intended for consumption by Symfony itself. People may well be using environment variables beginning with SYMFONY__ for their own container port mapping or other functionality related to setting up their Symfony environment, but external to the Symfony runtime itself.

chalasr · 2017-03-14T13:37:00Z

@javiereguiluz My point is mainly what @robfrawley said in the previous comment, but also that deprecating SYMFONY__ prefixed env vars does not make sense to me in term of upgrade.
Defining these prefixed env vars will not stop to work, only using them as parameters will.
The symfony-specific behavior is only to consider these vars differently, I think the deprecation should reflect this and that this behavior should really die in 4.0 (i.e. Kernel::getEnvParameters() be removed).
I'll propose the alternative PR.

javiereguiluz · 2017-03-14T14:04:05Z

@chalasr yes, I agree that both of you are right and your solution is better than mine :) Thanks for providing the alternative PR. I'll close this one then.

nicolas-grekas · 2017-03-14T15:48:18Z

In think this implem here is right, but misses the deprecation of Kernel::getEnvParameters
The false positive that has been identified makes no harm at all: it's mainly theoretical, and it's not like it's going to break anything (4.0 won't throw - it'll just ignore and not warn anymore).

The alternative proposed in #21997 looks worse to me because it breaks SRP.

Until someone finds another solution freed from any of the identified drawbacks, I'm 👍 here (I won't be the one working on this as the identified drawback of this PR doesn't look that realistic to me.)

chalasr · 2017-03-14T15:58:24Z

src/Symfony/Component/HttpKernel/Kernel.php

@@ -576,6 +576,7 @@ protected function getEnvParameters()
        $parameters = array();
        foreach ($_SERVER as $key => $value) {
            if (0 === strpos($key, 'SYMFONY__')) {
+                @trigger_error(sprintf('The support of special environment variables that start with SYMFONY__ (such as "%s") is deprecated as of 3.3 and will be removed in 4.0. Use the %env()% syntax instead to get the value of environment variables in configuration files.', $key), E_USER_DEPRECATED);
                $parameters[strtolower(str_replace('__', '.', substr($key, 9)))] = $value;


%env()% should be %%env()%%

javiereguiluz · 2017-03-14T15:59:39Z

For the record, I still think that comments from Rob and Robin are right ... but I'll update this PR in case we don't find a viable alternative solution.

nicolas-grekas · 2017-03-14T16:00:53Z

Reopening, this looks like the correct approach to me:

Defining these prefixed env vars will not stop to work, only using them as parameters will.

SYMFONY__ is not a common prefix. I'd better consider that we can claim triggering a deprecation notice here. In terms of DX, it's also way more important to me to warn users that if they expect a SYMFONY__ env var to be taken into account, then that won't happen in 4.0, even in the case they don't actually use it as params.

chalasr · 2017-03-14T16:07:26Z

even in the case they don't actually use it as params.

If not used as a parameter then there is no reason to expect the env var to be taken into account. I don't understand how this is a better approach regarding DX, but let's move on.

nicolas-grekas · 2017-03-14T16:11:21Z

@chalasr since these env vars are used as default values, they can very well be defined by the env just in case - but actually not being used because the param is defined / not used by the current app.

chalasr · 2017-03-14T16:31:38Z

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

@@ -735,6 +735,23 @@ public function testKernelWithoutBundles()
    }

    /**
+     * @group legacy
+     * @expectedDeprecation The Symfony\Component\HttpKernel\Kernel::getEnvParameters() method is deprecated as of 3.3 and will be removed in 4.0. Use the %s syntax to get the value of any environment variable from configuration files instead.
+     * @expectedDeprecation The support of special environment variables that start with SYMFONY__ (such as "SYMFONY__FOO__BAR") is deprecated as of 3.3 and will be removed in 4.0. Use the %env()% syntax instead to get the value of environment variables in configuration files.


The use of %env()% makes the assertion fails due to the use of assertStringMatchesFormat. For now I didn't find better than replacing %env()% by %s in the expected deprecation to solve this issue

%c is a bit better :)

chalasr · 2017-03-14T16:34:38Z

src/Symfony/Component/HttpKernel/Kernel.php

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


not great but we have to exclude the special env vars which are set from the core: SYMFONY__REDIS_HOST is one (https://travis-ci.org/symfony/symfony/jobs/211006453)

or not use it anymore :)

nicolas-grekas · 2017-03-14T16:37:35Z

missing entries in CHANGELOG / UPGRADE files

fabpot · 2017-03-14T19:21:46Z

👍

nicolas-grekas

A few comments, a rebase, and a test fix, then all good on my side.

nicolas-grekas · 2017-03-14T19:26:27Z

UPGRADE-3.3.md

@@ -181,6 +181,12 @@ HttpKernel
   which will tell the Kernel to use the response code set on the event's
   response object.

+ * The `getEnvParameters()` method has been deprecated and will be removed in 4.0.


Kernel::getEnvParameters()

nicolas-grekas · 2017-03-14T19:26:44Z

UPGRADE-4.0.md

@@ -321,6 +321,12 @@ HttpKernel
   which will tell the Kernel to use the response code set on the event's
   response object.

+ * The `getEnvParameters()` method has been removed.


Kernel::getEnvParameters()

nicolas-grekas · 2017-03-14T19:26:59Z

src/Symfony/Component/HttpKernel/CHANGELOG.md

@@ -7,6 +7,8 @@ CHANGELOG
 * Deprecated `LazyLoadingFragmentHandler::addRendererService()`
 * Added `SessionListener`
 * Added `TestSessionListener`
+ * Deprecated `getEnvParameters`


Kernel::getEnvParameters()

chalasr · 2017-03-20T14:07:42Z

src/Symfony/Bundle/FrameworkBundle/Tests/Functional/CachePoolsTest.php

@@ -19,12 +19,12 @@ class CachePoolsTest extends WebTestCase
 {
    protected function setUp()
    {
-        $_SERVER['SYMFONY__REDIS_HOST'] = getenv('REDIS_HOST');
+        $_SERVER['REDIS_HOST'] = getenv('REDIS_HOST');
    }


seems unnecessary

I wasn't sure. Can I remove both setUp() and tearDown() ?

I would say yes

fabpot · 2017-03-20T14:58:37Z

Tests are broken.

chalasr

👍

javiereguiluz · 2017-03-21T09:52:14Z

The last failing tests seem unrelated, so this is now ready! A big "thank you" to @chalasr for his great help fixing all the failing tests.

nicolas-grekas

👍

fabpot · 2017-03-21T15:27:23Z

Thank you @javiereguiluz.

The special SYMFONY__ environment variables have been deprecated in Symfony 3.3 (and will be removed in 4.0) as there is now support for runtime environment variables in Symfony. This commit allows for different environment variables to be used as the first step in removing the old variables. A later step should remove the `parameters.yml` file entirely and have the configuration be solely from passed environment variables (or .env file for development). ref: [Deprecation blog post](https://symfony.com/blog/new-in-symfony-3-3-deprecated-the-special-symfony-environment-variables) ref: symfony/symfony#21889

javiereguiluz added Deprecation HttpKernel labels Mar 6, 2017

carsonbot added the Status: Needs Review label Mar 6, 2017

javiereguiluz mentioned this pull request Mar 6, 2017

Deprecate the special SYMFONY__ environment variables #20100

Closed

nicolas-grekas added this to the 3.3 milestone Mar 6, 2017

javiereguiluz closed this Mar 14, 2017

chalasr mentioned this pull request Mar 14, 2017

Deprecate accessing environment variables prefixed by SYMFONY__ as parameters #21997

Closed

nicolas-grekas reopened this Mar 14, 2017

chalasr reviewed Mar 14, 2017

View reviewed changes

nicolas-grekas reviewed Mar 14, 2017

View reviewed changes

javiereguiluz added 3 commits March 19, 2017 12:50

Deprecate the special SYMFONY__ environment variables

2b8b6c7

Deprecated the entire getEnvParameters() method

4374128

Added UPGRADE and CHANGELOG info

924e18c

javiereguiluz force-pushed the finish_20100 branch from 3482860 to 924e18c Compare March 19, 2017 11:54

Minor improvement

1c08baa

javiereguiluz added 2 commits March 20, 2017 14:57

Fixed the tested deprecation message

a1ba61d

Don't use deprecated features in tests

247d17e

chalasr reviewed Mar 20, 2017

View reviewed changes

Removed unneeded code

9e99d7b

javiereguiluz added 3 commits March 20, 2017 16:01

Define a default value for REDIS_HOST parameter

d9e2375

Fixed another failing test config

b55f181

Unset the env var after the test is finished

3ed6d2f

chalasr approved these changes Mar 21, 2017

View reviewed changes

nicolas-grekas approved these changes Mar 21, 2017

View reviewed changes

fabpot closed this in a96a997 Mar 21, 2017

javiereguiluz mentioned this pull request Mar 26, 2017

[DI] Deprecate the special SYMFONY__ environment variables symfony/symfony-docs#7695

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

roverwolf mentioned this pull request Aug 16, 2017

refactor: deprecate special SYMFONY__ environment vars opensalt/opensalt#97

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate the special SYMFONY__ environment variables #21889

Deprecate the special SYMFONY__ environment variables #21889

javiereguiluz commented Mar 6, 2017

chalasr commented Mar 6, 2017

robfrawley commented Mar 6, 2017

javiereguiluz commented Mar 12, 2017

robfrawley commented Mar 12, 2017 •

edited

Loading

chalasr commented Mar 14, 2017

javiereguiluz commented Mar 14, 2017

nicolas-grekas commented Mar 14, 2017 •

edited

Loading

chalasr Mar 14, 2017 •

edited

Loading

javiereguiluz commented Mar 14, 2017

nicolas-grekas commented Mar 14, 2017

chalasr commented Mar 14, 2017

nicolas-grekas commented Mar 14, 2017 •

edited

Loading

chalasr Mar 14, 2017

nicolas-grekas Mar 14, 2017

chalasr Mar 14, 2017

nicolas-grekas Mar 14, 2017

nicolas-grekas commented Mar 14, 2017

fabpot commented Mar 14, 2017

nicolas-grekas left a comment

nicolas-grekas Mar 14, 2017

nicolas-grekas Mar 14, 2017

nicolas-grekas Mar 14, 2017

chalasr Mar 20, 2017

javiereguiluz Mar 20, 2017

chalasr Mar 20, 2017

fabpot commented Mar 20, 2017

chalasr left a comment

javiereguiluz commented Mar 21, 2017

nicolas-grekas left a comment

fabpot commented Mar 21, 2017

Deprecate the special SYMFONY__ environment variables #21889

Deprecate the special SYMFONY__ environment variables #21889

Conversation

javiereguiluz commented Mar 6, 2017

chalasr commented Mar 6, 2017

robfrawley commented Mar 6, 2017

javiereguiluz commented Mar 12, 2017

robfrawley commented Mar 12, 2017 • edited Loading

chalasr commented Mar 14, 2017

javiereguiluz commented Mar 14, 2017

nicolas-grekas commented Mar 14, 2017 • edited Loading

chalasr Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

javiereguiluz commented Mar 14, 2017

nicolas-grekas commented Mar 14, 2017

chalasr commented Mar 14, 2017

nicolas-grekas commented Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Mar 14, 2017

fabpot commented Mar 14, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 20, 2017

chalasr left a comment

Choose a reason for hiding this comment

javiereguiluz commented Mar 21, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Mar 21, 2017

robfrawley commented Mar 12, 2017 •

edited

Loading

nicolas-grekas commented Mar 14, 2017 •

edited

Loading

chalasr Mar 14, 2017 •

edited

Loading

nicolas-grekas commented Mar 14, 2017 •

edited

Loading