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

Fixes #601 - .env.local failed to open stream php warning #603

Merged
merged 1 commit into from
Jul 30, 2021

Conversation

fab120
Copy link
Contributor

@fab120 fab120 commented Jul 30, 2021

For reference: b2781b8#commitcomment-54196503

I also removed this if
https://github.com/roots/bedrock/blob/master/config/application.php#L33

If .env file is not present a \Dotenv\Exception\InvalidPathException will be raised.

Fixes #601

@swalkinshaw
Copy link
Member

My only question is if we really want to remove the file existence check. I think that was originally done to allow more flexibility with actual env vars. .env was chosen because it's portable and doesn't have to be used. You can inject env vars via your web server for example.

This PR actually forces people to have a .env file which could be a regression for some users.

@tangrufus
Copy link
Contributor

tangrufus commented Jul 30, 2021

@austinpray mentioned there is no .env file on a docker setup.
Therefore, we should allow omitting .env files.
See: #595 (comment)

@fab120
Copy link
Contributor Author

fab120 commented Jul 30, 2021

@austinpray mentioned there is no .env file on a docker setup.
Therefore, we should allow omitting .env files.
See: #595 (comment)

Oh yes. Missed that.
Removed the second commit that was deleting the if.

@swalkinshaw swalkinshaw merged commit 98f0c89 into roots:master Jul 30, 2021
@swalkinshaw
Copy link
Member

Thank you 👏 we'll get a release out asap

@fab120
Copy link
Contributor Author

fab120 commented Jul 30, 2021

My only question is if we really want to remove the file existence check. I think that was originally done to allow more flexibility with actual env vars. .env was chosen because it's portable and doesn't have to be used. You can inject env vars via your web server for example.

This PR actually forces people to have a .env file which could be a regression for some users.

Totally agree and honestly i didn't think at this use case when I was looking at that code.
A solution should use safeLoad instead of load:

$dotenv = Dotenv\Dotenv::createUnsafeImmutable($root_dir, $env_files, false);
$dotenv->safeLoad();
$dotenv->required(['WP_HOME', 'WP_SITEURL']);
if (!env('DATABASE_URL')) {
    $dotenv->required(['DB_NAME', 'DB_USER', 'DB_PASSWORD']);
}

If the file is missing no error is raised.
The two required checks will also pass because Dotenv is checking the environment and not the content of the .env file

@swalkinshaw
Copy link
Member

@fab120 that's nicer! Feel free to open a PR if you want since that's an improvement

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.

.env.local failed to open stream php error when WP_ENV='development' and .env.local does not exist.
3 participants