--env no longer equivalent to APP_ENV

[bendavies]

Related to #491

These two should be equivalent, shouldn't they?

> APP_ENV=prod APP_DEBUG=0 bin/console cache:clear --no-warmup
> bin/console cache:clear --no-warmup --env=prod --no-debug

Reproducer:

> composer create-project symfony/skeleton skeleton 
> cd skeleton
> rm -rf vendor
> composer install --no-dev #error will be here too.
> bin/console cache:clear --no-warmup --env=prod --no-debug

> Fatal error: Uncaught RuntimeException: The "APP_ENV" environment variable is not set to "prod". Please run "composer require symfony/dotenv" to load the ".env" files configuring the application. in project/src/.bootstrap.php on line 13

Thanks

[nicolas-grekas]

We did that on purpose: defining the real APP_ENV to prod means opting out from loading .env files.
--env prod loads .env file. The fatal error is bad though :) We should replace it with a better behavior, and also merge symfony/skeleton#122 IMHO (and you can move Dotenv to "require" meanwhile).
ping @weaverryan FYI, WDYT?

[bendavies]

right, so --env and --no-debug are not equivalent to APP_ENV and APP_DEBUG any more?
That's a pretty big gotcha, and totally unexpected.

related discussion here: #491 (comment)

[nicolas-grekas]

Hum, the exception is right, you should require dotenv.
We should move it by default to "require" to me, please support symfony/skeleton#122 if you agree. (please tell why and how we should do otherwise ;)

[bendavies]

I'm afraid I can't agree since (repeating myself - sorry), it seems really poor from a DX perspective that --env and APP_ENV are not equivalent, as @Tobion pointed out.

It seems like breaking change to me. what is the BC policy here?

FYI, we had mostly migrated an app to completely use env vars, but had some older supervisord jobs using --env=prod, and they broke as soon as this change was deployed.

[nicolas-grekas]

I understand the current behavior might not be unanimously OK, but the BC break argument makes no sense to me: you don't apply new recipes on existing code automatically. It's your responsibility to commit the resulting files and once done, they won't change unless you accept the change.

[bendavies]

I get what you are saying about BC, but from my point of view:

1. The app started on symfony 4.1,
2. I updated a recipe as suggested (because i'm a good boy!), and i would expect no functional changes at all, apart from those obvious ones documented.
3. The app is still on symfony 4.1
4. and the result is an error in production which would have been very hard, almost impossible, for us to catch in tests.

I hope I'm making my point clear!
Thanks for considering!

[weaverryan]

would expect no functional changes at all, apart from those obvious ones documented.

Then it might be best fixed in documentation. Seen from the opposite perspective, suppose a project is not using real env vars - they have a .env.local on production. They run bin/console cache:clear --env=prod (or maybe --env=dev for some reason) and suddenly their .env files are not loaded. The env vars ARE loaded without the flag, however.

The new logic is meant to be simple: if you use real ENV vars (specifically APP_ENV=prod), the .env files are not loaded. If you do NOT use real env vars, the .env files are loaded.

Let me know if that makes sense!

[bendavies]

hi @weaverryan, thanks for the reply.

I understand what you are saying - it makes sense - but I can't agree as stated above :)

I don't see how a such large change in behaviour can be brought in seemingly under the radar.
Obviously I agree this should be documented if it's going to remain, but I'd question how many will notice and how many people will be caught out, despite being documented.

Apologies for kicking up such a fuss, I just don't like being caught out in production :)

Cheers

[weaverryan]

@bendavies No worries :). Actually, I could use your help. I was just checking into adding some notes about this. But, I'm confused about what the functionality was BEFORE. Here's the bin/console file back from before the recent changes: https://github.com/symfony/recipes/blob/584967704025c23f12165a259fd80a5ad7717509/symfony/console/3.3/bin/console

Based on your comments, I was expecting that, when --env=prod was passed, the .env file was not loaded. But the code there looks like .env IS loaded with --env=prod (unless you also have a real APP_ENV=prod env var set).

I'm certain I'm misunderstanding a subtle thing here... and so, almost certainly something we should point out in the docs.

I don't see how a such large change in behaviour can be brought in seemingly under the radar.

And that's definitely not the goal - these recipe changes are tricky because they take effect immediately... hence why I wanted to write that blog post + these docs. Let's make those as foolproof as possible.

Thanks!

[bendavies]

@weaverryan you are right.

I checked, It's actually a nice edge case.
What our supervisor processors were running was, in effect:
APP_ENV=prod bin/console whatever --env=prod.

So they were specifying both, as we had moved the majority of config to env vars, but had not removed --env=prod from our supervisord config.

So under those conditions, on the old version, .env would not be loaded, and on the new version, it is.