[WebServerBundle] fix a bug where require would not require the good file because of env #25523

Simperfit · Dec 16, 2017

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#25515
License	MIT
Doc PR

This fixes a bug with putenv that could be not working on certain version of php. (>=7.0.0).

chalasr · Dec 16, 2017

src/Symfony/Bundle/WebServerBundle/Resources/router.php

@@ -32,6 +32,10 @@

 $script = getenv('APP_FRONT_CONTROLLER') ?: 'index.php';

+if (false === getenv('APP_FRONT_CONTROLLER')) {
+    $script = $_ENV['APP_FRONT_CONTROLLER'];


Not good, it should not squash the ternary above (when the env var doesn't exist, the value should remain 'index.php'). No need for calling getenv twice also

Simperfit · Dec 16, 2017

Status: Needs Review

fabpot · Dec 16, 2017

src/Symfony/Bundle/WebServerBundle/WebServerConfig.php

@@ -33,6 +33,7 @@ public function __construct($documentRoot, $env, $address = null, $router = null
        }

        putenv('APP_FRONT_CONTROLLER='.$file);


Is it possible to just remove putenv here?

I think we should keep it because like @ostrolucky said there could be cases where $_ENV could be empty

I also think we should remove the call to putenv (but alongside with -dvariable_order)
and then, symfony/process should be bumped to symfony/process ^3.4.2 in the bundle's composer.json

ostrolucky · Dec 16, 2017

There should be some investigation regarding general usage of getenv/putenv/$_ENV/$_SERVER in Symfony code. I've seen people on slack too who complained that $_SERVER is not populated with Apache, but $_ENV is.

I have tried it now with apache and neither $_SERVER nor $_ENV is populated, but getenv() is. When env is set via .htaccess, both $_SERVER and getenv is populated, but $_ENV isn't

Then there are concurrency issues with getenv()/putenv() and now this PHP bug(?).

Other than these bugs(?), there are also several other disadvantages for env variables. Old parameters.yml seems sweeter and sweeter to me as time goes on.

Simperfit · Dec 17, 2017

Status: Needs Review

nicolas-grekas · Dec 18, 2017

src/Symfony/Bundle/WebServerBundle/Resources/router.php

-$script = getenv('APP_FRONT_CONTROLLER') ?: 'index.php';
+$script = getenv('APP_FRONT_CONTROLLER');
+
+if (isset($_ENV['APP_FRONT_CONTROLLER']) && false === $script) {


this doesn't work when "E" is not listed in variables_order, which is the default on Ubuntu
note that this file is used only with php -S so we don't care behavior on Apache
I'd suggest to add -dvariables_order=EGPCS when calling php -S, and then we can get rid of the call to getenv completely

chalasr · Dec 18, 2017

Should it target 3.3? This code did not change in 3.4 AFAIK

Simperfit · Dec 19, 2017

i'll rebase on 3.3, tell me if it's better to rebase on 3.4.

The things is the bug appear only on 3.4 but hey, it could be on 3.3 too I guess.

nicolas-grekas · Dec 19, 2017

@Simperfit you forgot to account for this comment (just replace ^3.4.2 by ^3.3.14 since you rebased on 3.3):

symfony/process should be bumped to symfony/process ^3.4.2 in the bundle's composer.json

…file because of env

Simperfit · Dec 20, 2017

done @nicolas-grekas

nicolas-grekas · Dec 20, 2017

@Simperfit not quite: ^3.3.14 != ~3.3.14 ;)

nicolas-grekas · Dec 20, 2017

Thank you @Simperfit.

…e the good file because of env (Simperfit) This PR was merged into the 3.3 branch. Discussion ---------- [WebServerBundle] fix a bug where require would not require the good file because of env | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | #25515 | License | MIT | Doc PR | This fixes a bug with putenv that could be not working on certain version of php. (>=7.0.0). Commits ------- bfeee1f [WebServerBundle] fix a bug where require would not require the good file because of env

stof · Dec 20, 2017

src/Symfony/Bundle/WebServerBundle/composer.json

@@ -19,7 +19,7 @@
        "php": "^5.5.9|>=7.0.8",
        "symfony/console": "~3.3",
        "symfony/http-kernel": "~3.3",
-        "symfony/process": "~3.3"
+        "symfony/process": "~3.3.14"


this now forbids using 3.4, so not good. Should be ~3.3.14 || ^3.4.2

was patched while merged, see 743be09

carsonbot added Status: Needs Review Bug labels Dec 16, 2017

Simperfit force-pushed the bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from 05888ce to cdd1c58 Compare December 16, 2017 17:02

carsonbot removed the Status: Needs Review label Dec 16, 2017

chalasr requested changes Dec 16, 2017

View reviewed changes

carsonbot added the Status: Needs Work label Dec 16, 2017

Simperfit force-pushed the bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from cdd1c58 to cd1dafa Compare December 16, 2017 18:03

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 16, 2017

fabpot requested changes Dec 16, 2017

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 16, 2017

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 17, 2017

Simperfit mentioned this pull request Dec 17, 2017

[WebServerBundle] server not work after update #25515

Closed

nicolas-grekas reviewed Dec 18, 2017

View reviewed changes

nicolas-grekas added this to the 3.4 milestone Dec 18, 2017

Simperfit force-pushed the bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch 2 times, most recently from 2bbab0f to 4fdf6d8 Compare December 19, 2017 07:29

Simperfit force-pushed the bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from 4fdf6d8 to 7f69bf8 Compare December 19, 2017 07:36

Simperfit changed the base branch from 3.4 to 3.3 December 19, 2017 07:37

Simperfit force-pushed the bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from 7f69bf8 to 9352603 Compare December 19, 2017 07:37

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 19, 2017

nicolas-grekas approved these changes Dec 19, 2017

View reviewed changes

nicolas-grekas modified the milestones: 3.4, 3.3 Dec 19, 2017

Simperfit force-pushed the bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from 9352603 to 2a878a4 Compare December 19, 2017 13:08

[WebServerBundle] fix a bug where require would not require the good …

bfeee1f

…file because of env

Simperfit force-pushed the bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from 2a878a4 to bfeee1f Compare December 19, 2017 13:09

chalasr approved these changes Dec 20, 2017

View reviewed changes

nicolas-grekas merged commit bfeee1f into symfony:3.3 Dec 20, 2017

Simperfit deleted the bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch December 20, 2017 14:42

stof reviewed Dec 20, 2017

View reviewed changes

nicolas-grekas mentioned this pull request Dec 21, 2017

env var APP_FRONT_CONTROLLER empty after upgrading from 3.3 to 3.4 (the same issue is on fresh 3.4 installation) #25575

Closed

This was referenced Jan 5, 2018

Release v3.3.15 #25690

Merged

Release v3.4.3 #25691

Merged

Release v4.0.3 #25692

Merged

ostrolucky mentioned this pull request Jan 6, 2018

[RFC][Dotenv] Add a convenient way to retrieve env vars #25693

Closed

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

Uh oh!

[WebServerBundle] fix a bug where require would not require the good file because of env #25523

[WebServerBundle] fix a bug where require would not require the good file because of env #25523

Uh oh!

Conversation

Simperfit commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chalasr Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

Simperfit Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Dec 16, 2017

Uh oh!

fabpot Dec 16, 2017

Choose a reason for hiding this comment

Uh oh!

Simperfit Dec 17, 2017

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

ostrolucky commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simperfit commented Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commented Dec 18, 2017

Uh oh!

Simperfit commented Dec 19, 2017

Uh oh!

nicolas-grekas commented Dec 19, 2017

Uh oh!

Simperfit commented Dec 20, 2017

Uh oh!

nicolas-grekas commented Dec 20, 2017

Uh oh!

nicolas-grekas commented Dec 20, 2017

Uh oh!

stof Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Simperfit commented Dec 16, 2017 •

edited

Loading

ostrolucky commented Dec 16, 2017 •

edited

Loading

Simperfit commented Dec 17, 2017 •

edited

Loading

nicolas-grekas Dec 18, 2017 •

edited

Loading

stof Dec 20, 2017 •

edited

Loading