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

[5.8] Only use $_SERVER for env variables #27462

Merged
merged 2 commits into from Feb 11, 2019
Merged

[5.8] Only use $_SERVER for env variables #27462

merged 2 commits into from Feb 11, 2019

Conversation

@GrahamCampbell
Copy link
Member

@GrahamCampbell GrahamCampbell commented Feb 8, 2019

This change:

  1. Tells phpdotenv to only write to $_SERVER and to only read from $_SERVER for resolving nested environment variables.
  2. Changes Laravel's env function to only look at $_SERVER
  3. Stops Laravel's env function stripping quotes.

Why?

  1. In Local development, people are still confused when other websites are interfering with the environment variables. Moreover, the interference is both way, so Laravel variables are leaking out. By instead only reading and writing to the server superglobal, and avoiding putenv and getenv, we avoid this issue.
  2. As mentioned in the first point, getenv is also not threadsafe, so variables may leak into Laravel apps even though we're not calling putenv ourselves. The only other consequence of this change I envisage will be case sensitivity of variable resolution, however I don't envisage this being an issue, as I think most people will be getting their case correct when they do the calls, since nearly everyone uses uppercase, and is used to typing it.
  3. phpdotenv already natively handles quoted variables, so I don't see the need to strip quotes off again. This is only going to be annoying if you'd escaped the quotes in the env file, then laravel strips then anyway.
case 'false':
case '(false)':
return false;
case 'empty':
Copy link
Member Author

@GrahamCampbell GrahamCampbell Feb 8, 2019

I am tempted to also remove handling for empty, since you can already do FOO="" or just FOO=, and phpdotenv will treat it as the empty string.

I think that removing the "empty" will be an useless BC.

@barryvdh
Copy link
Contributor

@barryvdh barryvdh commented Feb 10, 2019

Do we need to write to $_SERVER at all? Can't we just use it for reading only and combine them with the .env values? In production it's also not written to the super globals, right?

@michaeldyrynda
Copy link
Contributor

@michaeldyrynda michaeldyrynda commented Feb 10, 2019

phpdotenv is made for development environments, and generally should not be used in production. In production, the actual environment variables should be set so that there is no overhead of loading the .env file on each request.

I know we have the ability to cache the contents of the .env file so the production thing is sort of moot, but will this change affect people storing config in actual environment variables, rather than the file in production?

If we no longer use $_ENV, that is? Or do proper environment values end up in $_SERVER, as well?

@GrahamCampbell
Copy link
Member Author

@GrahamCampbell GrahamCampbell commented Feb 10, 2019

phpdotenv is made for development environments, and generally should not be used in production. In production, the actual environment variables should be set so that there is no overhead of loading the .env file on each request.

This is no longer the case for v3. I should probably update the readme.

Do we need to write to $_SERVER at all? Can't we just use it for reading only and combine them with the .env values?

Yes, actually we could do that instead if we wanted (https://github.com/vlucas/phpdotenv/blob/master/src/Environment/Adapter/ArrayAdapter.php). The side effect would be that nested env variables would break though, if they were loaded outside of the env file. We'd also need to find a good way to share this adapter instance.

If we no longer use $_ENV, that is?

That was actually never used, and is kinda badly named. The sever superglobal behaves as you might expect env to.

Or do proper environment values end up in $_SERVER, as well?

Yes, always, and not always in the env supergloal.

In production it's also not written to the super globals, right?

Yeh, so no changes there. :)

@michaeldyrynda
Copy link
Contributor

@michaeldyrynda michaeldyrynda commented Feb 10, 2019

Rock on 🤘🏻

@taylorotwell taylorotwell merged commit 4bd1d9c into master Feb 11, 2019
4 checks passed
@driesvints driesvints deleted the env-changes branch Feb 11, 2019
crynobone added a commit to crynobone/laravel that referenced this issue Feb 13, 2019
Laravel 5.8 limits dotenv to only rely on $_SERVER and not $_ENV. See laravel/framework#27462
@driesvints
Copy link
Member

@driesvints driesvints commented Feb 14, 2019

@GrahamCampbell can you send in the same PR to lumen-framework?

@GrahamCampbell
Copy link
Member Author

@GrahamCampbell GrahamCampbell commented Feb 14, 2019

Yes, of course. :)

@GrahamCampbell
Copy link
Member Author

@GrahamCampbell GrahamCampbell commented Feb 14, 2019

Is a 5.8 branch getting split off there soon also?

@GrahamCampbell
Copy link
Member Author

@GrahamCampbell GrahamCampbell commented Feb 14, 2019

@driesvints
Copy link
Member

@driesvints driesvints commented Feb 14, 2019

Is a 5.8 branch getting split off there soon also?

We're going to do this after the framework release.

@GertjanRoke
Copy link

@GertjanRoke GertjanRoke commented Mar 5, 2019

@GrahamCampbell or @driesvints is this a breaking change for when you upgrade from 5.7 to 5.8? If so we maybe also need to update the "Upgrade Guide" for the docs to tell people to change the phpunit.xml file

@GrahamCampbell
Copy link
Member Author

@GrahamCampbell GrahamCampbell commented Mar 6, 2019

phpunit config does not need changing actually.

@GrahamCampbell
Copy link
Member Author

@GrahamCampbell GrahamCampbell commented Mar 6, 2019

See #27519.

@dabernathy89
Copy link

@dabernathy89 dabernathy89 commented Mar 12, 2019

We had to update our phpunit.xml file to add force="true" on our environment variables. I can't say with 100% confidence it's related to this change, but perhaps someone else will see this and want to give it a try.

Gomasy added a commit to Gomasy/books-manager that referenced this issue Sep 17, 2019
Laravel 5.8 limits dotenv to only rely on $_SERVER and not $_ENV. See laravel/framework#27462
Gomasy added a commit to Gomasy/books-manager that referenced this issue Sep 17, 2019
* Target Laravel 5.8

* Update composer.json

* Update `RegisterController` password validation rule and associated lang file

* Update UserFactory password in line with #4794

The new password is of 8 characters, as required by #4794

* Update VerificationController.php

* add dynamo to stubs

* tweak wording

* Modify RedirectIfAuthenticated middleware to accept multiple guards

* add env variable for mysql ssl cert

* Add beanstalk queue block_for config key

This functionality was added in laravel/framework 9aa1706.

* Hint for lenient log stacks

* adjust name of configuration value

* Use same version as framework

* Use $_SERVER instead of $_ENV for phpunit.

Laravel 5.8 limits dotenv to only rely on $_SERVER and not $_ENV. See laravel/framework#27462

* change default redis configuration structure

* update client

* update config file

* [5.8] use bigIncrements by default

All new migrations will be using bigIncrements
laravel/framework#26472

* Revert "[5.8] Modify RedirectIfAuthenticated middleware to accept multiple guards"

* revert to old redis config

* add postmark token

* Add Arr and Str aliases by default

* add postmark

* update env variable stubs

* set default region

* add bucket to env example

* Use correct env name for AWS region from env.example

* comment

* comment out options

* check if extension loaded

* Ignore SQLite journals

* Prefix redis database connection by default to mitigate multiple sites on the same server potentially sharing the same queued jobs

* Use Str class instead of helper function

* Additional underscore on redis database prefix

* Additional underscore on cache prefix

* Remove underscore as cache prefixes automatically have a colon appended to them

* Update UserFactory.php

* Fix phpdoc to order by syntax convention (#5005)

Reorder the `@var` phpdoc syntax by convention, see http://docs.phpdoc.org/references/phpdoc/tags/var.html

* Update database config relating to Url addition.

* formatting

* Add ends_with validation message

* Fix type hint for case of trusting all proxies (string) (#5025)

* Add DYNAMODB_ENDPOINT to the cache config (#5034)

This adds the DYNAMODB_ENDPOINT environment variable to the
dynamodb store of the cache cofig.

Its usage is implemented in the framework as laravel/framework#28600

* Added support for new redis URL property in config/database.php (#5037)

Regarding laravel/framework#28612

* use generic default db config

* Update .gitignore (#5046)

* Move TrustProxies to highest priority - fixes maintenance mode ip whitelist if behind proxy e.g. Cloudflare (#5055)

* update deprecated pusher option (#5058)

* Using environment variable to set redis prefix (#5062)

It was the only redis setting that wasn't overridable by an environment variable. It can help if you have multiple instances using the same `APP_NAME`, e.g. a staging instance

* Remove Stripe config settings

These now ship with a dedicated config file for Cashier.

* formatting

* Update composer.lock

* Revert "[5.8] use bigIncrements by default"

This reverts commit 6c5798e.

* Update laravel/telescope
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants