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

[WebServerBundle] Environment variables are not reloaded when the .env file was changed #23723

Closed
voronkovich opened this issue Jul 30, 2017 · 11 comments

Comments

@voronkovich
Copy link
Contributor

voronkovich commented Jul 30, 2017

Q A
Bug report? yes
Symfony version 4.0

I think this problem occurs because when the server is started (see https://github.com/symfony/recipes/blob/master/symfony/console/3.3/bin/console#L20), the .env file is loaded and then, loaded variables cannot be overridden, because a Dotenv component doesn't override the existing ones.
We could solve this issue by adding an ability to Dotenv component to override the existing environment variables.

@javiereguiluz
Copy link
Member

I'm afraid I don't understand the exact problem:

  • Dotenv doesn't reload the .env file and that's why if you change its contents, you don't see any change?
  • Dotenv doesn't override the existing env vars in your computer if their names match the ones found in .env file?

@voronkovich
Copy link
Contributor Author

@javiereguiluz. To be more concise - the front controller doesn't reload the .env file because the APP_ENV variable is already loaded since the server's process inherits its parent's process variables.

// This piece of code occurs in the bin/console and in the front controller
if (!getenv('APP_ENV')) {
    (new Dotenv())->load(__DIR__.'/../.env');
}

@voronkovich
Copy link
Contributor Author

voronkovich commented Jul 31, 2017

@javiereguiluz, I've created a simple application that reproduces the problem. See https://github.com/voronkovich/symfony-unreloadable-envs

@chalasr
Copy link
Member

chalasr commented Jul 31, 2017

Marking this as a feature request for the ability to overload env vars, which is not supported currently.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 5, 2017

Got it :)
Instead of #23761 and #23720, I suggest a third approach: configuration via env vars:
when creating vars, Dotenv would add a special additional env var, let's call it DOTENV_VARS, that would contain all env var names that it created, separated by a = (an impossible char in a name) eg.:
$_SERVER['DOTENV_VARS'] = 'FOO=BAR=BAZ';.

Reciprocally, when this env var is set, Dotenv would allow overriding of the listed vars.
That should fix this issue and open for more use cases.

@fabpot
Copy link
Member

fabpot commented Aug 5, 2017

What about resetting env vars at the end of each request when using the Symfony web server?

@voronkovich
Copy link
Contributor Author

@nicolas-grekas, Your solution sounds great for me! So, the front controller will look like this?:

if (file_exists(__DIR__'/../.env')) {
    (new Dotenv())->load(__DIR__.'/../.env');
}

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 5, 2017

@fabpot the issue is the subprocess that runs php -S: it inherits real env vars from the parent process, which itself created "false" env vars from .env. I don't think we can do anything between each request that would fix that. Or I have missed the point :)

@voronkovich I wouldn't change at all the front controller, but I would change the WebServer class so that it doesn't forward the APP_ENV var when the var has been created by Dotenv (which we can know by looking at DOTENV_VARS there.)

@voronkovich
Copy link
Contributor Author

@nicolas-grekas, so, we should add a method to remove all loaded envs? Dotenv::removeLoadedVars or something like this.

// WebServer.php

(new Dotenv())->removeLoadedVars();

Maybe it's better to release the WebServer as a standalone app? See #23771

@nicolas-grekas
Copy link
Member

If we can make it work without any change, that'd be the best. I don't think we need to make it standalone. At least I don't see the benefit. Fixing this issue doesn't require any new method nor class to me, it just needs dealing with an env var in some places. At least I'd make it work that way first, and of course the discussion can go on.

@voronkovich
Copy link
Contributor Author

@nicolas-grekas, I've created a PR with implementation of your idea: #23799 Could you please take a look at the code?

fabpot added a commit that referenced this issue Aug 22, 2017
…bles (voronkovich)

This PR was squashed before being merged into the 3.3 branch (closes #23799).

Discussion
----------

[Dotenv][WebServerBundle] Override previously loaded variables

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23723
| License       | MIT

This PR implements @nicolas-grekas's idea about how we could refresh loaded environment variables. See his comment #23723 (comment)

Commits
-------

c5a1218 [Dotenv][WebServerBundle] Override previously loaded variables
@fabpot fabpot closed this as completed Aug 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants