Adding test bootstrap.php file so .env vars are read #366

Open
wants to merge 2 commits into
from

Conversation

Projects
None yet
10 participants
@weaverryan
Member

weaverryan commented Feb 11, 2018

Q A
License MIT

Fixes symfony/flex#251

The problem is that currently, environment variables must be completely duplicated between .env and phpunit.xml.dist. That's just unacceptable DX to put in the user... which includes me - I don't want to do that :).

This is an easy win:

  • .env is read in the same way as bin/console and public/index.php
  • Environment variables in phpunit.xml.dist WIN over .env. This is used to set the environment to test.
Adding a new test bootstrap.php file so .env vars are read
This works particularly well, because any env vars set in
phpunit.xml.dist WIN over .env. This means that .env can
contain default values, but can then be easily overridden
for the test environment.
@symfony-flex-server

Pull request passes validation.

phpunit/phpunit/4.7/tests/bootstrap.php
+ * Environment variables can also be specified in phpunit.xml.dist.
+ * Those variables will override any defined in .env.
+ */
+if (!isset($_SERVER['APP_ENV'])) {

This comment has been minimized.

Show comment Hide comment
@quentinus95

quentinus95 Feb 11, 2018

Just by curiosity, why do we use this condition, also in the console? This disallow from having a generic bash script with a conditional behavior depending of the APP_ENV, and then launch specific commands based on this. It does not make sense in my opinion that APP_ENV=dev ./bin/console s:r launches the server without reading the .env file while ./bin/console s:r does. I will have the same issue here as I may set the APP_ENV var to test before launching the tests (I actually do) ; it won't load the .env file consequently...

I would expect that the .env file is always loaded (except if APP_ENV is defined and set to prod so we can prevent misuses of .env files), if it exists. The DotEnv component already takes care to avoid overriding already defined variables.

@quentinus95

quentinus95 Feb 11, 2018

Just by curiosity, why do we use this condition, also in the console? This disallow from having a generic bash script with a conditional behavior depending of the APP_ENV, and then launch specific commands based on this. It does not make sense in my opinion that APP_ENV=dev ./bin/console s:r launches the server without reading the .env file while ./bin/console s:r does. I will have the same issue here as I may set the APP_ENV var to test before launching the tests (I actually do) ; it won't load the .env file consequently...

I would expect that the .env file is always loaded (except if APP_ENV is defined and set to prod so we can prevent misuses of .env files), if it exists. The DotEnv component already takes care to avoid overriding already defined variables.

@fabpot

This comment has been minimized.

Show comment Hide comment
@fabpot

fabpot Feb 11, 2018

Owner

@weaverryan You should also create a companion PR on symfony/flex to remove the addition of env vars in phpunit.xml.dist.

Owner

fabpot commented Feb 11, 2018

@weaverryan You should also create a companion PR on symfony/flex to remove the addition of env vars in phpunit.xml.dist.

@ogizanagi

This comment has been minimized.

Show comment Hide comment
@ogizanagi

ogizanagi Feb 11, 2018

Member

@weaverryan: That's already a great improvement, but this doesn't solve another linked issue that should also be considered to me: running a command in another env, including test one, which is sometimes handy to debug your test suite, or even simply to init the database/other stuff (despite it can also be done from the test suite bootstrap file).

If it can be of any help, here is what I'm using currently on flex projects: A DotEnvLoader class allows to load an extra .env.[env] file (in addition to the .env one), where "[env]" is the symfony env. Other patches inside the gist shows how to use it.

Member

ogizanagi commented Feb 11, 2018

@weaverryan: That's already a great improvement, but this doesn't solve another linked issue that should also be considered to me: running a command in another env, including test one, which is sometimes handy to debug your test suite, or even simply to init the database/other stuff (despite it can also be done from the test suite bootstrap file).

If it can be of any help, here is what I'm using currently on flex projects: A DotEnvLoader class allows to load an extra .env.[env] file (in addition to the .env one), where "[env]" is the symfony env. Other patches inside the gist shows how to use it.

Loading .env even if APP_ENV is set
This is tough: we're getting WTF moments when someone tries to set,
for example, APP_ENV=test or some other environment, and suddenly
the .env file isn't loaded.

This is to hopefully make things work in a more predictable way: .env
is always loaded, except in the prod environment. This works because
DotEnv does not override any existing variables, so there's no risk
in loading it.
@weaverryan

This comment has been minimized.

Show comment Hide comment
@weaverryan

weaverryan Feb 11, 2018

Member

Hey guys!

I've just made this PR more complicated :). I've change to load the .env file even if the APP_ENV. The only time we don't load it is in the prod environment.

I think this is important: we continue to see users confused about why the .env variable suddenly isn't read if they set APP_ENV. The previous situation also made this PR strange: the phpunit.xml.dist env vars are loaded FIRST, and then bootstrap.php is executed. But, the phpunit.xml.dist env vars are ONLY added to $_ENV, not $_SERVER. So, .env was still loaded (which I liked). But if you suddenly started setting true env vars, you'd get a different behavior.

So the goal is simple: load the .env file in the least WTF kind of way. Because it doesn't override existing env vars, I think this makes sense.

Also, @ogizanagi brings up another point in his comment. There's not an easy way, for example, to set a different DATABASE_URL in test, and be able to use it in your tests and in bin/console. He suggests the possibility of having a .env.[env] file that would override the normal one, and I agree. It's also what Laravel does, which I think is important, because they've been using the .env idea longer so have probably learned a few things.

So.... I think we need to make several changes to our .env handling.... and so possibly some sort of bootstrap.php file that's used by all entry points - index.php, console, testing.

Cheers!

Member

weaverryan commented Feb 11, 2018

Hey guys!

I've just made this PR more complicated :). I've change to load the .env file even if the APP_ENV. The only time we don't load it is in the prod environment.

I think this is important: we continue to see users confused about why the .env variable suddenly isn't read if they set APP_ENV. The previous situation also made this PR strange: the phpunit.xml.dist env vars are loaded FIRST, and then bootstrap.php is executed. But, the phpunit.xml.dist env vars are ONLY added to $_ENV, not $_SERVER. So, .env was still loaded (which I liked). But if you suddenly started setting true env vars, you'd get a different behavior.

So the goal is simple: load the .env file in the least WTF kind of way. Because it doesn't override existing env vars, I think this makes sense.

Also, @ogizanagi brings up another point in his comment. There's not an easy way, for example, to set a different DATABASE_URL in test, and be able to use it in your tests and in bin/console. He suggests the possibility of having a .env.[env] file that would override the normal one, and I agree. It's also what Laravel does, which I think is important, because they've been using the .env idea longer so have probably learned a few things.

So.... I think we need to make several changes to our .env handling.... and so possibly some sort of bootstrap.php file that's used by all entry points - index.php, console, testing.

Cheers!

@symfony-flex-server

Pull request passes validation.

@quentinus95

This comment has been minimized.

Show comment Hide comment
@quentinus95

quentinus95 Feb 11, 2018

I guess the possibility to use .env.[env] files is mitigated by the fact that we can now load the .env file even if some environment files are already loaded. One could easily create custom .env.test file (for instance) and load those using a source + allexport in a bash script and then run the tests normally.

I guess the possibility to use .env.[env] files is mitigated by the fact that we can now load the .env file even if some environment files are already loaded. One could easily create custom .env.test file (for instance) and load those using a source + allexport in a bash script and then run the tests normally.

@ogizanagi

This comment has been minimized.

Show comment Hide comment
@ogizanagi

ogizanagi Feb 12, 2018

Member

What about symfony/flex providing something like the DotEnvLoader class mentioned earlier (or whatever name we use for it) and use it here ?

Member

ogizanagi commented Feb 12, 2018

What about symfony/flex providing something like the DotEnvLoader class mentioned earlier (or whatever name we use for it) and use it here ?

@SergeC

This comment has been minimized.

Show comment Hide comment
@SergeC

SergeC Feb 16, 2018

Maybe we should have a .env.[env].dist which will be updated by SF Flex the same way as .env.dist? The .env.[env].dist will be pushed to GIT while .env.[env] will be ignored. If env=prod the use .env.

SergeC commented Feb 16, 2018

Maybe we should have a .env.[env].dist which will be updated by SF Flex the same way as .env.dist? The .env.[env].dist will be pushed to GIT while .env.[env] will be ignored. If env=prod the use .env.

@bocharsky-bw

This comment has been minimized.

Show comment Hide comment
+ * Those variables will override any defined in .env.
+ */
+
+if (!isset($_SERVER['APP_ENV']) || 'prod' !== $_SERVER['APP_ENV']) {

This comment has been minimized.

Show comment Hide comment
@bocharsky-bw

bocharsky-bw Feb 22, 2018

Contributor

Why don't we use getenv('APP_ENV') instead of $_SERVER['APP_ENV']. Are there any reasons behind? Symfony recommends to use getenv() in docs but has $_SERVER[] in code which is not consistent as for me.

@bocharsky-bw

bocharsky-bw Feb 22, 2018

Contributor

Why don't we use getenv('APP_ENV') instead of $_SERVER['APP_ENV']. Are there any reasons behind? Symfony recommends to use getenv() in docs but has $_SERVER[] in code which is not consistent as for me.

This comment has been minimized.

Show comment Hide comment
@ogizanagi

ogizanagi Feb 22, 2018

Member

See #156 about that. It should probably be updated in the docs then.

@ogizanagi

ogizanagi Feb 22, 2018

Member

See #156 about that. It should probably be updated in the docs then.

@Nyholm

Great. I like this. Just had a minor question

+ * Environment variables can also be specified in phpunit.xml.dist.
+ * Those variables will override any defined in .env.
+ */
+if (!isset($_SERVER['APP_ENV'])) {

This comment has been minimized.

Show comment Hide comment
@Nyholm

Nyholm Feb 25, 2018

Member

Shouln't this be the same as for version 4.7?

if (!isset($_SERVER['APP_ENV']) || 'prod' !== $_SERVER['APP_ENV']) {

@Nyholm

Nyholm Feb 25, 2018

Member

Shouln't this be the same as for version 4.7?

if (!isset($_SERVER['APP_ENV']) || 'prod' !== $_SERVER['APP_ENV']) {

+
+/*
+ * Environment variables can also be specified in phpunit.xml.dist.
+ * Those variables will override any defined in .env.

This comment has been minimized.

Show comment Hide comment
@Nyholm

Nyholm Feb 25, 2018

Member

Good!

@Nyholm

Nyholm Feb 25, 2018

Member

Good!

@@ -8,7 +8,7 @@
require __DIR__.'/../vendor/autoload.php';
// The check is to ensure we don't use .env in production
-if (!isset($_SERVER['APP_ENV'])) {
+if (!isset($_SERVER['APP_ENV']) || 'prod' !== $_SERVER['APP_ENV']) {

This comment has been minimized.

Show comment Hide comment
@B-Galati

B-Galati Mar 8, 2018

Contributor

I just had an issue trying to warm-up my cache on the CI.
My APP_ENV was test and I was overriding the env with the --env option like so :

bin/console --env=prod cache:clear --no-warmup
[08-Mar-2018 16:54:49 Europe/Paris] PHP Fatal error:  Uncaught RuntimeException: APP_ENV environment variable is not defined. You need to define environment variables for configuration or add "symfony/dotenv" as a Composer dependency to load variables from a .env file. in /app/bin/console:20

I can see that my case is kind of weird and I am going to change the way I ran the command but we could do something to improve DX in that case perhaps ?

@B-Galati

B-Galati Mar 8, 2018

Contributor

I just had an issue trying to warm-up my cache on the CI.
My APP_ENV was test and I was overriding the env with the --env option like so :

bin/console --env=prod cache:clear --no-warmup
[08-Mar-2018 16:54:49 Europe/Paris] PHP Fatal error:  Uncaught RuntimeException: APP_ENV environment variable is not defined. You need to define environment variables for configuration or add "symfony/dotenv" as a Composer dependency to load variables from a .env file. in /app/bin/console:20

I can see that my case is kind of weird and I am going to change the way I ran the command but we could do something to improve DX in that case perhaps ?

@sstok

This comment has been minimized.

Show comment Hide comment
@sstok

sstok Mar 11, 2018

Contributor

To be clear phpunit follows the same convention as the DotEnv component, if an env is already set it will not overwrite, unless you provide force="true".

Contributor

sstok commented Mar 11, 2018

To be clear phpunit follows the same convention as the DotEnv component, if an env is already set it will not overwrite, unless you provide force="true".

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