Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Nullable environment variable processor#29767
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
OskarStark 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.
I like the PR
src/Symfony/Component/DependencyInjection/Tests/EnvVarProcessorTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
bpolaszek commentedJan 3, 2019
Appveyor randomly fails on |
OskarStark commentedJan 3, 2019
You can have a look here:#29760 |
bpolaszek commentedJan 3, 2019
Reworded commit message and force pushed, everything's fine now 🎉 |
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| if ('nullable' ===$prefix) { | ||
| return'' ===$env ?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.
What if the env var is not defined at all? To me, not providing the env var meansnull.
Currently, you can already get anull behavior forFOO env var by setting aenv(FOO) parameter:
parameters:env(FOO):null
sonull is used as default value ifFOO env var is not set.
Don't we just need this allowing an env var not to be set to getnull, without defining a parameter as default?
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 think we should avoid
env(DEFAULT_NULL): null%env(DEFAULT_NULL)%competing with
%env(nullable:RUNTIME_ARBITRARY_NULL)%to take this further we could also deprecate setting null defaults, in favor of always usingnullable: + string values (related to#27808)
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.
Yes, my point is%env(nullable:FOO)% should be accepted even ifFOO is not set and noenv(FOO) parameter exists => injected value will benull.
Second point: not sure we need the empty string to be considerednull.
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.
My worry isRUNTIME_ENV= can only be nulled by removing it or move it to config.
Currently there's only one source where null can come from, and it's:env(DEFAULT): ~
Addingnullable creates 2 scenarios. Hence my suggestion to deprecateenv(DEFAULT): ~ in favor of
RUNTIME=env(DEFAULT): ""%env(nullable:DEFAULT)%%env(nullable:RUNTIME)%IMHO that's the easiest to reason about.
nicolas-grekasJan 27, 2019 • 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.
When the env var is not defined, we already added the "default" processor in#28976. I think this processor should do one thing, turn empty strings to null - what it does currently - and "default" covers the other use cases.
I also don't agree deprecatingenv(FOO): ~ is a good idea: we shouldn't deprecate for the sake of it.
nicolas-grekasJan 27, 2019 • 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.
Note that "default" has not been released yet so we can also alter its behavior.
But each processor should have one responsibility - we shouldn't have many processors doing the same in slightly different ways.
nicolas-grekas commentedJan 27, 2019
please rebase |
bpolaszek commentedJan 28, 2019
@nicolas-grekas Done. |
xabbuh 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.
Can you also add an entry to the component's changelog file please?
bpolaszek commentedJan 30, 2019
Sure. |
fabpot commentedFeb 13, 2019
Thank you@bpolaszek. |
This PR was merged into the 4.3-dev branch.Discussion----------Nullable environment variable processor| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR | todoHey there,Because environment variables are strings by design, empty environment variables are evaluated to `""` by default.In the same way, `MY_ENV_VAR=null` will be evaluated to `"null"`, as a string.What I suggest is to allow some environment variables to be evaluated to `null` (the real one) when their strings are _blank_ or equal _null_, _Null_ or _NULL_.This can be easily done through a new `nullable` processor:```bash# .envAPI_KEY=``````yaml# config/services.yamlservices: FooService: arguments: $apiKey: %env(nullable:API_KEY)%``````php# src/Services/FooServicenamespace App\Services;final class FooService{ /** *@var string|null */ private $apiKey; /** * FooService constructor. */ public function __construct(?string $apiKey) { $this->apiKey = $apiKey; } public function doSomething() { // Free plan if (null === $this->apiKey) { // ... } }}```That's an example that comes to my mind but there can be many others.This can also help in using null coalesce operators in constructors instead of checking if a string equals `""` (which is very PHP4 style).Of course it can be used in conjunction with other types, i.e. `%env(float:nullable:SOME_OPTIONAL_FLOAT_ENV_VAR)%`.What do you think?Commits-------3a604ac Nullable environment variable processor… "default" one (nicolas-grekas)This PR was merged into the 4.3-dev branch.Discussion----------[DI] replace "nullable" env processor by improving the "default" one| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Neither `nullable` nor `default` are released yet.I propose to replace the `nullable` processor (see#29767) with an improved `default` one (from#28976).`%env(default::FOO)%` now defaults to `null` when the env var doesn't exist or compares to false".ping@jderusse@bpolaszekCommits-------c50aad2 [DI] replace "nullable" env processor by improving the "default" one
Uh oh!
There was an error while loading.Please reload this page.
Hey there,
Because environment variables are strings by design, empty environment variables are evaluated to
""by default.In the same way,
MY_ENV_VAR=nullwill be evaluated to"null", as a string.What I suggest is to allow some environment variables to be evaluated to
null(the real one) when their strings areblank or equalnull,Null orNULL.This can be easily done through a new
nullableprocessor:# .envAPI_KEY=That's an example that comes to my mind but there can be many others.
This can also help in using null coalesce operators in constructors instead of checking if a string equals
""(which is very PHP4 style).Of course it can be used in conjunction with other types, i.e.
%env(float:nullable:SOME_OPTIONAL_FLOAT_ENV_VAR)%.What do you think?