Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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

Closed
voronkovich wants to merge4 commits intosymfony:3.3fromvoronkovich:dotenv-override-previousy-loaded
Closed

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

voronkovich wants to merge4 commits intosymfony:3.3fromvoronkovich:dotenv-override-previousy-loaded

Conversation

@voronkovich
Copy link
Contributor

@voronkovichvoronkovich commentedAug 6, 2017•
edited
Loading

QA
Branch?3.3
Bug fix?yes
New feature?no
BC breaks?yes
Deprecations?no
Tests pass?yes
Fixed tickets#23723
LicenseMIT

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

@voronkovichvoronkovich changed the title[Dotenv] Override previosly loaded variables[Dotenv] Override previously loaded variablesAug 6, 2017
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

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;     }

*/
publicfunctionpopulate($values)
{
$loadedVars =getenv('SYMFONY_DOTENV_VARS') ?explode(',',getenv('SYMFONY_DOTENV_VARS')) :array();

Choose a reason for hiding this comment

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

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


foreach ($valuesas$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))) {

Choose a reason for hiding this comment

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

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

$_ENV[$name] =$value;
$_SERVER[$name] =$value;

$loadedVars[] =$name;

Choose a reason for hiding this comment

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

$loadedVars[$name] = true;

$loadedVars[] =$name;
}

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

Choose a reason for hiding this comment

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

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

@nicolas-grekasnicolas-grekas added this to the3.4 milestoneAug 6, 2017
@voronkovichvoronkovich changed the title[Dotenv] Override previously loaded variables[Dotenv][WebServerBundle] Override previously loaded variablesAug 6, 2017
@voronkovich
Copy link
ContributorAuthor

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

@nicolas-grekas
Copy link
Member

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

@voronkovich
Copy link
ContributorAuthor

@nicolas-grekas, It introduces a BC break:

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

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

@nicolas-grekas
Copy link
Member

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
Copy link
ContributorAuthor

@nicolas-grekas, Sorry, example should be:

(newDotenv())->populate(['FOO' =>'foo' ]);(newDotenv())->populate(['FOO' =>'bar' ]);echogetenv('FOO');

@nicolas-grekas
Copy link
Member

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

@voronkovichvoronkovich changed the base branch frommaster to3.3August 6, 2017 17:53
@voronkovich
Copy link
ContributorAuthor

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

$loadedVars[$name] =true;
}

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

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

3.3 LGTM

* Sets values as environment variables (via putenv, $_ENV, and $_SERVER).
*
*Note that existingenvironment variables are never overridden.
*Existingenvironment variables are never overridden, unless they are listed in the SYMFONY_DOTENV_VARS env var.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

@fabpot
Copy link
Member

Thank you@voronkovich.

fabpot added a commit that referenced this pull requestAug 22, 2017
…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       | MITThis 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
@fabpotfabpot closed thisAug 22, 2017
@fabpotfabpot mentioned this pull requestAug 28, 2017
@tiger-seo
Copy link

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

@symfonysymfony deleted a comment fromvoronkovichSep 1, 2017
@tiger-seo
Copy link

to give you more info why it is a BC, please see ourenv_autoload.php, which is used instead ofautoload.php:

<?php/** @var \Composer\Autoload\ClassLoader $loader */useSymfony\Component\Dotenv\Dotenv;useSymfony\Component\Dotenv\Exception\PathException;$loader =require__DIR__ .'/autoload.php';$dotenv =newDotenv;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
Copy link
Member

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

@voronkovich
Copy link
ContributorAuthor

voronkovich commentedSep 1, 2017•
edited
Loading

@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
Copy link

@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
Copy link
ContributorAuthor

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

6 participants

@voronkovich@nicolas-grekas@fabpot@tiger-seo@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp