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

Add a new Dotenv component #21234

Merged
merged 1 commit into from Jan 12, 2017

Conversation

@fabpot
Copy link
Member

fabpot commented Jan 11, 2017

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

This comment has been minimized.

Copy link
Contributor

simensen commented Jan 11, 2017

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

This comment has been minimized.

Copy link
Member

fabpot commented Jan 11, 2017

@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

This comment has been minimized.

Copy link
Contributor

dzuelke commented Jan 11, 2017

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

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 11, 2017

@dzuelke What do you mean by "automatically"?

@fabpot fabpot force-pushed the fabpot:dotenv branch 5 times, most recently from 3261280 to 7c0cb18 Jan 11, 2017

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

This comment has been minimized.

@javiereguiluz

javiereguiluz Jan 11, 2017

Member

concataned -> concatenated ?

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

fixed

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

This comment has been minimized.

@hhamon

hhamon Jan 11, 2017

Contributor

missing array typehint.

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

This comment has been minimized.

@jderusse

jderusse Jan 11, 2017

Contributor

could be moved in the if block

This comment has been minimized.

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

This comment has been minimized.

@jderusse

jderusse Jan 11, 2017

Contributor

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

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

fixed

@linaori

This comment has been minimized.

Copy link
Contributor

linaori commented Jan 11, 2017

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

This comment has been minimized.

Copy link
Contributor

hhamon commented Jan 11, 2017

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.

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

This comment has been minimized.

@goetas

goetas Jan 11, 2017

Contributor

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 comment has been minimized.

@linaori

linaori Jan 11, 2017

Contributor

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

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

fixed

@rvanlaak

This comment has been minimized.

Copy link
Contributor

rvanlaak commented Jan 11, 2017

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?

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

This comment has been minimized.

@renan

renan Jan 11, 2017

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.

This comment has been minimized.

@rvanlaak

rvanlaak Jan 11, 2017

Contributor

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

This comment has been minimized.

@renan

renan Jan 11, 2017

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()

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

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

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

This comment has been minimized.

@webda2l

webda2l Jan 11, 2017

No else case required to throw the next condition

This comment has been minimized.

@fabpot
@King2500

This comment has been minimized.

Copy link
Contributor

King2500 commented Jan 11, 2017

Maybe DotEnv would be more clear than Dotenv ?

* @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*/)

This comment has been minimized.

@goetas

goetas Jan 11, 2017

Contributor

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

This comment has been minimized.

@linaori

linaori Jan 11, 2017

Contributor

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

This comment has been minimized.

@goetas

goetas Jan 11, 2017

Contributor

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

so it should work 👍

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

This comment has been minimized.

@jakzal

jakzal Jan 11, 2017

Member

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

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

fixed

@dzuelke

This comment has been minimized.

Copy link
Contributor

dzuelke commented Jan 11, 2017

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

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

This comment has been minimized.

@jakzal

jakzal Jan 11, 2017

Member

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

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

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

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

This comment has been minimized.

@jakzal

jakzal Jan 11, 2017

Member

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

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

fixed

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"),

This comment has been minimized.

@lyrixx

lyrixx Jan 11, 2017

Member

yield FTW.

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

This comment has been minimized.

@jakzal

jakzal Jan 11, 2017

Member

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

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

old code, not used anymore, removed :)

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

This comment has been minimized.

@jakzal

jakzal Jan 11, 2017

Member

"Register" => "Registers"?

This comment has been minimized.

@fabpot
@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 11, 2017

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

This comment has been minimized.

Copy link

josegonzalez commented Jan 11, 2017

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.

@fabpot fabpot force-pushed the fabpot:dotenv branch from 39f790a to efcc938 Jan 11, 2017

@jakzal

This comment has been minimized.

Copy link
Member

jakzal commented Jan 11, 2017

@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

This comment has been minimized.

Copy link
Member

fabpot commented Jan 11, 2017

All comments addressed.

@josegonzalez

This comment has been minimized.

Copy link

josegonzalez commented Jan 11, 2017

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

This comment has been minimized.

Copy link
Member

fabpot commented Jan 11, 2017

@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).

@fabpot fabpot force-pushed the fabpot:dotenv branch from efcc938 to f9c1e37 Jan 11, 2017

@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

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

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 11, 2017

Member

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

This comment has been minimized.

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

This comment has been minimized.

@nicolas-grekas

This comment has been minimized.

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 11, 2017

Member

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

This comment has been minimized.

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 11, 2017

Member

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

This comment has been minimized.

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 11, 2017

Member

$_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?

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

changed to use built-in features of Process

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

What would you use instead of echo?

}
}
private function resolveCommands($value)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 11, 2017

Member

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

This comment has been minimized.

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 11, 2017

Member

Process::class

This comment has been minimized.

@fabpot

@fabpot fabpot force-pushed the fabpot:dotenv branch 2 times, most recently from 508cb22 to 151af7d Jan 11, 2017

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

This comment has been minimized.

@jderusse

jderusse Jan 11, 2017

Contributor

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

This comment has been minimized.

@fabpot

fabpot Jan 11, 2017

Member

Not sure it's worth it :)

@fabpot fabpot force-pushed the fabpot:dotenv branch 2 times, most recently from 2b289e0 to 117f405 Jan 11, 2017

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 11, 2017

failing tests not related

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

This comment has been minimized.

@xabbuh

xabbuh Jan 12, 2017

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 12, 2017

Member

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

This comment has been minimized.

@xabbuh

xabbuh Jan 12, 2017

Member

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.

This comment has been minimized.

@fabpot

fabpot Jan 12, 2017

Member

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

@fabpot fabpot force-pushed the fabpot:dotenv branch from 117f405 to f448581 Jan 12, 2017

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 12, 2017

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.

@fabpot fabpot force-pushed the fabpot:dotenv branch from f448581 to fa4ffdf Jan 12, 2017

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

@fabpot fabpot force-pushed the fabpot:dotenv branch from fa4ffdf to 5a6be8a Jan 12, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 12, 2017

👍

1 similar comment
@jakzal

This comment has been minimized.

Copy link
Member

jakzal commented Jan 12, 2017

👍

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

2 of 3 checks passed

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

fabpot added a commit that referenced this pull request Jan 12, 2017

feature #21234 Add a new Dotenv component (fabpot)
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

xabbuh added a commit to symfony/symfony-docs that referenced this pull request Jan 24, 2017

feature #7350 Add docs for the Dotenv component (fabpot)
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

@fabpot fabpot deleted the fabpot:dotenv branch Jan 25, 2017

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

@Seb33300 Seb33300 referenced this pull request Mar 28, 2018

Closed

Dotenv dependency #23715

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