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 a new loadForEnv() method mimicking Ruby's dotenv behavior #28533

Merged
merged 1 commit into from Oct 26, 2018

Conversation

@dunglas
Member

dunglas commented Sep 21, 2018

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, Rails' DotEnv 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... - Environment-specific settings.
  • .env.local - Local overrides. This file is loaded for all environments except test.
  • .env.dev.local, .env.test.local... - Local overrides of environment-specific settings.

The plan is to use this method in the default SF installation (symfony/recipes#466).

Note: this feature can also be used in prod if the app doesn’t follow the 12factor methodology (discouraged, but sometimes mandatory in constrained hosting environments not allowing to set environment variables).

@dunglas dunglas requested a review from ogizanagi Sep 21, 2018

@dunglas dunglas changed the title from [DotEnv] Add a new loadForEnv() method mimicing Ruby's dotenv behavior to [DotEnv] Add a new loadForEnv() method mimicking Ruby's dotenv behavior Sep 21, 2018

/**
* Loads one or several .env and the corresponding env.local, env.$env and env.$env.local files if they exist.
*
* The env.test.local is always ignored because tests should produce the same results for everyone.

This comment has been minimized.

@ogizanagi

ogizanagi Sep 21, 2018

Member

I don't think we should put any restriction and hardcode this. If that's really what the users want, that's on them. I'd remove it.

This comment has been minimized.

@dunglas

dunglas Sep 21, 2018

Member

I was hesitating, but Create React App, Rails and Sinatra behave like this. Consistency is the key, it helps switching from one project to another. It especially matters when using both React and SF in a project (more and more common).

And, it's a good idea!

I really don't want to introduce something Symfony-specific here!

This comment has been minimized.

@ogizanagi

ogizanagi Sep 21, 2018

Member

Why is .env.test.local mentioned in the following sentence, then ? 😄

.env.dev.local, .env.test.local, .env.prod.local... - Local overrides of environment-specific settings.

This comment has been minimized.

@dunglas

dunglas Sep 21, 2018

Member

@ogizanagi the comment was wrong 😁. Fixed!

/**
* Loads one or several .env and the corresponding env.local, env.$env and env.$env.local files if they exist.
*
* env.local is always ignored in test env because tests should produce the same results for everyone.

This comment has been minimized.

@ogizanagi

ogizanagi Sep 21, 2018

Member
- env.local
+ .env.local
// or
+ $path.local

? 😄

@@ -50,6 +51,33 @@ public function load(string $path, string ...$paths): void
$this->doLoad(false, $path, $paths);
}
/**
* Loads one or several .env and the corresponding env.local, env.$env and env.$env.local files if they exist.

This comment has been minimized.

@weaverryan

weaverryan Sep 21, 2018

Member

Environment variables in later files override earlier files, correct?

This comment has been minimized.

@dunglas

dunglas Sep 21, 2018

Member

Almost, see PR description.

@Wirone

This comment has been minimized.

Wirone commented Sep 25, 2018

I don't get what's the purpose of having .env.prod while at the same time discuraging or even blocking DotEnv usage on production 🤔 Also, development instances shouldn't use production configuration. .env.prod should not be commited, nor included in built Docker image. So what is the use case for it?

I don't like the idea of this PR. It makes simple task (app configuration) much more complicated and IMO doesn't bring added value (the only one I see is configuring variables for different environments and use it only by changing APP_ENV, without need to edit .env by commenting/uncommenting lines per environment). I thought that YAML files' split (packages in prod/dev subdirectories) and .env file were supposed to fulfill all the config scenarios. This is a step back for me.

But maybe I don't understand something..

@fbourigault

This comment has been minimized.

Contributor

fbourigault commented Sep 25, 2018

The first use case I have is to have on a local machine the dev and test environments running at the "same" time. I mainly have a different DATABASE_URL env variable which is probably different per developer and I don't want to use DATABSE_URL=... vendor/bin/simple-phpunit!

@dunglas

This comment has been minimized.

Member

dunglas commented Sep 25, 2018

@Wirone .env.prod is totally optional (for people not able to set environment variables in prod). This file will not be generated by the default skeleton (only .env.test will).
However it will make easier to use Dotenv in prod in cases were there is no alternatives (shared servers usually, and even for jobs in dedicated servers without Docker/K8S).

However, having .env.test, .env.panther, .env.otherenv on the development machine will be very useful (and it’s my main goal, as I use Kubernetes in prod, without env files obviously).

/**
* @param bool $envMode Silently ignore not existing files in $paths (but not the one in $path)
*/
private function doLoad(bool $overrideExistingVars, string $path, array $paths, bool $envMode = false): void

This comment has been minimized.

@Wirone

Wirone Sep 26, 2018

$envMode variable name does not correspond with what it does. Should be named $silent (as it's used internally) or $ignorePathErrors or something like that.

PS. phpDoc is missing the other arguments

This comment has been minimized.

@dunglas

dunglas Sep 26, 2018

Member

$silent or $ignorePathErrors is less accurate: an error will be thrown of the main file (usually .env) doesn’t exist. Only files for specific environments are optional.

Regarding the PHPDoc, in Symfony we don’t add the useless lines.

This comment has been minimized.

@Wirone

Wirone Sep 26, 2018

IMO $envMode is not meaningful. If you don't look at implementation, only on signature, would you know what $envMode does? Honestly, it doesn't even look like bool variable, rather string or bitwise flag.

This comment has been minimized.

@dunglas

dunglas Sep 26, 2018

Member

@Wirone I agree, it's why I added a PHPDoc description :) If you have better name in mind, I'll be very happy to switch ($silent and $ignorePathErrors were my first thoughts, but they are more confusing than $envMode).

This comment has been minimized.

@Wirone

Wirone Sep 26, 2018

What about $safe (determines if call is safe and will not throw an exception) or $required (determines if all paths should exist - it would require changing to $silent = !$required).

This comment has been minimized.

@dunglas

dunglas Sep 26, 2018

Member

IMO they are not more explicit, and can be confusing. environment mode is exactly what it is, and $ignoreFilesThatNotExistExceptTheFirstOne is a bit long. Isn't the description in the PHPDoc enough? This method is private, it will never be used by the end user, only by Symfony contributors (we may expect that they read the PHPDoc :D).

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 20, 2018

Member

doLoad(bool $overrideExistingVars, string $path, bool $ignoreMissingExtraPaths, string ...$extraPaths = array())?

This comment has been minimized.

@dunglas

dunglas Oct 21, 2018

Member

Not sure that using the splat operator is a good idea in this private method (we may add extra parameters at some point), also it is already used without the splat operator in other functions.

This comment has been minimized.

@dunglas

dunglas Oct 21, 2018

Member

parameter renamed

@chalasr

Finally! This will allow me to drop an extra class from all my enterprise projects.
👍 Thanks for raising

@teohhanhui

This comment has been minimized.

Contributor

teohhanhui commented Oct 4, 2018

I think what's missing from this picture is where .dist files fit in. We should not follow blindly the Create React App / Rails way, because it conflicts with our conventions.

@dunglas

This comment has been minimized.

Member

dunglas commented Oct 4, 2018

@teohhanhui the whole point of this PR is to switch from “our” conventions to the ones other projects follow.
Basically, .env.dist is now .env, and .env is now .env.local.

@teohhanhui

This comment has been minimized.

Contributor

teohhanhui commented Oct 5, 2018

That sounds like a really bad thing to me. We shouldn't just switch conventions like that. What's wrong with sticking with .dist? It makes it more explicit that it's a distribution file. Plus, this would not go well with Docker Compose (.env is special for Docker Compose).

What I'm trying to say is, we could stick with our conventions while having all the benefits of an extended .env support like in the other projects.

@dunglas

This comment has been minimized.

Member

dunglas commented Oct 5, 2018

This has already been discussed a lot in symfony/recipes#466 and related tickets. It actually makes it easier to deal with Docker Compose.

@teohhanhui

This comment has been minimized.

Contributor

teohhanhui commented Oct 5, 2018

I've read through some of the comments, but don't see how it makes it easier for anyone, compared to providing the same feature but sticking with the existing .dist convention.

If you check in .env, you'll have problems doing local overrides for Docker Compose. I'm not talking about env_file here, as the .env file is read by Docker Compose itself for special variables and variable substitution.

@teohhanhui

This comment has been minimized.

Contributor

teohhanhui commented Oct 5, 2018

It actually makes it easier to deal with Docker Compose.

We should make Docker Compose support .dist files instead, as we've discussed before. That will be a huge win for everyone.

@dunglas

This comment has been minimized.

Member

dunglas commented Oct 5, 2018

Most popular projects work like this, so it makes it easier for an user to use both Create React App and Symfony, or to switch with Rails projects.
It also solves unrelated SF issues such as the ability to use the same format to configure all environments, to have different set of default configs for specific environment (test, e2e...), and to easily execute console commands in a specific environment.

We should make Docker Compose support .dist files instead, as we've discussed before. That will be a huge win for everyone.

It's very unlikely that they'll accept a patch for something used only by Symfony. They would more likely accept a patch to always override the .env by env.local when available, and then support a bunch of existing projects.

Btw, Create React App works well with Docker Compose: facebook/create-react-app#982 (comment)
Way better than the current Symfony mess with .dist file (see the currently "broken" thing we have regarding env files in api-platform/api-platform to make both Docker Compose and Flex working, we don't have this problem with Create React App).

@dunglas

This comment has been minimized.

Member

dunglas commented Oct 5, 2018

If you check in .env, you'll have problems doing local overrides for Docker Compose. I'm not talking about env_file here, as the .env file is read by Docker Compose itself for special variables and variable substitution.

It only means that the .env file we generate must be compatible with Docker Compose: https://docs.docker.com/compose/env-file/
IIRC, we already had this issue with Flex, and Flex is already compatible with this.

Btw, versioning .env files is considered a best practice in every projects I know, except in Symfony.

@dunglas

This comment has been minimized.

Member

dunglas commented Oct 5, 2018

Btw, versioning .env files is considered a best practice in every projects I know, except in Symfony.

Not in Laravel actually: https://laravel.com/docs/5.7/configuration
At least, we'll not be accused of copying this time 😅

*
* @see https://github.com/bkeepers/dotenv#what-other-env-files-can-i-use
*/
public function loadForEnv(string $env, string $path, string ...$paths): void

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 21, 2018

Member

I'd suggest renaming $paths to $extraPaths everywhere in this PR

This comment has been minimized.

@dunglas

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Oct 24, 2018

public function loadForEnv(string $env, string $path, string ...$extraPaths): void
{
array_unshift($extraPaths, $path);
foreach ($extraPaths as $p) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 24, 2018

Member

$p should be renamed $path (as in doLoad)

This comment has been minimized.

@dunglas

dunglas Oct 24, 2018

Member

It triggers a PHPStorm warning, but done

@@ -405,16 +433,24 @@ private function createFormatException($message)
return new FormatException($message, new FormatExceptionContext($this->data, $this->path, $this->lineno, $this->cursor));
}
private function doLoad(bool $overrideExistingVars, string $path, array $paths): void
/**
* @param bool $envMode Silently ignore not existing files in $paths (but not the one in $path)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 24, 2018

Member

should be removed

{
array_unshift($paths, $path);
array_unshift($extraPaths, $path);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Oct 24, 2018

Member

I'd suggest moving this outside of the doLoad method - and have this method accept $paths directly.

This comment has been minimized.

@dunglas

dunglas Oct 24, 2018

Member

It means duplicating this line 3 times? Does is it worth it?

@nicolas-grekas

Final approval on my side, ready to go!

@fabpot

fabpot approved these changes Oct 26, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Oct 26, 2018

Thank you @dunglas.

@fabpot fabpot merged commit 774a78c into symfony:master Oct 26, 2018

3 checks passed

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

fabpot added a commit that referenced this pull request Oct 26, 2018

feature #28533 [DotEnv] Add a new loadForEnv() method mimicking Ruby'…
…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     <!-- see https://symfony.com/bc -->
| Deprecations? |no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| 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
}
/**
* Loads one or several .env and the corresponding env.$env, env.local and env.$env.local files if they exist.

This comment has been minimized.

@Tobion

Tobion Oct 29, 2018

Member

this seems to be missing the dots in front of the files like .env.$env

@dunglas dunglas deleted the dunglas:loadForEnv branch Oct 29, 2018

fabpot added a commit that referenced this pull request Oct 29, 2018

minor #29012 [DotEnv] Fix loadForEnv PHPDoc (dunglas)
This PR was merged into the 4.2-dev branch.

Discussion
----------

[DotEnv] Fix loadForEnv PHPDoc

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #28533 (review)
| License       | MIT
| Doc PR        | n/a

<!--
Write a short README entry for your feature/bugfix here (replace this comment block.)
This will help people understand your PR and can be used as a start of the Doc PR.
Additionally:
 - Bug fixes must be submitted against the lowest branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

c10710c [DotEnv] Fix loadForEnv PHPDoc
@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 30, 2018

did we really need this?

Why .env.prod over config/packages/prod? if you cant define real env vars those locations are effectively the same already... and you can already import a git ignored local parameter file.

the behavior of Create React App, Rails' DotEnv and probably some other libs:

so yeah.. curious if the config/packages/ convention didnt already provide such behavior in SF.

@jaikdean

This comment has been minimized.

jaikdean commented Oct 30, 2018

Is there a well-defined use case for this to explain why it's useful? The only ones I can think up are already solved by using parameters and environment variables as they are in Symfony 4.1, but I could well be missing something.

In my projects we have phpunit.xml.dist and .php_cs.dist files committed and their non-dist versions ignored, so the convention adopted from Rails etc just brings another convention into play, rather than replacing one. It's understandable if a lot of others are already used to this convention, but it's the first time I've seen it.

Just my 2 pence!

@weaverryan

This comment has been minimized.

Member

weaverryan commented Oct 30, 2018

This feature has a lot of options in it, but the original motivation was to help solve exactly one problem. Currently, you need to add all of your environment variables in phpunit.xml.dist. So, your environment variables are completely duplicated, even if you only want to change a few environment variables. This + this WIP recipe (symfony/recipes#466) will help make that happen (as you can just have a .env.dist). I think that, mostly, this change will not mean much to people - other than you don't need to duplicate your env vars in the test environment anymore and can instead create a .env.test with only the overrides.

Cheers!

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 30, 2018

still.. there's zero difference with doing

# config/packages/test/some.yaml
env(SOME): "default in test"

no?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 30, 2018

The value can be changed without invalidating the container, that's the diff :)

@weaverryan

This comment has been minimized.

Member

weaverryan commented Oct 30, 2018

That's a VERY fair point @ro0NL :). But I think setting a default in that way is a bit less obvious than having a .env.test that's created for you when installing the simple-phpunit recipe.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 30, 2018

@weaverryan right!, that's the 2nd difference: flex automation!

@ro0NL

This comment has been minimized.

Contributor

ro0NL commented Oct 30, 2018

understood. im just skeptical if the overhaul is worth it eventually, it's quite significant.

@ryancastle

This comment has been minimized.

ryancastle commented Oct 31, 2018

@weaverryan Thanks for this! We were starting to think we'd gone mad and had completely missed something.

This was referenced Nov 3, 2018

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()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment