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

Fix inconsistency with .env loaders out of the box #2860

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ostrolucky
Copy link

@ostrolucky ostrolucky commented Dec 20, 2023

Description of changes:

PHP .env loaders such as symfony/dotenv or vlucas/phpdotenv populate $_SERVER out of the box, but don't call putenv() in order to preserve thread safeness. This means .env is loaded by them, but AWS SDK does not read it, creating inconsistent experience when handling real env variables and env variables loaded by these loaders.

With this change, we are still reading via getenv() by default, but fallback to $_SERVER in case getenv() doesn't give back anything. Can be easily extended later to other global variable holders like $_ENV

@GrahamCampbell
Copy link
Contributor

As the maintainer of phpdotenv, $_SERVER is the best place to look at for env variables. PHP's naming of things is kinda confusing.

@ostrolucky
Copy link
Author

I decided to stick to $_ENV, because in this case variable is used only as fallback in cases where getenv() does not work. getenv() should work in all the cases, except the cases with .env loaders. Loaders do populate $_ENV in those cases though. Using $_ENV here is more semantically correct and we don't risk exposing something else as $_SERVER contains other garbage too, outside of env vars.

@GrahamCampbell
Copy link
Contributor

$_ENV here is more semantically correct

Not really. PHP's naming is confusing like I said. $_SERVER is the closest thing to getenv. It does contain other stuff, but $_ENV is also missing stuff!

@ostrolucky
Copy link
Author

Again, $_ENV is not missing stuff in combination with getenv(). getenv() ?: $_ENV is more semantically correct in this case than getenv() ?: $_SERVER. Your proposal would make sense only if there was no getenv() call.

PHP .env loaders such as `symfony/dotenv` or `vlucas/phpdotenv` populate `$_ENV` out of the box, but don't call `putenv()` in order to preserve thread safeness. This means .env is loaded by them, but AWS SDK does not read it, creating inconsistent experience when handling real env variables and env variables loaded by these loaders
@ostrolucky
Copy link
Author

Just to clarify why I don't just replace getenv() calls with $_SERVER:

  1. Even though unlikely, $_SERVER can be disabled through variables_order directive. getenv() cannot
  2. $_SERVER is not populated under PHP's built-in web server (and possibly other SAPIs)
  3. It would require changing lot of lines of code compared to just importing this function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants