Add a new Dotenv component #21234

fabpot · 2017-01-11T02:10:12Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	symfony/symfony-docs#7350

This introduces a new Dotnv Component that manages .env files. Read the referenced doc PR above for more information about usage:

But here, I want to explain the rationale behind creating such a component instead of reusing an existing one.

First, this version only implements what you can do in a "real" bash shell script (which is what a .env really is): so no value validation for instance (and anyway, an env var value is always a string). That's important as in production, we should use real env variables, and we don't have validation for them there;
It allows to only parse a file without populating the env variables (we have 3 stages: load parse and populate);
Strict implementation of what you can do in a .env file, same behavior as bash ($VAR and ${VAR} are supported for instance, executing commands as well);
Great error messages: I spent a lot of time being sure that error reporting is top notch;
Clean, simple, and straightforward code (small public API);
It only does .env management, there is no uneeded abstractions like being able to add an env variable directly (just use putenv);

There are some unimplemented features as I don't think they are needed and would increase the complexity of the code: several concatenated strings FOO='foo'"bar" for instance.

simensen · 2017-01-11T02:32:55Z

One issue I've had with most of the .env libraries is that if you create a .env.dist you are forced to copy it to .env before running the application otherwise it throws something like a PathException. What would be the best way to use this such that you don't have to have a .env defined and only load it if that path exists?

I would be easy enough to use try/catch, but might not be as elegant. With an application using something like app.php vs app_dev.php, it is easy to just put the .env loader into app_dev.php only, but some environments (like Envoyer & Forge) rely on using .env for configuration in production. It could be argued this is wrong (and the docs with this PR clearly state that it should not be used in production...) but not everyone sees it that way.

All-in-all, looks like a reasonable thing to add and reasons for not using prior art directly (proper bash support) seems legit.

fabpot · 2017-01-11T02:43:32Z

@simensen That's an excellent question... for which I have some answers. Basically, you need some env vars to be present. So, for a Symfony app, you might have something like this in your web front controller:

if (!getenv('APP_ENV')) {
    (new Dotenv())->load(__DIR__.'/../.env');
}

That way, you can define real env vars and we fallback automatically to a .env file if not present (and there, we do want to enforce it as we do need those env vars).

As you might imagine, this component is a prerequisite for Symfony Flex which automatically copies the .env.dist file to a .env file that you can then customize (during composer update/install); Flex also automatically adds env vars when adding some specific bundles like Doctrine or Swiftmailer. I still have one question in Symfony Flex: whether we want a .env file under the root directory or if we want an env file (not that it does not start with a dot) under the config directory for easy discovery (as some/most IDEs do not list dot files). I would prefer the second option but this is "less standard".

dzuelke · 2017-01-11T02:44:20Z

Please no using .dist automatically! If an app needs default values then those can be achieved using the 3.2 functionality for env.

fabpot · 2017-01-11T02:46:00Z

@dzuelke What do you mean by "automatically"?

javiereguiluz · 2017-01-11T07:34:08Z

src/Symfony/Component/Env/Tests/DotenvTest.php

+            array('FOO="  "', array('FOO' => '  ')),
+            array("FOO=\"bar\nfoo\"", array('FOO' => "bar\nfoo")),
+
+            // concataned values


concataned -> concatenated ?

hhamon · 2017-01-11T07:53:53Z

src/Symfony/Component/Env/Dotenv.php

+     *
+     * @param array An array of env variables
+     */
+    public function populate($values)


missing array typehint.

jderusse · 2017-01-11T07:44:04Z

src/Symfony/Component/Env/Dotenv.php

+            $this->cursor += 7;
+        }
+
+        $this->skipWhitespace();


could be moved in the if block

jderusse · 2017-01-11T07:50:57Z

src/Symfony/Component/Env/Dotenv.php

+            $this->skipComment();
+        } elseif ('"' === $this->data[$this->cursor]) {
+            ++$this->cursor;
+            while ('"' !== $this->data[$this->cursor] || '\\' === $this->data[$this->cursor - 1]) {


What if my value really finish with an \ like in export FOO="BAR\\"? or a concrete example PATH="c:\\"

linaori · 2017-01-11T08:47:13Z

I have the feeling that load(), populate() and parse() should be in a different class. I've got 0 experience with .env so I can't really comment on when you would use it, but I have the feeling this won't be in a service container anyway.

What about some static helper methods like creating a request if it's in the front controller?

Dotenv::load(...);
Dotenv::populate(...);
Dotenv::parse(...);

The parser itself should not be in the same class as the above 3 methods imo, it feels weird because the class has a state but the same instance can be re-used.

hhamon · 2017-01-11T09:04:32Z

The parser/lexer methods should be moved to a separate DotEnvParser/DotEnvLexer class as @iltar suggests. We can keep the Dotenv class for loading a file and populating the global variables.

goetas · 2017-01-11T09:28:10Z

src/Symfony/Component/Env/Dotenv.php

+ */
+final class Dotenv
+{
+    const VARNAME_REGEX = '[A-Z][A-Z0-9_]*';


In my experience an env variable can be lovercase. In my laptop:

$ env|egrep "^[a-z_]+=" rvm_bin_path=/home/goetas/.rvm/bin _system_type=Linux rvm_path=/home/user/.rvm rvm_prefix=/home/user _system_arch=x86_64 _system_version=14.04 rvm_version=1.26.11 (latest) _system_name=Ubuntu _=/usr/bin/env

This is correct, I'm using some lower-case as well.

rvanlaak · 2017-01-11T09:41:58Z

So if I understand Flex correctly, an enhancement would be that the component automatically adds *.env to the .gitignore file?

Next to that, will the values eventually be sent to the Opcache, or will they be part of the compiled container?

renan · 2017-01-11T10:03:00Z

src/Symfony/Component/Env/Dotenv.php

+    private $end;
+    private $state;
+    private $export;
+    private $values;


Could you add an accessor for the values property?
Reason being I very often don't populate back the environment ($_ENV, $_SERVER, etc) but instead just use as a configuration param bag.

Where do you want to access it? As it would be accessible via the request object already.

What (is the) request object?

Right now there is a Dotenv::populate() method which gets the parsed values and populate them into the environment.
I would like to be able to read the values being parsed from the .env file without having to modify my environment (read: $_ENV, $_SERVER), through something like Dotenv::getValues()

Use parse then, it returns the parsed values without populating them.

webda2l · 2017-01-11T12:52:08Z

src/Symfony/Component/Env/Dotenv.php

+        if ($this->cursor === $this->end) {
+            if ($this->export) {
+                throw new FormatException('Unable to unset an environment variable', $this->createFormatExceptionContext());
+            } else {


No else case required to throw the next condition

King2500 · 2017-01-11T13:34:07Z

Maybe DotEnv would be more clear than Dotenv ?

goetas · 2017-01-11T13:45:04Z

src/Symfony/Component/Env/Dotenv.php

+     * @throws FormatException when a file has a syntax error
+     * @throws PathException   when a file does not exist or is not readable
+     */
+    public function load(/*...$paths*/)


What about an array instead of variadic? Arrays are more easy to manipulate, validate and so on.. compared to multiple arguments functions

In php7 (not hhvm!) you can actually do load(string ...$paths). In fact, this is just an array, just like func_get_args()

I was talking more about invoking a the function... but will result in Env::load(...$paths);

so it should work 👍

jakzal · 2017-01-11T14:45:06Z

src/Symfony/Component/Env/Dotenv.php

+    /**
+     * Sets values as environment variables (via putenv, $_ENV, and $_SERVER).
+     *
+     * Not that existing environment variables are never overridden.


Typo: "Not that" => "Note that"

dzuelke · 2017-01-11T14:56:08Z

@fabpot I think I misunderstood @simensen's suggestion as "auto fall back to .env.dist if no .env present", so nevermind ;)

jakzal · 2017-01-11T15:04:14Z

src/Symfony/Component/Env/Dotenv.php

+            )
+        /x';
+
+        $env = array_replace($_ENV, $_SERVER, $this->values);


Is rewriting all the $_SERVER values to the $_ENV variable intended here? I can understand why this is done for $this->values.

Indeed, I don't remember why I did that, removed for now.

jakzal · 2017-01-11T15:10:10Z

src/Symfony/Component/Env/Dotenv.php

+            (\{)?                      # optional brace
+            ('.self::VARNAME_REGEX.')  # var name
+            (\})?                      # optional closing brace
+        /xi';


The regex is evaluated in case-insensitive mode, while it's case sensitive in the lexVarname() method.

lyrixx · 2017-01-11T15:12:37Z

src/Symfony/Component/Env/Tests/DotenvTest.php

+    public function getEnvDataWithFormatErrors()
+    {
+        $tests = array(
+            array('FOO=BAR BAZ', "A value containing spaces must be surrounded by quotes in \".env\" at line 1.\n...FOO=BAR BAZ...\n             ^ line 1 offset 11"),


yield FTW.

jakzal · 2017-01-11T15:14:53Z

src/Symfony/Component/Env/Dotenv.php

+    {
+        $name = trim(str_replace(array('export ', '\'', '"'), '', $name));
+
+        if (!preg_match('/^[A-Z0-9_]+$/i', $name)) {


Shouldn't this be the same regex as defined in the VARNAME_REGEX constant?

old code, not used anymore, removed :)

jakzal · 2017-01-11T15:24:20Z

src/Symfony/Component/Env/composer.json

+{
+    "name": "symfony/env",
+    "type": "library",
+    "description": "Register environment variables from a .env file",


"Register" => "Registers"?

fabpot · 2017-01-11T18:42:37Z

I don't understand why the lexer/parse should be in another class. The Dotenv class is all about parsing a .env file already.

josegonzalez · 2017-01-11T21:49:56Z

Any reason why you aren't using an existing library for this? For instance, the m1/env library is pretty feature complete and should handle everything you don't already implement.

I'd also say for actually loading the .env file, both my own library and the one by vlucas should more than handle any use case symfony users have.

jakzal · 2017-01-11T21:56:41Z

@josegonzalez the explanation is given in this issue's description. Also, both of the loading libraries you referenced require PHP 7 while Symfony still supports PHP 5.5.

fabpot · 2017-01-11T21:57:17Z

All comments addressed.

josegonzalez · 2017-01-11T22:00:55Z

Not speaking on behalf of vlucas, but:

The m1/env library addresses the whole "Implement .env file parsing to bash spec" issue.
My library does separate file loading, parsing, and population as you might want.
Pretty sure my library handles the user-facing portion in a similar manner as to what you are looking for.

The PHP version is a fair concern. I dropped 5.x support because mocking out php functions - apache_getenv - isn't possible until 7.x, though the previous version (2.1.) works just fine and has more or less all the same functionality.

Just curious though, it's obviously your framework so the decision is up to you. Nice to see other frameworks adopting .env support! 👍

fabpot · 2017-01-11T22:03:18Z

@josegonzalez At first, I wanted to use vlucas library, I found many issues, I started to fix them to submit PRs, and then I realized that I removed almost all the code. So, I decided to start from scratch instead.

Never heard of m1/env before, but at first glance it fails my first criteria which is to stick to only what bash can understand (the readme starts with unlike bash the assignment is pretty relaxed).

nicolas-grekas

LGTM. Did not read the parsing logic in deep details though :)

nicolas-grekas · 2017-01-11T21:52:55Z

src/Symfony/Component/Env/Dotenv.php

+    public function populate($values)
+    {
+        foreach ($values as $name => $value) {
+            if (getenv($name)) {


should we check _ENV before to save a fn call? that's what is done in Container when reading env vars.

nicolas-grekas · 2017-01-11T21:54:52Z

src/Symfony/Component/Env/Dotenv.php

+
+        // optional export
+        $this->export = false;
+        if ('export ' === substr($this->data, $this->cursor, 7)) {


nicolas-grekas · 2017-01-11T21:57:09Z

src/Symfony/Component/Env/Dotenv.php

+                    if ($this->cursor + 1 === $this->end) {
+                        break;
+                    }
+                    if ("'" === $this->data[$this->cursor + 1]) {


use !== + break, then move ++ out of indentation?

nicolas-grekas · 2017-01-11T21:59:59Z

src/Symfony/Component/Env/Dotenv.php

+
+    private function skipEmptyLines()
+    {
+        if (preg_match('/(\n+|^#[^\n]*(\n*|$))+/Asm', $this->data, $match, null, $this->cursor)) {


Asm => m only (+^at the beginning)? maybe more common?

nicolas-grekas · 2017-01-11T22:05:05Z

src/Symfony/Component/Env/Dotenv.php

+                throw new \LogicException('Resolving commands requires the Symfony Process component.');
+            }
+
+            $process = new Process('echo '.$matches[0], null, $env);


$_ENV, from which $env is sourced, is not populated by default on eg Ubuntu.
But Process 3.2 has a method to enable env inheriting, so that $env can be merged automatically.

why "echo" btw?

changed to use built-in features of Process

What would you use instead of echo?

nicolas-grekas · 2017-01-11T22:06:28Z

src/Symfony/Component/Env/Dotenv.php

+        }
+    }
+
+    private function resolveCommands($value)


we should maybe disable commands on Windows, because escaping is hard there, and it might come later?

nicolas-grekas · 2017-01-11T22:06:39Z

src/Symfony/Component/Env/Dotenv.php

+                return substr($matches[0], 1);
+            }
+
+            if (!class_exists('Symfony\Component\Process\Process')) {


Process::class

jderusse · 2017-01-11T23:17:53Z

src/Symfony/Component/Env/Dotenv.php

+            $this->skipComment();
+        } elseif ('"' === $this->data[$this->cursor]) {
+            ++$this->cursor;
+            while ('"' !== $this->data[$this->cursor] || ('\\' === $this->data[$this->cursor - 1] && '\\' !== $this->data[$this->cursor - 2])) {


Shouldn't we handle escaping? Or it doesn't worst it?

"Foo" => OK

"Foo\" => KO

"Foo\\" => OK

"Foo\\\" => KO <= this (edge) case look not handled by your implementation.

edit: github strip slashes in my previous comment

Not sure it's worth it :)

fabpot · 2017-01-11T23:52:37Z

failing tests not related

xabbuh · 2017-01-12T07:03:53Z

src/Symfony/Component/Env/Dotenv.php

+    public function populate($values)
+    {
+        foreach ($values as $name => $value) {
+            if (isset($_ENV[$name]) || false !== getenv($name)) {


Shouldn't we check for $_SERVER[$name] too?

$_SERVER mixes env + http headers + PHP special keys. This leads to complications that can open security issues. Better not IMHO.

I just think that people may be confused if they use an env var name that is not part of $_ENV, but exists in $_SERVER. It would then still be overridden though that we claim in the docblock that it wouldn't.

How can you have an env var is $_SERVER but not in $_ENV? I don't think that's possible.

fabpot · 2017-01-12T15:08:19Z

Regarding the main class name, people refer to this concept as dotenv (see Google). Naming the class DotEnv would mean that the component name would be dot-env which we don't want.

nicolas-grekas · 2017-01-12T16:41:10Z

👍

jakzal · 2017-01-12T17:40:36Z

👍

This PR was merged into the 3.3-dev branch. Discussion ---------- Add a new Dotenv component | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | symfony/symfony-docs#7350 This introduces a new Dotnv Component that manages `.env` files. Read the referenced doc PR above for more information about usage: But here, I want to explain the rationale behind creating such a component instead of reusing an existing one. * First, this version only implements what you can do in a "real" bash shell script (which is what a `.env` really is): so **no value validation** for instance (and anyway, an env var value is always a string). That's important as in production, we should use real env variables, and we don't have validation for them there; * It allows to only parse a file without populating the env variables (we have 3 stages: `load` `parse` and `populate`); * Strict implementation of what you can do in a `.env` file, same behavior as bash ($VAR and ${VAR} are supported for instance, executing commands as well); * Great error messages: I spent a lot of time being sure that error reporting is top notch; * Clean, simple, and straightforward code (small public API); * It only does `.env` management, there is no uneeded abstractions like being able to add an env variable directly (just use `putenv`); There are some unimplemented features as I don't think they are needed and would increase the complexity of the code: several concatenated strings `FOO='foo'"bar"` for instance. Commits ------- 5a6be8a [Dotenv] added the component

This PR was merged into the master branch. Discussion ---------- Add docs for the Dotenv component See symfony/symfony#21234 Commits ------- 03fda49 added docs for the env component

carsonbot added Status: Needs Review Feature labels Jan 11, 2017

fabpot mentioned this pull request Jan 11, 2017

Add docs for the Dotenv component symfony/symfony-docs#7350

Merged

fabpot force-pushed the dotenv branch 5 times, most recently from 3261280 to 7c0cb18 Compare January 11, 2017 03:30

javiereguiluz reviewed Jan 11, 2017

View reviewed changes

hhamon reviewed Jan 11, 2017

View reviewed changes

jderusse reviewed Jan 11, 2017

View reviewed changes

goetas reviewed Jan 11, 2017

View reviewed changes

renan reviewed Jan 11, 2017

View reviewed changes

webda2l reviewed Jan 11, 2017

View reviewed changes

goetas reviewed Jan 11, 2017

View reviewed changes

jakzal reviewed Jan 11, 2017

View reviewed changes

lyrixx reviewed Jan 11, 2017

View reviewed changes

jakzal reviewed Jan 11, 2017

View reviewed changes

fabpot force-pushed the dotenv branch from 39f790a to efcc938 Compare January 11, 2017 21:56

fabpot force-pushed the dotenv branch from efcc938 to f9c1e37 Compare January 11, 2017 22:03

nicolas-grekas reviewed Jan 11, 2017

View reviewed changes

fabpot force-pushed the dotenv branch 2 times, most recently from 508cb22 to 151af7d Compare January 11, 2017 23:17

jderusse reviewed Jan 11, 2017

View reviewed changes

fabpot force-pushed the dotenv branch 2 times, most recently from 2b289e0 to 117f405 Compare January 11, 2017 23:40

xabbuh reviewed Jan 12, 2017

View reviewed changes

fabpot force-pushed the dotenv branch from 117f405 to f448581 Compare January 12, 2017 15:06

fabpot force-pushed the dotenv branch from f448581 to fa4ffdf Compare January 12, 2017 16:37

fabpot changed the title ~~Add a new Env component~~ Add a new Dotenv component Jan 12, 2017

[Dotenv] added the component

5a6be8a

fabpot force-pushed the dotenv branch from fa4ffdf to 5a6be8a Compare January 12, 2017 16:39

fabpot merged commit 5a6be8a into symfony:master Jan 12, 2017

fabpot deleted the dotenv branch January 25, 2017 19:33

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Seb33300 mentioned this pull request Mar 28, 2018

Dotenv dependency laravel/framework#23715

Closed

Add a new Dotenv component #21234

Add a new Dotenv component #21234

Conversation

fabpot commented Jan 11, 2017 • edited Loading

simensen commented Jan 11, 2017

fabpot commented Jan 11, 2017

dzuelke commented Jan 11, 2017

fabpot commented Jan 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linaori commented Jan 11, 2017

hhamon commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvanlaak commented Jan 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

Choose a reason for hiding this comment

King2500 commented Jan 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

dzuelke commented Jan 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

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

fabpot commented Jan 11, 2017

josegonzalez commented Jan 11, 2017

jakzal commented Jan 11, 2017

fabpot commented Jan 11, 2017

josegonzalez commented Jan 11, 2017

fabpot commented Jan 11, 2017

nicolas-grekas 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

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

jderusse Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

fabpot Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

fabpot commented Jan 11, 2017

Choose a reason for hiding this comment

nicolas-grekas Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jan 12, 2017

nicolas-grekas commented Jan 12, 2017

jakzal commented Jan 12, 2017

fabpot commented Jan 11, 2017 •

edited

Loading

jderusse Jan 11, 2017 •

edited

Loading

fabpot Jan 11, 2017 •

edited

Loading

nicolas-grekas Jan 12, 2017 •

edited

Loading