Add checks for Dotenv before calling it #247

BPScott · 2017-11-10T16:41:18Z

composer install --no-dev fails when Dotenv is a dev dependency (which
it is in skeleton) and you're running in an environment without an
APP_ENV variable set (say you're testing prod-like settings locally).

After that bin/console help also fails, as does
bin/console --env=prod help (as Dotenv is loaded before the argument
can be read).

APP_ENV=prod bin/console help works but instead lets avoid needing to
remember that by only trying to load Dotenv if it actually exists.

Strictly speaking this is only required for console, however I have
placed the check in the other places Dotenv is included for consistency.

See symfony/skeleton#32 for some initial pondering relating to this.

Q	A
License	MIT

ghost

Pull request passes validation.

ghost

Pull request passes validation.

dunglas · 2017-11-10T17:55:43Z

behat/symfony2-extension/2.1/features/bootstrap/bootstrap.php

@@ -3,6 +3,6 @@
 use Symfony\Component\Dotenv\Dotenv;

 // The check is to ensure we don't use .env in production
-if (!isset($_SERVER['APP_ENV'])) {
+if (class_exists(Dotenv::class) && !isset($_SERVER['APP_ENV'])) {


Should be done after the call to isset for perf.

ghost

Pull request passes validation.

BPScott · 2017-11-11T13:08:08Z

Good call @dunglas. Updated.

fabpot · 2017-11-11T15:40:45Z

I don't think this is a good strategy as the app will fail as the env vars are not defined. We should only support 2 use cases:

.env file (DotEnv component is required)
real en vars all the way (DotEnv component is not needed)

With this PR, you make it possible to have neither. That will not work and should be avoided.

What we can do is throwing an exception when the env vars are not defined and DotEnv is not available explaining that the user must either define all env vars or install DotEnv.

ghost

Pull request passes validation.

BPScott · 2017-11-11T23:17:42Z

Indeed, the application will still fail, just a little bit later once it tries to use a variable that was not provided through the actual env or dotenv. I was originally thinking of "this could let you run a subset of the app that didn't need all the env parameters". An example of that being bin/console cache:clear which takes its environment from the CLI argument rather than the APP_ENV variable.

It's a weird twilight zone that works more through luck than judgement and is brittle, so I'm happy to say all bets are off, Symfony doesn't support that and to provide a more relevant error message.

I now throw an error instead of powering through. Let me know if that's good wording (I'm not familiar with the conventions that Symfony uses for messaging, this was based off the other class_exists check in the console) and I'll squash and rebase.

fabpot · 2017-11-11T23:40:34Z

behat/symfony2-extension/2.1/features/bootstrap/bootstrap.php

@@ -4,5 +4,8 @@

 // The check is to ensure we don't use .env in production
 if (!isset($_SERVER['APP_ENV'])) {
+    if (!class_exists(Dotenv::class)) {
+        throw new \RuntimeException('You need to define environment variables for configuration or add "symfony/dotenv" as a Composer dependency');


What about something along the lines of:

APP_ENV environment variable is not defined. All project's environment variables should be defined or set via a .env file (for which you need to require "symfony/dotenv" as a Composer dependency).

I'm not very satisfied with the should be defined or set part as I'm not sure it's easily understandable. We could probably find something better.

What about straightforward; Loading project environment variables requires symfony/dotenv ~3.4|4.0?

I'd go for Loading project environment variables from .env file requires symfony/dotenv, so we don't need to show anything about the version

(Whoops, life got busy)

@ro0NL & @Pierstoval: I think the "this is your next step" message should make it clear that Symfony prefers using env variables rather than a .env in production. Thus this ends up being a bit wordy as the user has two options dependant on what they want to do. Given you're more likely to run into this issue while building for production (as dotenv is installed by default in dev) it's worth starting off talking about env variables, rather than directing people straight to dotenv IMO.

The existing message takes inspiration from the other class_exists check in bin/console (see below). I think it's worth keeping the voice between these two messages somewhat consistent, which is why I started with "You need to...".

if (!class_exists(Application::class)) { throw new \RuntimeException('You need to add "symfony/framework-bundle" as a Composer dependency.'); }

How about 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.?

And then updating Application class check message to Application class does not exist. You need to add "symfony/framework-bundle" as a Composer dependency. Which keeps a "[why the check failed] [how to fix it]" format.

@BPScott Your proposal sounds good to me. Can you update the PR?

ghost

Pull request passes validation.

BPScott · 2017-11-23T14:13:20Z

@fabpot updated, squashed and rebased onto latest master.

fabpot · 2017-11-23T14:14:11Z

symfony/console/3.3/bin/console

@@ -12,10 +12,13 @@ set_time_limit(0);
 require __DIR__.'/../vendor/autoload.php';

 if (!class_exists(Application::class)) {
-    throw new \RuntimeException('You need to add "symfony/framework-bundle" as a Composer dependency.');
+    throw new \RuntimeException('Application class does not exist. You need to add "symfony/framework-bundle" as a Composer dependency.');


I would remove this addition as it does not add anything useful for the user. Nobody knows what this Application class is and why it is useful (and nobody should care to be honest). Moreover this error message should never be displayed anyway.

fixed

`composer install --no-dev` fails when Dotenv is a dev dependency (which it is in skeleton) and you're running in an environment without an APP_ENV variable set. Throw exception when no env variables are set and Dotenv is absent so the user knows to either require DotEnv or specify an APP_ENV variable.

ghost

Pull request passes validation.

apfelbox · 2018-01-10T13:17:20Z

This unfortunately breaks if you don't want to use env-Vars all the way (as in our case we have to copy the env vars to around 10 different places instead of one parameters.yaml).

Why do we force DotEnv or env variables on the ecosystem?
Why not just use the defaults, as they are already still defined:

recipes/symfony/console/3.3/bin/console

Lines 26 to 27 in b3c5ff1

    
           $env = $input->getParameterOption(['--env', '-e'], $_SERVER['APP_ENV'] ?? 'dev'); 
        
           $debug = ($_SERVER['APP_DEBUG'] ?? ('prod' !== $env)) && !$input->hasParameterOption(['--no-debug', '']);

Is it not an option to just have:

$env = $_SERVER['APP_ENV'] ?? 'dev';
$debug = $_SERVER['APP_DEBUG'] ?? ('prod' !== $env);

if ($debug && class_exists(Dotenv::class)) {
    (new Dotenv())->load(__DIR__.'/../.env');
}

... which would satisfy both worlds, the ones that go all the env way, and the ones that go the config/parameters.yaml way.

apfelbox · 2018-01-11T16:29:19Z

@fabpot @nicolas-grekas what do you think about the last comment?

nicolas-grekas · 2018-01-11T16:39:32Z

@apfelbox please open PR so we can keep track of any discussion :)

apfelbox · 2018-01-11T17:09:11Z

@nicolas-grekas oh sure! Yeah, discussing in a closed PR is not the best idea. 👍

See #340

ghost deployed to staging November 10, 2017 16:41 View deployment

ghost approved these changes Nov 10, 2017

View reviewed changes

ghost deployed to staging November 10, 2017 16:41 View deployment

ghost approved these changes Nov 10, 2017

View reviewed changes

BPScott mentioned this pull request Nov 10, 2017

composer install --no-dev fails symfony/skeleton#32

Closed

dunglas previously requested changes Nov 10, 2017

View reviewed changes

ghost deployed to staging November 11, 2017 13:05 View deployment

ghost approved these changes Nov 11, 2017

View reviewed changes

ghost deployed to staging November 11, 2017 23:13 View deployment

ghost approved these changes Nov 11, 2017

View reviewed changes

fabpot reviewed Nov 11, 2017

View reviewed changes

xabbuh mentioned this pull request Nov 13, 2017

DotEnv used even if component is not installed #235

Closed

garak mentioned this pull request Nov 22, 2017

recipe fails if .env.dist is not present #270

Closed

ghost deployed to staging November 23, 2017 14:10 View deployment

ghost approved these changes Nov 23, 2017

View reviewed changes

fabpot requested changes Nov 23, 2017

View reviewed changes

ghost deployed to staging November 23, 2017 15:39 View deployment

ghost approved these changes Nov 23, 2017

View reviewed changes

fabpot approved these changes Nov 23, 2017

View reviewed changes

nicolas-grekas approved these changes Nov 23, 2017

View reviewed changes

ghost merged commit b3c5ff1 into symfony:master Nov 23, 2017

ghost pushed a commit that referenced this pull request Nov 23, 2017

Merge pull request #247

391c470

apfelbox mentioned this pull request Jan 10, 2018

Support for bundle inline recipe configuration symfony/flex#206

Closed

apfelbox mentioned this pull request Jan 11, 2018

Don’t break when not using env vars #340

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checks for Dotenv before calling it #247

Add checks for Dotenv before calling it #247

BPScott commented Nov 10, 2017 •

edited

Loading

ghost left a comment

ghost left a comment

dunglas Nov 10, 2017

ghost left a comment

BPScott commented Nov 11, 2017

fabpot commented Nov 11, 2017

ghost left a comment

BPScott commented Nov 11, 2017

fabpot Nov 11, 2017

ro0NL Nov 21, 2017

Pierstoval Nov 21, 2017

BPScott Nov 21, 2017

fabpot Nov 23, 2017

ghost left a comment

BPScott commented Nov 23, 2017

fabpot Nov 23, 2017

BPScott Nov 23, 2017

ghost left a comment

apfelbox commented Jan 10, 2018

apfelbox commented Jan 11, 2018

nicolas-grekas commented Jan 11, 2018

apfelbox commented Jan 11, 2018

Add checks for Dotenv before calling it #247

Add checks for Dotenv before calling it #247

Conversation

BPScott commented Nov 10, 2017 • edited Loading

ghost left a comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

BPScott commented Nov 11, 2017

fabpot commented Nov 11, 2017

ghost left a comment

Choose a reason for hiding this comment

BPScott commented Nov 11, 2017

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

ghost left a comment

Choose a reason for hiding this comment

BPScott commented Nov 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

apfelbox commented Jan 10, 2018

apfelbox commented Jan 11, 2018

nicolas-grekas commented Jan 11, 2018

apfelbox commented Jan 11, 2018

BPScott commented Nov 10, 2017 •

edited

Loading