Bootstrap environment #408

joshlopes · 2018-05-15T11:24:41Z

Picking up one of @weaverryan branches and helping out glue everything together having in consideration the discussion on his PR #366 and the DotEnvLoad from @ogizanagi.

Below is the copy of that PR.

Q	A
License	MIT

Fixes symfony/flex#251

WIP

Needs to be reviewed and need tests?

Problem

The problem is that currently, environment variables must be completely duplicated between .env and phpunit.xml.dist

Solution

Extend the Kernel to allow different env to be loaded, falling back to the old behaviour ($_SERVER['APP_ENV']). The env files are overwritten, so we always load .env and .env.test values will override .env ones and so on.

.env is read in the same way as bin/console and public/index.php
Environment variables in phpunit.xml.dist WIN over .env.

ghost

Pull request passes validation.

joshlopes · 2018-05-15T11:26:26Z

symfony/framework-bundle/3.3/src/Kernel.php

@@ -35,6 +36,27 @@ public function registerBundles()
        }
    }

+    public static function bootstrapEnvironment(string $env = null)
+    {
+        $env = $env ?? $_SERVER['APP_ENV'] ?? null;


If we decide to include $env ?? $_SERVER['APP_ENV'] ?? $_ENV['APP_ENV'] ?? null we can remove the tenerary on phpunit bootsrap.

I think we should do this. But, read $_ENV['APP_ENV'] first - it should take priority. This is consistent with how env vars are read in the container:

https://github.com/symfony/symfony/blob/4f4c1725f0fcc2bb858ef0e2b5b892c5dac253a8/src/Symfony/Component/DependencyInjection/EnvVarProcessor.php#L71-L74

@weaverryan according to with what you just reviewed later:

So, for me, this $env argument is more of an optional, "override" - for a case where you want to use this $env even IF $_SERVER['APP_ENV'] is already set. So, it should be optional and should not be passed, except from the console.

This is the whole purpose of $env, so i think it should be read first, and fallback to the APP_ENV if we are not overriding it.

joshlopes · 2018-05-15T11:26:56Z

symfony/phpunit-bridge/3.3/phpunit.xml.dist

@@ -5,16 +5,16 @@
         xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/6.1/phpunit.xsd"
         backupGlobals="false"
         colors="true"
-         bootstrap="vendor/autoload.php"
+         bootstrap="tests/bootstrap.php"


should we update phpunit bridge configuration too ? or should this be done in phpunit bridge script instead?

ogizanagi · 2018-05-15T15:39:41Z

Thanks for taking care of this @joshlopes!
The https://github.com/symfony/recipes/blob/master/symfony/console/3.3/bin/console file misses an update too.
At first I thought about making Flex itself providing the DotenvLoader class but I guess it makes sense having it in the fwb recipe.

ghost

Pull request passes validation.

covex-nn · 2018-05-15T16:12:44Z

symfony/framework-bundle/3.3/src/Kernel.php

+        }
+
+        $envFiles = ["$projectDir/.env"];
+        if (file_exists($file = "$projectDir/.env.$env")) {


$env can be null here; under windows file_exists('.env.') will return true. So, if null === $env, then $envFiles will contain two elements: with .env and with .env.

covex-nn · 2018-05-15T16:38:29Z

symfony/framework-bundle/3.3/src/Kernel.php

@@ -35,6 +36,27 @@ public function registerBundles()
        }
    }

+    public static function bootstrapEnvironment(string $env = null)


I think $env should not be null here.
You already pass not null value from bin/console. You can pass a same value from public/index.php (you call bootstrapEnvironment and create $env var on the next line, just swap that lines and pass $env to bootstrapEnvironment!). Also you can pass $_SERVER['APP_ENV'] ?? 'test' from tests/bootstrap.php.

The current logic (which we want to keep) says that the .env file should ONLY be loaded if !isset($_SERVER['APP_ENV']). That means that we do need to use $_SERVER['APP_ENV'] inside this method. So, for me, this $env argument is more of an optional, "override" - for a case where you want to use this $env even IF $_SERVER['APP_ENV'] is already set. So, it should be optional and should not be passed, except from the console.

@weaverryan environment could be not set if 'test' === $_SERVER['APP_ENV'] if symfony/flex#318 will be accepted. And if so, then .env file(s) should be loaded (or .env.test?). And if so, then $env should be passed from tests/bootstrap.php too

if $_SERVER['APP_ENV'] was defined, we assume we already have the enviromnents loaded.

stof · 2018-05-15T17:03:15Z

symfony/console/3.3/bin/console

-    }
-    (new Dotenv())->load(__DIR__.'/../.env');
-}
-
 $input = new ArgvInput();
 $env = $input->getParameterOption(['--env', '-e'], $_SERVER['APP_ENV'] ?? 'dev', true);


This means that the environment from .env will not be considered anymore as fallback

I think i got you now, i think we should default this to null if $_SERVER['APP_ENV'] does not exist. That way the .env file will still be loaded and the environment of .env will be used as fallback.

stof · 2018-05-15T17:04:54Z

symfony/console/3.3/bin/console

 $input = new ArgvInput();
 $env = $input->getParameterOption(['--env', '-e'], $_SERVER['APP_ENV'] ?? 'dev', true);
+
+Kernel::bootstrapEnvironment($env);


I see an issue here: as soon as you use --env, the .env file won't be loaded anymore, meaning the flag won't be usable anymore locally.

even when you don't use it btw, due to the default to dev

@stof not sure what you mean, the env will always be loaded if there is a .env file and DotEnv is registered. It will also load both .env and .env.{ENV}. The default Server APP_ENV that is for, say, production. Whereas the default would then be prod for the cases without the option defined and then bootstrap would not happen.

any definition of APP_ENV in this file would not work btw, as it will always be set to a default value here to define $env

@stof i've changed so it loads .env and after that load whatever .env.$serverAppEnv was set there

symfony/framework-bundle/3.3/src/Kernel.php

weaverryan · 2018-05-17T14:46:15Z

symfony/framework-bundle/3.3/src/Kernel.php

@@ -35,6 +36,27 @@ public function registerBundles()
        }
    }

+    public static function bootstrapEnvironment(string $env = null)


The current logic (which we want to keep) says that the .env file should ONLY be loaded if !isset($_SERVER['APP_ENV']). That means that we do need to use $_SERVER['APP_ENV'] inside this method. So, for me, this $env argument is more of an optional, "override" - for a case where you want to use this $env even IF $_SERVER['APP_ENV'] is already set. So, it should be optional and should not be passed, except from the console.

weaverryan · 2018-05-17T14:48:41Z

symfony/framework-bundle/3.3/src/Kernel.php

+    {
+        $env = $env ?? $_SERVER['APP_ENV'] ?? null;
+        $projectDir = __DIR__.'/..';
+        if (!$env && !class_exists(Dotenv::class)) {


The current functionality is to load .env if and only if !isset($_SERVER['APP_ENV']). To stay consistent, I think this logic should change to:

if (isset($_SERVER['APP_ENV'])) { // environment is set, do not load .env return; } // then the rest of the code if (!class_exists(Dotenv::class)) { // ... } // ...

weaverryan · 2018-05-17T15:07:03Z

symfony/framework-bundle/3.3/src/Kernel.php

@@ -35,6 +36,27 @@ public function registerBundles()
        }
    }

+    public static function bootstrapEnvironment(string $env = null)
+    {
+        $env = $env ?? $_SERVER['APP_ENV'] ?? null;


I think we should do this. But, read $_ENV['APP_ENV'] first - it should take priority. This is consistent with how env vars are read in the container:

https://github.com/symfony/symfony/blob/4f4c1725f0fcc2bb858ef0e2b5b892c5dac253a8/src/Symfony/Component/DependencyInjection/EnvVarProcessor.php#L71-L74

weaverryan · 2018-05-17T15:10:27Z

phpunit/phpunit/4.7/phpunit.xml.dist

 >
    <php>
        <ini name="error_reporting" value="-1" />
        <env name="KERNEL_CLASS" value="App\Kernel" />
+        <env name="SHELL_VERBOSITY" value="-1" />
+
+        <!-- override or set env variables for the test env here -->
        <env name="APP_ENV" value="test" />
        <env name="APP_DEBUG" value="1" />


I don't believe this even works. This will be a string "1". I think we should remove it.

weaverryan · 2018-05-17T15:18:10Z

symfony/framework-bundle/3.3/public/index.php

-    }
-    (new Dotenv())->load(__DIR__.'/../.env');
-}
+Kernel::bootstrapEnvironment();


I'm wondering if we should SET $_SERVER['APP_ENV'] inside Kernel::bootstrapEnvironment();. Basically, move this logic there. We would:

Load .env when needed

If APP_ENV is still not set, set $_SERVER['APP_ENV'] = 'dev';

Except, if $env is passed to bootstrapEnvironment() (like for console), set that $env value into $_SERVER['APP_ENV'].

We could do the same for $_SERVER['APP_DEBUG']. The idea would be to simplify / de-duplicate all of the APP_ENV and APP_DEBUG logic. We could even move the umask & Debug code into bootstrapEnvironment()

weaverryan · 2018-08-30T13:00:37Z

Ping @joshlopes!

ghost

Pull request passes validation.

joshlopes · 2018-09-21T12:49:05Z

@weaverryan @dunglas should i just close this PR in favor of your PR's? I don't oppose to make it consistent with how react app is doing now for the env.

weaverryan · 2018-09-21T15:00:43Z

+1 Let's close @joshlopes - but I would love your input on the other PR's that are being made for this :).

…s dotenv behavior (dunglas) This PR was merged into the 4.2-dev branch. Discussion ---------- [DotEnv] Add a new loadForEnv() method mimicking Ruby's dotenv behavior | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no  | Deprecations? |no  | Tests pass? | yes  | Fixed tickets | helps for symfony/recipes#465, symfony/recipes#408 | License | MIT | Doc PR | todo This PR adds a new `loadForEnv()` method that mimics the behavior of [Create React App](https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/template/README.md#what-other-env-files-can-be-used), [Rails' DotEnv](https://github.com/bkeepers/dotenv#what-other-env-files-can-i-use) and probably some other libs: `DotEnv::loadForEnv()` will load the following files, starting from the bottom. The first value set (or those already defined in the environment) take precedence: - `.env` - The Original® - `.env.dev`, `.env.test`, `.env.prod`... - Environment-specific settings. - `.env.local` - Local overrides. This file is loaded for all environments _except_ `test`. - `.env.dev.local`, `.env.test.local`, `.env.prod.local`... - Local overrides of environment-specific settings. The plan is to use this method in the default SF installation (symfony/recipes#466). Commits ------- 774a78c [DotEnv] Add a new loadForEnv() method mimicking Ruby's dotenv behavior

joshlopes added 2 commits May 15, 2018 10:09

Boostrap env vars for console and phpunit

74eb3bc

Use the APP_ENV from so phpunit env is used

3eae3ed

ghost deployed to staging May 15, 2018 11:24 View deployment

ghost approved these changes May 15, 2018

View reviewed changes

joshlopes commented May 15, 2018

View reviewed changes

weaverryan mentioned this pull request May 15, 2018

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

Closed

Updated console script too

b0b5e1a

ghost requested a deployment to staging May 15, 2018 15:42 Abandoned

ghost approved these changes May 15, 2018

View reviewed changes

Removed DotEnv use

c6a9254

covex-nn reviewed May 15, 2018

View reviewed changes

stof requested changes May 15, 2018

View reviewed changes

stof reviewed May 15, 2018

View reviewed changes

fabpot requested changes May 17, 2018

View reviewed changes

symfony/framework-bundle/3.3/src/Kernel.php Outdated Show resolved Hide resolved

weaverryan requested changes May 17, 2018

View reviewed changes

weaverryan reviewed May 17, 2018

View reviewed changes

ogizanagi mentioned this pull request Sep 20, 2018

PHPUnit: use a .env file #465

Closed

This was referenced Sep 21, 2018

[DotEnv] Add a new loadForEnv() method mimicking Ruby's dotenv behavior symfony/symfony#28533

Merged

Allow to override .env files for specific environments #466

Closed

Tweaked how we default the env, removed unused logic

aba7823

ghost deployed to staging September 21, 2018 12:10 View deployment

ghost approved these changes Sep 21, 2018

View reviewed changes

weaverryan closed this Sep 21, 2018

joshlopes deleted the bootstrap-env-1 branch September 21, 2018 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bootstrap environment #408

Bootstrap environment #408

joshlopes commented May 15, 2018

ghost left a comment

joshlopes May 15, 2018

weaverryan May 17, 2018

joshlopes Sep 20, 2018

joshlopes May 15, 2018

ogizanagi commented May 15, 2018

ghost left a comment

covex-nn May 15, 2018 •

edited

Loading

covex-nn May 15, 2018 •

edited

Loading

weaverryan May 17, 2018

covex-nn May 17, 2018 •

edited

Loading

joshlopes Sep 21, 2018

stof May 15, 2018

joshlopes May 17, 2018 •

edited

Loading

stof May 15, 2018

stof May 16, 2018

joshlopes May 17, 2018

stof May 17, 2018

joshlopes Sep 21, 2018

weaverryan May 17, 2018

weaverryan May 17, 2018

weaverryan May 17, 2018

weaverryan May 17, 2018

weaverryan May 17, 2018

weaverryan commented Aug 30, 2018

ghost left a comment

joshlopes commented Sep 21, 2018

weaverryan commented Sep 21, 2018

Bootstrap environment #408

Bootstrap environment #408

Conversation

joshlopes commented May 15, 2018

WIP

Problem

Solution

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented May 15, 2018

ghost left a comment

Choose a reason for hiding this comment

covex-nn May 15, 2018 • edited Loading

Choose a reason for hiding this comment

covex-nn May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

covex-nn May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshlopes May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Aug 30, 2018

ghost left a comment

Choose a reason for hiding this comment

joshlopes commented Sep 21, 2018

weaverryan commented Sep 21, 2018

covex-nn May 15, 2018 •

edited

Loading

covex-nn May 15, 2018 •

edited

Loading

covex-nn May 17, 2018 •

edited

Loading

joshlopes May 17, 2018 •

edited

Loading