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

Projects
None yet
7 participants
@voronkovich
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

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Jul 31, 2017

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?
Member

javiereguiluz commented Jul 31, 2017

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

This comment has been minimized.

Show comment
Hide comment
@voronkovich

voronkovich Jul 31, 2017

Contributor

@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');
}
Contributor

voronkovich commented Jul 31, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@voronkovich

voronkovich Jul 31, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Jul 31, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 5, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Aug 5, 2017

Member

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

Member

fabpot commented Aug 5, 2017

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

@voronkovich

This comment has been minimized.

Show comment
Hide comment
@voronkovich

voronkovich Aug 5, 2017

Contributor

@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');
}
Contributor

voronkovich commented Aug 5, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 5, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@voronkovich

voronkovich Aug 5, 2017

Contributor

@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

Contributor

voronkovich commented Aug 5, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 5, 2017

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.

Member

nicolas-grekas commented Aug 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@voronkovich

voronkovich Aug 6, 2017

Contributor

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

Contributor

voronkovich commented Aug 6, 2017

@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

bug #23799 [Dotenv][WebServerBundle] Override previously loaded varia…
…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 Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment