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

[Dotenv] add loadEnv(), a smoother alternative to loadForEnv() #29129

Merged
merged 1 commit into from Nov 9, 2018

Conversation

Projects
None yet
5 participants
@nicolas-grekas
Member

nicolas-grekas commented Nov 7, 2018

Q A
Branch? 4.2
Bug fix? yes
New feature? no
BC breaks? yes (4.2-only)
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

This PR replaces the loadForEnv() method introduced in #28533 by a new loadEnv() method.

  • It accepts only one mandatory argument: $path, which is the path to the .env file.
  • The 2nd argument is optional and defines the name of the environment variable that defines the Symfony env. This plays better with the current practice of defining the env in .env (loadForEnv() requires knowing the env before being called, leading to a chicken-n-egg situation that loadEnv() avoids.)
  • the possibility to load several files at once is removed. We don't have a use case for it and those who do can call loadEnv() in a loop anyway.

In addition to $path (.env), the following files are loaded, the latter taking precedence in this order:
.env < env.local < .env.$env < .env.$env.local

Note that loadForEnv() used to give higher precedence to .env.local vs .env.$env.
The new behavior is aligned with the order used by create-react-app. It also allows overriding the env in .env.local, which should be convenient for DX.

Last but not least, the "test" env has this special behaviors:

  • .env.local file is skipped for the "test" env (same as before and as in create-react-app)
  • vars defined in .env files override real env vars (similar to what Rails' dotenv does: you don't want your tests to randomly fail because of some real env vars).
@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Nov 7, 2018

Note that loadForEnv() used to give higher preference to .env.$env vs .env.local.
The new behavior is aligned with the order used by create-react-app.

But not aligned with bkeepers/dotenv. See symfony/symfony-docs#10626 (comment)
Not sure which one really is the most convenient one.

vars defined in .env files override real env vars (similar to what Rails' dotenv does: you don't want your tests to randomly fail because of some real env vars).

I'm used to occasionally start phpunit with APP_DEBUG=1 bin/phpunit --filter=[...] to debug some tests (APP_DEBUG=0 is used by default in my tests suites). This will prevent it, right?
What about CI using real env vars?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 7, 2018

But not aligned with bkeepers/dotenv.

Sure. create-react-app has the behavior that best matches Symfony, so why should we care more about bkeepers?

APP_DEBUG=1 bin/phpunit --filter=[...] This will prevent it, right? What about CI using real env vars?

Nope it won't, useless you defined APP_DEBUG yourself in .env (we don't do that by default).
Same for other real env vars: if they're not defined in .env(.*) they'll be used.

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Nov 7, 2018

create-react-app has the behavior that best matches Symfony

Based on which facts? 🤔

unless you defined APP_DEBUG yourself in .env (we don't do that by default).

Indeed. We provided the value in https://github.com/symfony/recipes/blob/5b3ce909504b9366405820c49bc9f0e13ac4be54/symfony/phpunit-bridge/4.1/phpunit.xml.dist#L14, but that's not relevant with planned changes.
Still, if my test suite is meant to be ran by default with APP_DEBUG=0, it'll need to be set in .env.test, so wouldn't be overridable anymore.

Same for other real env vars: if they're not defined in .env(.*) they'll be used.

So, what do you preconize for CI appart from either removing .env(.*) files or not using env vars at all and requiring generating a .env.test[.local] on each build?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 7, 2018

Based on which facts? thinking

You're asking because you think we should keep the current behavior?

My arguments for the new one are:

  • matching the behavior of create-react-app looks more important than matching the one of a rails app (how often will ppl use both rails+symfony vs symfony+create-react-app?)
  • as explained in the description, having this order allows defining the env in .env.local - before .env.$env(.local). It wouldn't be that easy without.

if my test suite is meant to be ran by default with APP_DEBUG=0, it'll need to be set in .env.test

yes, this is described in https://metaskills.net/2013/10/03/using-dotenv-in-rails/

Now, one last thing. We need to make sure that our tests do not use any real, development or local environment settings. This is where @ecbypi overload feature comes in handy.

the reason is that tests run in a special set of configuration and the very purpose of .env.test is to commit that configuration. If you don't want to follow what the devs set there, you have to do extra work. OR if your test setup needs to be flexible, you can omit defining those vars in .env files.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 7, 2018

BTW, about the test env: if, for some FOO env var, one would like to give control to the real env while that var is defined in .env, the way to do it would be to add FOO=$FOO in .env.test (or .env.test.local)

@dunglas

This comment has been minimized.

Member

dunglas commented Nov 7, 2018

The new behavior is aligned with the order used by create-react-app. It also allows overriding the env in .env.local, which should be convenient for DX.

👍 on my side. Many people will use Create React App and Symfony together in the same project, Rails + Symfony will definitely be less popular.

vars defined in .env files override real env vars (similar to what Rails' dotenv does: you don't want your tests to randomly fail because of some real env vars).

Are you sure about that? According to their docs, it's not the case by default, but you can use overload (as in our DotEnv) to have this behavior: https://github.com/bkeepers/dotenv#why-is-it-not-overriding-existing-env-variables

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 7, 2018

OK, overloading for test removed. Ready.

@dunglas

dunglas approved these changes Nov 7, 2018

@ogizanagi

What would it be like in the recipe with the --env re-introduction, though?

Dotenv::loadEnv('.env', 'APP_ENV', $input->getParameterOption(['--env', '-e'], 'dev', true));

? --env would be ignored if APP_ENV env var is set...which is fine to me. Just sayin' :)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Nov 8, 2018

--env would be ignored if APP_ENV env var is set

Right now, --env is considered more specific, so it wins over the env var.
I don't see why we should change that part.

@Tobion

Tobion approved these changes Nov 9, 2018

@nicolas-grekas nicolas-grekas merged commit 0cf9acb into symfony:master Nov 9, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Nov 9, 2018

bug #29129 [Dotenv] add loadEnv(), a smoother alternative to loadForE…
…nv() (nicolas-grekas)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Dotenv] add loadEnv(), a smoother alternative to loadForEnv()

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | yes (4.2-only)
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

This PR replaces the `loadForEnv()` method introduced in #28533 by a new `loadEnv()` method.
- It accepts only one mandatory argument: `$path`, which is the path to the `.env` file.
- The 2nd argument is optional and defines the name of the environment variable that defines the Symfony env. This plays better with the current practice of defining the env in `.env` (`loadForEnv()` requires knowing the env before being called, leading to a chicken-n-egg situation that `loadEnv()` avoids.)
- the possibility to load several files at once is removed. We don't have a use case for it and those who do can call `loadEnv()` in a loop anyway.

In addition to $path (.env), the following files are loaded, the latter taking precedence in this order:
.env < env.local < .env.$env < .env.$env.local

Note that `loadForEnv()` used to give higher precedence to .env.local vs .env.$env.
The new behavior is aligned with [the order used by create-react-app](https://github.com/facebook/create-react-app/blob/master/docusaurus/docs/adding-custom-environment-variables.md#what-other-env-files-can-be-used). It also allows overriding the env in .env.local, which should be convenient for DX.

Last but not least, the "test" env has this special behaviors:
- `.env.local` file is skipped for the "test" env (same as before and as in create-react-app)
- ~vars defined in .env files **override** real env vars (similar to what Rails' dotenv does: you don't want your tests to randomly fail because of some real env vars)~.

Commits
-------

0cf9acb [Dotenv] add loadEnv(), a smoother alternative to loadForEnv()

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:dotenv-loadenv branch Nov 9, 2018

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