Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Add a new Dotenv component#21234
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
simensen commentedJan 11, 2017
One issue I've had with most of the I would be easy enough to use All-in-all, looks like a reasonable thing to add and reasons for not using prior art directly (proper bash support) seems legit. |
fabpot commentedJan 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')) { (newDotenv())->load(__DIR__.'/../.env');} That way, you can define real env vars and we fallback automatically to a As you might imagine, this component is a prerequisite for Symfony Flex which automatically copies the |
dzuelke commentedJan 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 commentedJan 11, 2017
@dzuelke What do you mean by "automatically"? |
3261280 to7c0cb18Compare| array('FOO=" "', array('FOO' => ' ')), | ||
| array("FOO=\"bar\nfoo\"", array('FOO' => "bar\nfoo")), | ||
| // concataned values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
concataned ->concatenated ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
| * | ||
| * @param array An array of env variables | ||
| */ | ||
| public function populate($values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missingarray typehint.
src/Symfony/Component/Env/Dotenv.php Outdated
| $this->cursor += 7; | ||
| } | ||
| $this->skipWhitespace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
could be moved in theif block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
src/Symfony/Component/Env/Dotenv.php Outdated
| $this->skipComment(); | ||
| } elseif ('"' === $this->data[$this->cursor]) { | ||
| ++$this->cursor; | ||
| while ('"' !== $this->data[$this->cursor] || '\\' === $this->data[$this->cursor - 1]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What if my value really finish with an\ like inexport FOO="BAR\\"? or a concrete examplePATH="c:\\"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
linaori commentedJan 11, 2017
I have the feeling that What about some static helper methods like creating a request if it's in the front controller? 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 commentedJan 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. |
src/Symfony/Component/Env/Dotenv.php Outdated
| */ | ||
| final class Dotenv | ||
| { | ||
| const VARNAME_REGEX = '[A-Z][A-Z0-9_]*'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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=Linuxrvm_path=/home/user/.rvmrvm_prefix=/home/user_system_arch=x86_64_system_version=14.04rvm_version=1.26.11 (latest)_system_name=Ubuntu_=/usr/bin/env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is correct, I'm using some lower-case as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
rvanlaak commentedJan 11, 2017
So if I understand Flex correctly, an enhancement would be that the component automatically adds 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you add an accessor for thevalues property?
Reason being I very often don't populate back the environment ($_ENV,$_SERVER, etc) but instead just use as a configuration param bag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Where do you want to access it? As it would be accessible via therequest object already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What (is the)request object?
Right now there is aDotenv::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 likeDotenv::getValues()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Useparse then, it returns the parsed values without populating them.
src/Symfony/Component/Env/Dotenv.php Outdated
| if ($this->cursor === $this->end) { | ||
| if ($this->export) { | ||
| throw new FormatException('Unable to unset an environment variable', $this->createFormatExceptionContext()); | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No else case required to throw the next condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
King2500 commentedJan 11, 2017
Maybe |
| * @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*/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about an array instead of variadic? Arrays are more easy to manipulate, validate and so on.. compared to multiple arguments functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In php7 (not hhvm!) you can actually doload(string ...$paths). In fact, this is just an array, just likefunc_get_args()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I was talking more about invoking a the function... but will result inEnv::load(...$paths);
so it should work 👍
src/Symfony/Component/Env/Dotenv.php Outdated
| /** | ||
| * Sets values as environment variables (via putenv, $_ENV, and $_SERVER). | ||
| * | ||
| * Not that existing environment variables are never overridden. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Typo: "Not that" => "Note that"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
dzuelke commentedJan 11, 2017
src/Symfony/Component/Env/Dotenv.php Outdated
| ) | ||
| /x'; | ||
| $env = array_replace($_ENV, $_SERVER, $this->values); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is rewriting all the$_SERVER values to the$_ENV variable intended here? I can understand why this is done for$this->values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Indeed, I don't remember why I did that, removed for now.
src/Symfony/Component/Env/Dotenv.php Outdated
| (\{)? # optional brace | ||
| ('.self::VARNAME_REGEX.') # var name | ||
| (\})? # optional closing brace | ||
| /xi'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The regex is evaluated in case-insensitive mode, while it's case sensitive in thelexVarname() method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yield FTW.
src/Symfony/Component/Env/Dotenv.php Outdated
| { | ||
| $name = trim(str_replace(array('export ', '\'', '"'), '', $name)); | ||
| if (!preg_match('/^[A-Z0-9_]+$/i', $name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't this be the same regex as defined in theVARNAME_REGEX constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
old code, not used anymore, removed :)
| { | ||
| "name": "symfony/env", | ||
| "type": "library", | ||
| "description": "Register environment variables from a .env file", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"Register" => "Registers"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
fabpot commentedJan 11, 2017
I don't understand why the lexer/parse should be in another class. The |
josegonzalez commentedJan 11, 2017
Any reason why you aren't using an existing library for this? For instance, them1/env library is pretty feature complete and should handle everything you don't already implement. I'd also say for actuallyloading the |
jakzal commentedJan 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 commentedJan 11, 2017
All comments addressed. |
josegonzalez commentedJan 11, 2017
Not speaking on behalf of vlucas, but:
The PHP version is a fair concern. I dropped 5.x support because mocking out php functions - Just curious though, it's obviously your framework so the decision is up to you. Nice to see other frameworks adopting |
fabpot commentedJan 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 |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. Did not read the parsing logic in deep details though :)
src/Symfony/Component/Env/Dotenv.php Outdated
| public function populate($values) | ||
| { | ||
| foreach ($values as $name => $value) { | ||
| if (getenv($name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should we check _ENV before to save a fn call? that's what is done in Container when reading env vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
src/Symfony/Component/Env/Dotenv.php Outdated
| // optional export | ||
| $this->export = false; | ||
| if ('export ' === substr($this->data, $this->cursor, 7)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
strpos?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
src/Symfony/Component/Env/Dotenv.php Outdated
| if ($this->cursor + 1 === $this->end) { | ||
| break; | ||
| } | ||
| if ("'" === $this->data[$this->cursor + 1]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
use !== + break, then move ++ out of indentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok
src/Symfony/Component/Env/Dotenv.php Outdated
| private function skipEmptyLines() | ||
| { | ||
| if (preg_match('/(\n+|^#[^\n]*(\n*|$))+/Asm', $this->data, $match, null, $this->cursor)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Asm => m only (+^at the beginning)? maybe more common?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nope
src/Symfony/Component/Env/Dotenv.php Outdated
| throw new \LogicException('Resolving commands requires the Symfony Process component.'); | ||
| } | ||
| $process = new Process('echo '.$matches[0], null, $env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$_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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
changed to use built-in features of Process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What would you use instead of echo?
| } | ||
| } | ||
| private function resolveCommands($value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we should maybe disable commands on Windows, because escaping is hard there, and it might come later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
src/Symfony/Component/Env/Dotenv.php Outdated
| return substr($matches[0], 1); | ||
| } | ||
| if (!class_exists('Symfony\Component\Process\Process')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Process::class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
508cb22 to151af7dCompare| $this->skipComment(); | ||
| } elseif ('"' === $this->data[$this->cursor]) { | ||
| ++$this->cursor; | ||
| while ('"' !== $this->data[$this->cursor] || ('\\' === $this->data[$this->cursor - 1] && '\\' !== $this->data[$this->cursor - 2])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure it's worth it :)
2b289e0 to117f405Comparefabpot commentedJan 11, 2017
failing tests not related |
| public function populate($values) | ||
| { | ||
| foreach ($values as $name => $value) { | ||
| if (isset($_ENV[$name]) || false !== getenv($name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't we check for$_SERVER[$name] too?
nicolas-grekasJan 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$_SERVER mixes env + http headers + PHP special keys. This leads to complications that can open security issues. Better not IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
How can you have an env var is$_SERVER but not in$_ENV? I don't think that's possible.
fabpot commentedJan 12, 2017
Regarding the main class name, people refer to this concept as |
nicolas-grekas commentedJan 12, 2017
👍 |
1 similar comment
jakzal commentedJan 12, 2017
👍 |
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#7350This 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 componentSeesymfony/symfony#21234Commits-------03fda49 added docs for the env component
Uh oh!
There was an error while loading.Please reload this page.
This introduces a new Dotnv Component that manages
.envfiles. 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
.envreally is): sono 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:
loadparseandpopulate);Strict implementation of what you can do in a$VAR and $ {VAR} are supported for instance, executing commands as well);
.envfile, same behavior as bash (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
.envmanagement, there is no uneeded abstractions like being able to add an env variable directly (just useputenv);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.