Movatterモバイル変換

fmata mentioned this pull request

[Dotenv] Tell component to override#26846

Closed

natebrunette suggested changes

src/Symfony/Component/Dotenv/Dotenv.php Outdated

		private$values;
		private$enableOverride;

		publicfunction__construct(bool$enableOverride =false)

Copy link

natebrunetteApr 7, 2018

Choose a reason for hiding this comment

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

Maybe a docblock specifying what the parameter is for.

Copy link

ContributorAuthor

fmataApr 7, 2018

Choose a reason for hiding this comment

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

docblock added.

src/Symfony/Component/Dotenv/Dotenv.php Outdated

		$notHttpName =0 !==strpos($name,'HTTP_');
		// don't check existence with getenv() because of thread safety issues
		if (!isset($loadedVars[$name]) && (isset($_ENV[$name]) \|\| (isset($_SERVER[$name]) &&$notHttpName))) {
		if (!isset($loadedVars[$name]) && ((!$this->enableOverride &&isset($_ENV[$name])) \|\| (isset($_SERVER[$name]) &&$notHttpName))) {

Copy link

natebrunetteApr 7, 2018

Choose a reason for hiding this comment

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

Shouldn't this be used for allisset checks?

if (!isset($loadedVars[$name]) && (!$this->enableOverride && (isset($_ENV[$name]) || (isset($_SERVER[$name]) &&$notHttpName)))) {

Additionally, I prefer an explicit=== false check for readability, but I'm not sure what the convention is.

Copy link

ContributorAuthor

fmataApr 7, 2018

Choose a reason for hiding this comment

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

For security reasons I think it's better to not allow overriding$_SERVER['HTTP_*'] values, a previous discussion took place before the creation of this component. Maybe a core team member can confirm this later.

About CS style, there are plenty of examples in the codebase (and in this file too) where shortcut is used. Personaly I think it's very strange to explicitly test a false value against a boolean return type :)

Copy link

Contributor

TaluuApr 8, 2018

Choose a reason for hiding this comment

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

The!$notHttpName already fulfills the stop condition to not overwrite the http server vars. So I think it's safe to include your check on the$_SERVER ?

Copy link

ContributorAuthor

fmataApr 8, 2018

Choose a reason for hiding this comment

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

Updated

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Document the DotEnv::overload() methodsymfony/symfony-docs#9568

fmata mentioned this pull request

Apr 8, 2018

Merged

Copy link

ContributorAuthor

fmata commentedApr 8, 2018

Status: Needs Review

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Apr 8, 2018

Taluu reviewed

Apr 8, 2018

src/Symfony/Component/Dotenv/Tests/DotenvTest.php

Copy link

Contributor

Taluu left a comment

Choose a reason for hiding this comment

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

Not sure that this will make it to 4.1, as it's a feature freeze, unless it can be made as an exception ?

src/Symfony/Component/Dotenv/Dotenv.php Outdated

		$notHttpName =0 !==strpos($name,'HTTP_');
		// don't check existence with getenv() because of thread safety issues
		if (!isset($loadedVars[$name]) && (isset($_ENV[$name]) \|\| (isset($_SERVER[$name]) &&$notHttpName))) {
		if (!isset($loadedVars[$name]) && ((!$this->enableOverride &&isset($_ENV[$name])) \|\| (isset($_SERVER[$name]) &&$notHttpName))) {

Copy link

Contributor

TaluuApr 8, 2018

Choose a reason for hiding this comment

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

The!$notHttpName already fulfills the stop condition to not overwrite the http server vars. So I think it's safe to include your check on the$_SERVER ?

		$dotenv =newDotEnv(true);
		$dotenv->populate(array('TEST_ENV_VAR_OVERRIDEN' =>'new_value'));

		$this->assertSame('new_value',getenv('TEST_ENV_VAR_OVERRIDEN'));

Copy link

Contributor

TaluuApr 8, 2018

Choose a reason for hiding this comment

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

Add some asserts for the_ENV,_SERVER vars too ?

Copy link

ContributorAuthor

fmataApr 8, 2018

Choose a reason for hiding this comment

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

Asserts added.

Copy link

natebrunetteApr 9, 2018

Choose a reason for hiding this comment

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

Don't you want to test they're getting properly overridden from$_SERVER and$_ENV?

Copy link

natebrunetteApr 16, 2018

Choose a reason for hiding this comment

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

@fmata

nicolas-grekas added this to the4.1 milestone

Apr 9, 2018

Tobion approved these changes

Apr 26, 2018

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 26, 2018

fabpot modified the milestones:4.1,next

May 7, 2018

nicolas-grekas reviewed

Jul 7, 2018

src/Symfony/Component/Dotenv/CHANGELOG.md Outdated

		CHANGELOG
		=========

		4.1.0

Copy link

Member

nicolas-grekasJul 7, 2018

Choose a reason for hiding this comment

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

should be 4.2.0 now

src/Symfony/Component/Dotenv/Dotenv.php Outdated

		private$enableOverride;

		/**
		* @param bool $enableOverride true if you want to allow env vars overriding

Copy link

Member

nicolas-grekasJul 7, 2018

Choose a reason for hiding this comment

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

I'd propose to remove this comment and name the option$overrideExistingVars (default false) instead.

Copy link

natebrunetteJul 7, 2018•
edited
Loading

Choose a reason for hiding this comment

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

~~That would be a BC break~~ Nevermind, I see the name change switches the functionality. That change would of course require a logic change as well, but I’m for it 👍

Copy link

Member

nicolas-grekasJul 7, 2018

Choose a reason for hiding this comment

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

Why so? That's adding an optional argument on the constructor, which doesn't break BC usually.

Copy link

natebrunetteJul 7, 2018

Choose a reason for hiding this comment

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

Already edited 😀

Copy link

ContributorAuthor

fmata commentedJul 9, 2018

@nicolas-grekas rebased & comments addressed.

nicolas-grekas approved these changes

Jul 9, 2018

Copy link

Member

javiereguiluz commentedJul 12, 2018

I was looking for how did other similar libraries solved this:

PHPvlucas/phpdotenv defines two methods:load() (doesn't overwrite) andoverload() (overwrites)https://github.com/vlucas/phpdotenv#immutability
Rubybkeepers/dotenv defines two methods:load andoverloadhttps://github.com/bkeepers/dotenv#why-is-it-not-overriding-existing-env-variables
NodeJSmotdotla/dotenv doesn't allow to override in any way. You must do it yourself.https://github.com/motdotla/dotenv#what-happens-to-environment-variables-that-were-already-set
.NETtonerdo/dotnet-env overwrites by default, but you can pass a fourth boolean argument to prevent that:DotNetEnv.Env.Load(false, false, false, false)https://github.com/tonerdo/dotnet-env
Pythontheskumar/python-dotenv allows overriding passing an argument to load method:load_dotenv(find_dotenv(), override=True)https://github.com/theskumar/python-dotenv/pull/63/files#diff-88b99bb28683bd5b7e3a204826ead112
Gojoho/godotenv defines two methodsLoad andOverloadhttps://github.com/joho/godotenv/blob/1709ab122c988931ad53508747b3c061400c2984/godotenv.go#L64
Haskellstackbuilders/dotenv-hs defines aoverride option in theload methodhttps://github.com/stackbuilders/dotenv-hs/blob/92ca78c14f0423c91a5897e761d81378aca3f2a0/app/Main.hs#L26

The most popular option seems to be to define two methods calledload() andoverload(). Maybe we can do the same here?

Copy link

Member

nicolas-grekas commentedAug 7, 2018

Lets' addoverload() then?

Copy link

Member

fabpot commentedAug 28, 2018

I would also prefer to add another method.

[Dotenv] add Dotenv::overload() to allow env vars override

228b220

Copy link

ContributorAuthor

fmata commentedSep 2, 2018

I addedDotenv::overload() method but I had to add an optional parameter inDotenv::populate() too because this method is public (I don't know why).

Tests failed are unrelated.

nicolas-grekas reviewed

Sep 3, 2018

src/Symfony/Component/Dotenv/Dotenv.php

		* @param bool $overrideExistingVars true when existing environment variables must be overridden
		*/
		publicfunctionpopulate(array$values):void
		publicfunctionpopulate(array$values,bool$overrideExistingVars =false):void

Copy link

Member

nicolas-grekasSep 3, 2018

Choose a reason for hiding this comment

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

This should be reverted IMHO: it makes no sense to provide two public interfaces to do the same thing.

Copy link

ContributorAuthor

fmataSep 3, 2018

Choose a reason for hiding this comment

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

I agree but I think the same thing ofpopulate() method, I don't know why it is public because the purpose ofDotenv is to use a file, no directly an array of vars it's so low level IMHO. So I added here to be consistent but let me know if you have no doubt about this revert, I can do it :)

fabpot approved these changes

Copy link

Member

fabpot commentedSep 4, 2018

Thank you@fmata.

fabpot merged commit228b220 intosymfony:master

fabpot added a commit that referenced this pull request

feature#26859[Dotenv] add a flag to allow env vars override (fmata)

8f5229f

This PR was merged into the 4.2-dev branch.Discussion----------[Dotenv] add a flag to allow env vars override| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#26846| License       | MIT| Doc PR        |symfony/symfony-docs#9568I choose to use a new parameter in the constructor instead of `populate()` to not add boilerplate code to them who want allow overriding in their current setup. It's just a parameter to add in `Dotenv` creation instead of change or customize the loading of different .env files.I targeted 4.1 despite the feature freeze because it's a small change but if you don't agree I can change to 4.2.~~If you accept this PR I will do the doc PR then.~~doc readyCommits-------228b220 [Dotenv] add Dotenv::overload() to allow env vars override

stof reviewed

src/Symfony/Component/Dotenv/Dotenv.php

		if (!is_readable($path) \|\|is_dir($path)) {
		thrownewPathException($path);
		}
		$this->doLoad(false,$path, ...$paths);

Copy link

Member

stofSep 4, 2018

Choose a reason for hiding this comment

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

instead of creating a PHP array (with the variadic argument) to spread it again (here) to create another PHP array (variadic argument in the private method), it might make sense to pass an array to the private method instead of making it variadic.

Copy link

Member

fabpotSep 4, 2018

Choose a reason for hiding this comment

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

Indeed, we don't need a variadic argument in the private method.

Copy link

ContributorAuthor

fmataSep 4, 2018

Choose a reason for hiding this comment

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

@stof &@fabpot new PR#28359 created :)

fmata mentioned this pull request

[Dotenv] use array instead of variadic in Dotenv::doLoad()#28359

Merged

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request