Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dotenv] Prevent empty $_ENV superglobal #31505

Open
wants to merge 2 commits into
base: 4.3
from

Conversation

Projects
None yet
4 participants
@stucki
Copy link

commented May 15, 2019

Q A
Branch? master, 4.3, 4.2, ..., 3.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31504
License MIT
Doc PR n/A

Prevent that $_ENV is incomplete if variables_order does not contain "E".

@stucki stucki requested review from dunglas, lyrixx and sroze as code owners May 15, 2019

@stucki stucki changed the base branch from master to 4.3 May 15, 2019

@stucki stucki force-pushed the stucki:dotenv-prevent-empty-env-superglobal branch from a71b833 to 0375b4a May 15, 2019

@@ -132,12 +132,16 @@ public function overload(string $path, string ...$extraPaths): void
public function populate(array $values, bool $overrideExistingVars = false): void
{
$updateLoadedVars = false;
$alwaysSetEnvSuperglobal = (false === strpos(ini_get('variables_order'), 'E')) ? true : false;

This comment has been minimized.

Copy link
@Simperfit

Simperfit May 15, 2019

Contributor

$alwaysSetEnvSuperGlobal maybe ?

This comment has been minimized.

Copy link
@stucki

stucki May 15, 2019

Author

I took over the spelling from https://github.com/symfony/symfony/blame/0375b4aed9eb00901222dfcc53112786bb9fcfad/src/Symfony/Component/Dotenv/Tests/DotenvTest.php#L341. If you want then I can change all of them, but I don't think it's needed here...

@nicolas-grekas nicolas-grekas added this to the next milestone May 20, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Hi, thanks for the PR.
I'm 👎 here: this would make dotenv-loaded vars have different behavior than others.
One should fix their configuration instead.

@stucki

This comment has been minimized.

Copy link
Author

commented May 20, 2019

@nicolas-grekas That implies that my configuration is wrong, which is not the case IMO. Otherwise the documentation should mention that variables_order = EGPCS must be set in php.ini.

this would make dotenv-loaded vars have different behavior than others.

That is already the case now: dotenv-loaded vars are set if they are missing in $_SERVER, but otherwise they are not. See #31504 for a full description (compare behaviour of VAR1 vs. VAR2 in the example).

Should I add a test for this scenario and how else do you propose to fix that?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 20, 2019

A configuration that doesn't enable E is wrong for an app that uses $_ENV for reading such values IMHO. This change adds a new global side-effect in a condition. That's why I don't think it's a good idea.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Another way to "fix" the linked issue would be to add vars loaded from .env files to $_ENV only if E is listed in the gpc config option.
But honestly, this wouldn't provide many benefits to me. One should not rely on the exact setting of gpc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.