[Dotenv][WebServerBundle] Override previously loaded variables #23799

voronkovich · 2017-08-06T01:06:15Z

Q	A
Branch?	3.3
Bug fix?	yes
New feature?	no
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#23723
License	MIT

This PR implements @nicolas-grekas's idea about how we could refresh loaded environment variables. See his comment #23723 (comment)

nicolas-grekas

Almost good, just this missing:

--- a/src/Symfony/Bundle/WebServerBundle/WebServer.php
+++ b/src/Symfony/Bundle/WebServerBundle/WebServer.php
@@ -154,6 +154,11 @@ class WebServer
         $process->setWorkingDirectory($config->getDocumentRoot());
         $process->setTimeout(null);
 
+        if (in_array('APP_ENV', explode(',', getenv('SYMFONY_DOTENV_VARS')))) {
+            $process->setEnv(array('APP_ENV' => false));
+            $process->inheritEnvironmentVariables();
+        }
+
         return $process;
     }

nicolas-grekas · 2017-08-06T09:15:19Z

src/Symfony/Component/Dotenv/Dotenv.php

     * @param array $values An array of env variables
     */
    public function populate($values)
    {
+        $loadedVars = getenv('SYMFONY_DOTENV_VARS') ? explode(',', getenv('SYMFONY_DOTENV_VARS')) : array();


array_flip(explode(',', getenv('SYMFONY_DOTENV_VARS'))) (no need for the check, the resulting array is good enough)

nicolas-grekas · 2017-08-06T09:16:48Z

src/Symfony/Component/Dotenv/Dotenv.php

        foreach ($values as $name => $value) {
-            if (isset($_ENV[$name]) || isset($_SERVER[$name]) || false !== getenv($name)) {
+            if (!in_array($name, $loadedVars) && (isset($_ENV[$name]) || isset($_SERVER[$name]) || false !== getenv($name))) {


isset($loadedVars[$name]) || ...

nicolas-grekas · 2017-08-06T09:28:45Z

src/Symfony/Component/Dotenv/Dotenv.php

                continue;
            }

            putenv("$name=$value");
            $_ENV[$name] = $value;
            $_SERVER[$name] = $value;
+
+            $loadedVars[] = $name;


$loadedVars[$name] = true;

nicolas-grekas · 2017-08-06T09:30:18Z

src/Symfony/Component/Dotenv/Dotenv.php

        }
+
+        $loadedVars = implode(',', $loadedVars);


if (1 < count($loadedVars)) { ... } ("1" because of the empty string - or we should remove it before)

voronkovich · 2017-08-06T14:51:29Z

@nicolas-grekas, Thanks for the code review! I've fixed all issues you pointed out.
This PR should be merged in master branch, so I think we shouldn't use the Process::inheritEnvironmentVariables (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Process/Process.php#L1148)

nicolas-grekas · 2017-08-06T15:15:15Z

It should be merged in 3.4 to me, even maybe in 3.3 as bug fix?

voronkovich · 2017-08-06T15:23:45Z

@nicolas-grekas, It introduces a BC break:

// This code will work differently for a current version and the new one:
(new Dotenv())->populate([ 'FOO' => 'foo', 'FOO' => 'bar' ]);
// Current version echoes: "foo", new version echoes "bar"
echo getenv('FOO');

IMHO, we don't need this feature in the Symfony 3.x because it doesn't use the Dotenv component.

nicolas-grekas · 2017-08-06T15:39:01Z

Not sure to get your example, you can't define an array with two identical keys.
Moreover, Dotenv is for dev only, so we don't care about edge case behavior changes to me.

voronkovich · 2017-08-06T15:40:57Z

@nicolas-grekas, Sorry, example should be:

(new Dotenv())->populate([ 'FOO' => 'foo' ]);
(new Dotenv())->populate([ 'FOO' => 'bar' ]);
echo getenv('FOO');

nicolas-grekas · 2017-08-06T15:44:32Z

Ok :)
I still think we should propose this for 3.3 and see what others say about it.

voronkovich · 2017-08-06T18:11:19Z

@nicolas-grekas, Ok, I've changed the base branch to 3.3.

nicolas-grekas · 2017-08-06T18:16:56Z

src/Symfony/Component/Dotenv/Dotenv.php

        }
+
+        $loadedVars = implode(',', array_keys($loadedVars));


I think this block of 4 lines should be inside an if ($loadedVars) {...}

@nicolas-grekas, I think you're right. I've moved that block inside an if statement.

chalasr

3.3 LGTM

fabpot · 2017-08-18T11:49:08Z

src/Symfony/Component/Dotenv/Dotenv.php

@@ -60,20 +60,32 @@ public function load($path/*, ...$paths*/)
    /**
     * Sets values as environment variables (via putenv, $_ENV, and $_SERVER).
     *
-     * Note that existing environment variables are never overridden.
+     * Existing environment variables are never overridden, unless they are listed in the SYMFONY_DOTENV_VARS env var.


I would not document this feature as this is really just a hack to make the internal PHP web server work. Not a feature we want to promote.

@fabpot, Ok, I've removed that comment.

fabpot · 2017-08-22T13:33:53Z

Thank you @voronkovich.

@nicolas-grekas

…bles (voronkovich) This PR was squashed before being merged into the 3.3 branch (closes #23799). Discussion ---------- [Dotenv][WebServerBundle] Override previously loaded variables | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23723 | License | MIT This PR implements @nicolas-grekas's idea about how we could refresh loaded environment variables. See his comment #23723 (comment) Commits ------- c5a1218 [Dotenv][WebServerBundle] Override previously loaded variables

tiger-seo · 2017-09-01T12:47:44Z

guys, i am grateful for all your hard work, but this change is a breaking change and it is not mentioned :(

tiger-seo · 2017-09-01T14:16:19Z

to give you more info why it is a BC, please see our env_autoload.php, which is used instead of autoload.php:

<?php

/** @var \Composer\Autoload\ClassLoader $loader */

use Symfony\Component\Dotenv\Dotenv;
use Symfony\Component\Dotenv\Exception\PathException;

$loader = require __DIR__ . '/autoload.php';

$dotenv = new Dotenv;

try {
    // existing environment variables are never overridden
    $dotenv->load(__DIR__ . '/config/.env');
} catch (PathException $e) {
}

// existing environment variables are never overridden
$dotenv->load(__DIR__ . '/config/.env.dist');

return $loader;

chalasr · 2017-09-01T14:18:59Z

@tiger-seo Please open a dedicated issue so that we can look at possible alternatives. Comments on closed PR/issues are lost.

voronkovich · 2017-09-01T14:30:55Z

@tiger-seo, I think you could try to load the .env.dist earlier than the .env:

// existing environment variables are never overridden
$dotenv->load(__DIR__ . '/config/.env.dist');

try {
    // existing environment variables are never overridden
    $dotenv->load(__DIR__ . '/config/.env');
} catch (PathException $e) {
}

return $loader;

tiger-seo · 2017-09-03T16:29:06Z

@voronkovich yes, this is exactly how we've fixed it, but we spent at least 5 man-hours of our team to find(debug) and fix the issue. Symfony promises seamless upgrade for minor releases, but not this time.

voronkovich · 2017-09-03T17:05:52Z

Sorry, @tiger-seo! We thought that the BC break is so small, so nobody would notice it :)

carsonbot added Status: Needs Review Bug Feature labels Aug 6, 2017

voronkovich changed the title ~~[Dotenv] Override previosly loaded variables~~ [Dotenv] Override previously loaded variables Aug 6, 2017

voronkovich mentioned this pull request Aug 6, 2017

[WebServerBundle] Environment variables are not reloaded when the .env file was changed #23723

Closed

nicolas-grekas reviewed Aug 6, 2017

View reviewed changes

nicolas-grekas added this to the 3.4 milestone Aug 6, 2017

chalasr mentioned this pull request Aug 6, 2017

[Dotenv] Add support for overriding existing envs #23720

Closed

voronkovich changed the title ~~[Dotenv] Override previously loaded variables~~ [Dotenv][WebServerBundle] Override previously loaded variables Aug 6, 2017

Override previously loaded variables

a498945

voronkovich changed the base branch from master to 3.3 August 6, 2017 17:53

nicolas-grekas reviewed Aug 6, 2017

View reviewed changes

Move block inside 'if'

1ebc908

nicolas-grekas approved these changes Aug 6, 2017

View reviewed changes

chalasr approved these changes Aug 7, 2017

View reviewed changes

nicolas-grekas requested a review from fabpot August 8, 2017 06:51

fabpot requested changes Aug 18, 2017

View reviewed changes

voronkovich added 2 commits August 18, 2017 16:09

Remove comment about overriding existing envs

e7c7c15

Revert comment

7dc4d00

fabpot approved these changes Aug 22, 2017

View reviewed changes

chalasr added Dotenv Status: Reviewed WebServerBundle labels Aug 22, 2017

chalasr removed Status: Needs Review Feature labels Aug 22, 2017

fabpot closed this Aug 22, 2017

fabpot mentioned this pull request Aug 28, 2017

Release v3.3.7 #24013

Merged

symfony deleted a comment from voronkovich Sep 1, 2017

javiereguiluz mentioned this pull request Sep 3, 2017

[Dotenv][WebServerBundle] Possible BC break when overriding previously loaded variables #24078

Closed

kbond mentioned this pull request Jan 10, 2018

[WebServerBundle] Under certain conditions, APP_ENV is an empty string #25754

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dotenv][WebServerBundle] Override previously loaded variables #23799

[Dotenv][WebServerBundle] Override previously loaded variables #23799

voronkovich commented Aug 6, 2017 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas Aug 6, 2017

nicolas-grekas Aug 6, 2017

nicolas-grekas Aug 6, 2017

nicolas-grekas Aug 6, 2017

voronkovich commented Aug 6, 2017

nicolas-grekas commented Aug 6, 2017

voronkovich commented Aug 6, 2017

nicolas-grekas commented Aug 6, 2017

voronkovich commented Aug 6, 2017

nicolas-grekas commented Aug 6, 2017

voronkovich commented Aug 6, 2017

nicolas-grekas Aug 6, 2017

voronkovich Aug 6, 2017

chalasr left a comment

fabpot Aug 18, 2017

voronkovich Aug 18, 2017

fabpot commented Aug 22, 2017

tiger-seo commented Sep 1, 2017

tiger-seo commented Sep 1, 2017

chalasr commented Sep 1, 2017

voronkovich commented Sep 1, 2017 •

edited

Loading

tiger-seo commented Sep 3, 2017

voronkovich commented Sep 3, 2017

[Dotenv][WebServerBundle] Override previously loaded variables #23799

[Dotenv][WebServerBundle] Override previously loaded variables #23799

Conversation

voronkovich commented Aug 6, 2017 • edited Loading

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

voronkovich commented Aug 6, 2017

nicolas-grekas commented Aug 6, 2017

voronkovich commented Aug 6, 2017

nicolas-grekas commented Aug 6, 2017

voronkovich commented Aug 6, 2017

nicolas-grekas commented Aug 6, 2017

voronkovich commented Aug 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Aug 22, 2017

tiger-seo commented Sep 1, 2017

tiger-seo commented Sep 1, 2017

chalasr commented Sep 1, 2017

voronkovich commented Sep 1, 2017 • edited Loading

tiger-seo commented Sep 3, 2017

voronkovich commented Sep 3, 2017

voronkovich commented Aug 6, 2017 •

edited

Loading

voronkovich commented Sep 1, 2017 •

edited

Loading