NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:3.3fromSimperfit:bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working

Dec 20, 2017

Merged

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

nicolas-grekas merged 1 commit intosymfony:3.3fromSimperfit:bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working

Dec 20, 2017

Conversation

Copy link

Contributor

Simperfit commentedDec 16, 2017•
edited
Loading

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).

carsonbot added Status: Needs Review Bug labels

Dec 16, 2017

Simperfit force-pushed thebugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from05888ce tocdd1c58Compare

December 16, 2017 17:02

carsonbot removed the Status: Needs Review label

Dec 16, 2017

chalasr requested changes

Dec 16, 2017

View reviewed changes

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

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

changed

carsonbot added the Status: Needs Work label

Dec 16, 2017

Simperfit force-pushed thebugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch fromcdd1c58 tocd1dafaCompare

December 16, 2017 18:03

Copy link

ContributorAuthor

Simperfit commentedDec 16, 2017

Status: Needs Review

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

Dec 16, 2017

fabpot requested changes

Dec 16, 2017

View reviewed changes

src/Symfony/Bundle/WebServerBundle/WebServerConfig.php Outdated

		thrownew \InvalidArgumentException(sprintf('Unable to find the front controller under "%s" (none of these files exist: %s).',$documentRoot,implode(',',$this->getFrontControllerFileNames($env))));
		}

		putenv('APP_FRONT_CONTROLLER='.$file);

Copy link

Member

Choose a reason for hiding this comment

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

Is it possible to just removeputenv here?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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

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

Dec 16, 2017

Copy link

Contributor

ostrolucky commentedDec 16, 2017•
edited
Loading

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.

Copy link

ContributorAuthor

Simperfit commentedDec 17, 2017•
edited
Loading

Status: Needs Review

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

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

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

		if (isset($_ENV['APP_FRONT_CONTROLLER']) &&false ===$script) {

Copy link

Member

nicolas-grekasDec 18, 2017•
edited
Loading

Choose a reason for hiding this comment

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

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

nicolas-grekas added this to the3.4 milestone

Dec 18, 2017

Copy link

Member

chalasr commentedDec 18, 2017

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

Simperfit force-pushed thebugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch 2 times, most recently from2bbab0f to4fdf6d8Compare

December 19, 2017 07:29

Copy link

ContributorAuthor

Simperfit commentedDec 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.

Simperfit force-pushed thebugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from4fdf6d8 to7f69bf8Compare

December 19, 2017 07:36

Simperfit changed the base branch from3.4 to3.3

December 19, 2017 07:37

Simperfit force-pushed thebugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from7f69bf8 to9352603Compare

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

Copy link

Member

nicolas-grekas commentedDec 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

nicolas-grekas modified the milestones:3.4,3.3

Dec 19, 2017

Simperfit force-pushed thebugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from9352603 to2a878a4Compare

December 19, 2017 13:08

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

bfeee1f

…file because of env

Simperfit force-pushed thebugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch from2a878a4 tobfeee1fCompare

December 19, 2017 13:09

Copy link

ContributorAuthor

Simperfit commentedDec 20, 2017

done@nicolas-grekas

chalasr approved these changes

Dec 20, 2017

View reviewed changes

Copy link

Member

nicolas-grekas commentedDec 20, 2017

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

Copy link

Member

nicolas-grekas commentedDec 20, 2017

Thank you@Simperfit.

nicolas-grekas merged commitbfeee1f intosymfony:3.3

Dec 20, 2017

nicolas-grekas added a commit that referenced this pull request

Dec 20, 2017

bug#25523[WebServerBundle] fix a bug where require would not requir…

743be09

…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 <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | no <!-- don't forget to update UPGRADE-*.md files -->| 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

Simperfit deleted the bugfix/fix-a-bug-with-web-server-bundle-where-putenv-is-not-working branch

December 20, 2017 14:42