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

Nullable environment variable processor #29767

Merged
merged 1 commit into from Feb 13, 2019

Conversation

@bpolaszek
Copy link
Contributor

bpolaszek commented Jan 3, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR todo

Hey 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:

# .env
API_KEY=
# config/services.yaml
services:
    FooService:
        arguments:
            $apiKey: %env(nullable:API_KEY)%
# src/Services/FooService
namespace 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?

@bpolaszek bpolaszek force-pushed the bpolaszek:nullable-env-processor branch 2 times, most recently from 5662d14 to cfcb6d8 Jan 3, 2019

@OskarStark
Copy link
Contributor

OskarStark left a comment

I like the PR

@bpolaszek bpolaszek force-pushed the bpolaszek:nullable-env-processor branch from cfcb6d8 to 02d1445 Jan 3, 2019

@bpolaszek

This comment has been minimized.

Copy link
Contributor Author

bpolaszek commented Jan 3, 2019

Related tickets: #22151 #28618 #25129 #20434

@bpolaszek

This comment has been minimized.

Copy link
Contributor Author

bpolaszek commented Jan 3, 2019

Appveyor randomly fails on Symfony\Component\Process\Tests\ProcessTest::testWaitUntilSpecificOutput though this PR isn't related to this 😕

@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Jan 3, 2019

You can have a look here: #29760

@bpolaszek bpolaszek force-pushed the bpolaszek:nullable-env-processor branch from 02d1445 to 6629fd1 Jan 3, 2019

bpolaszek added a commit to bpolaszek/symfony that referenced this pull request Jan 3, 2019

@bpolaszek

This comment has been minimized.

Copy link
Contributor Author

bpolaszek commented Jan 3, 2019

Reworded commit message and force pushed, everything's fine now 🎉

@bpolaszek bpolaszek force-pushed the bpolaszek:nullable-env-processor branch 2 times, most recently from 43e2c23 to 8c279c4 Jan 8, 2019

@@ -195,6 +196,10 @@ public function getEnv($prefix, $name, \Closure $getEnv)
return str_getcsv($env);
}
if ('nullable' === $prefix) {
return '' === $env ? null : $env;

This comment has been minimized.

@ogizanagi

ogizanagi Jan 8, 2019

Member

What if the env var is not defined at all? To me, not providing the env var means null.

Currently, you can already get a null behavior for FOO env var by setting a env(FOO) parameter:

parameters:
    env(FOO): null

so null is used as default value if FOO env var is not set.

Don't we just need this allowing an env var not to be set to get null, without defining a parameter as default?

This comment has been minimized.

@ro0NL

ro0NL Jan 8, 2019

Contributor

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 using nullable: + string values (related to #27808)

This comment has been minimized.

@ogizanagi

ogizanagi Jan 8, 2019

Member

Yes, my point is %env(nullable:FOO)% should be accepted even if FOO is not set and no env(FOO) parameter exists => injected value will be null.

Second point: not sure we need the empty string to be considered null.

This comment has been minimized.

@ro0NL

ro0NL Jan 8, 2019

Contributor

My worry is RUNTIME_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): ~

Adding nullable creates 2 scenarios. Hence my suggestion to deprecate env(DEFAULT): ~ in favor of

RUNTIME=
env(DEFAULT): ""

%env(nullable:DEFAULT)%
%env(nullable:RUNTIME)%

IMHO that's the easiest to reason about.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 27, 2019

Member

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 deprecating env(FOO): ~ is a good idea: we shouldn't deprecate for the sake of it.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 27, 2019

Member

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 nicolas-grekas added this to the next milestone Jan 25, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 27, 2019

please rebase

@bpolaszek bpolaszek force-pushed the bpolaszek:nullable-env-processor branch 2 times, most recently from c738cba to 0d62884 Jan 28, 2019

@bpolaszek

This comment has been minimized.

Copy link
Contributor Author

bpolaszek commented Jan 28, 2019

@xabbuh

xabbuh approved these changes Jan 30, 2019

Copy link
Member

xabbuh left a comment

Can you also add an entry to the component's changelog file please?

@bpolaszek bpolaszek force-pushed the bpolaszek:nullable-env-processor branch from 0d62884 to 3a604ac Jan 30, 2019

@bpolaszek

This comment has been minimized.

Copy link
Contributor Author

bpolaszek commented Jan 30, 2019

Sure.

@fabpot

fabpot approved these changes Feb 13, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Feb 13, 2019

Thank you @bpolaszek.

@fabpot fabpot merged commit 3a604ac into symfony:master Feb 13, 2019

2 checks passed

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 Feb 13, 2019

feature #29767 Nullable environment variable processor (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        | todo

Hey 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
# .env
API_KEY=
```

```yaml
# config/services.yaml
services:
    FooService:
        arguments:
            $apiKey: %env(nullable:API_KEY)%
```
```php
# src/Services/FooService
namespace 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

@bpolaszek bpolaszek deleted the bpolaszek:nullable-env-processor branch Feb 13, 2019

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