Skip to content
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

[DI] Add a "default" EnvProcessor #28976

Merged
merged 1 commit into from Dec 1, 2018

Conversation

Projects
None yet
8 participants
@jderusse
Copy link
Contributor

commented Oct 25, 2018

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

This PR add a new fallback env processor in order to return a default value when the primary processor is not able to fetch a value (env variable, file or key does not exists)

# 
default_host: localhost
host: '%env(default:default_host:OPTIONAL_ENV_VARIABLE)%"

default_secret: this secret is not secret
secret: '%env(default:default_secret:file:THIS_FILE_ONLY_EXIST_IN_PRODUCTION)%"

default_charset: utf8
charset: '%env(default:default_charset:key:charset:json:DATABASE_CONFIG)%"

@jderusse jderusse force-pushed the jderusse:env-default branch from c196fa7 to e643fa8 Oct 25, 2018

@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

why not stick with env(SOME): 'default' in code?

@jderusse

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

@ro0NL because sometime the env variable exist, but the processing doesn't. See my examples.

secret: '%env(fallback:this secret is not secret:file:THIS_FILE_ONLY_EXIST_IN_PRODUCTION)%'
charset: '%env(fallback:utf8:key:charset:json:DATABASE_CONFIG)%'
@ro0NL

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

understood. Im a bit skeptical about making values part of the prefix string, but i cant think of anything better either.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

That won't work as the %env(...)% allows only a very limited set of characters between the brackets - on purpose. The fallback should be given as an environment variable. Or maybe a parameter name.
What about this: %env(catch:...:param)%? param would be the name of a parameter that would define the fallback value.

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 25, 2018

@jderusse jderusse force-pushed the jderusse:env-default branch from 15e6699 to ec186d2 Nov 5, 2018

4.3.0
-----

* added `%env(catch:...)%` processor to catch exception and fallback to a default value

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 5, 2018

Contributor

it must be (catch:...:DEFAULT_KEY) right

This comment has been minimized.

Copy link
@jderusse

jderusse Nov 5, 2018

Author Contributor

to be consistent with added %env(key:...)% processor

Show resolved Hide resolved src/Symfony/Component/DependencyInjection/EnvVarProcessor.php Outdated
Show resolved Hide resolved src/Symfony/Component/DependencyInjection/EnvVarProcessor.php Outdated
Show resolved Hide resolved .../DependencyInjection/Tests/Compiler/RegisterEnvVarProcessorsPassTest.php Outdated
}
$next = substr($name, 0, $i);
$default = substr($name, $i + 1);

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 5, 2018

Contributor

perhaps check to be non empty, im also worried it causes confusion as the last segment will always be used. If you forget it, another part will be implicitly used.

This comment has been minimized.

Copy link
@jderusse

jderusse Nov 5, 2018

Author Contributor

refactored the logic to default:fallback:origin in order to catch only NotFoundException and let RuntimeException bubble

Show resolved Hide resolved src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php Outdated

@jderusse jderusse force-pushed the jderusse:env-default branch from ec186d2 to 342ebdd Nov 5, 2018

@jderusse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

Thank your for the review @ro0NL , changed the logic in the PR to trap only the NotFound exceptions. Because it was a mistake to catch RuntimeException.
And move the fallback parameter next to the keyWord to be consistent with other processors

// previous
%env(catch:origin:fallback)%

// after
%env(default:fallback:origin)%
Show resolved Hide resolved src/Symfony/Component/DependencyInjection/EnvVarProcessor.php Outdated
Show resolved Hide resolved src/Symfony/Component/DependencyInjection/EnvVarProcessor.php Outdated
return $getEnv($next);
} catch (EnvNotFoundException $e) {
if (!$this->container->hasParameter($default)) {
throw new EnvNotFoundException(sprintf('Parameter "%s" not found. (fallback from not found "%s")', $default, $next), 0, $e);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 5, 2018

Member

sprintf('Invalid env fallback in "default:%s": parameter "%s" not found.', $name, $default)?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 5, 2018

Member

should it be a runtime exception btw?

This comment has been minimized.

Copy link
@jderusse

jderusse Nov 5, 2018

Author Contributor

fix message.
what's about default:my_fallback:default:optionnal_parameter_injected_by_third_party:foo?

Show resolved Hide resolved src/Symfony/Component/DependencyInjection/EnvVarProcessor.php Outdated
Show resolved Hide resolved src/Symfony/Component/DependencyInjection/EnvVarProcessor.php Outdated
Show resolved Hide resolved src/Symfony/Component/DependencyInjection/EnvVarProcessor.php Outdated
Show resolved Hide resolved .../DependencyInjection/Tests/Compiler/RegisterEnvVarProcessorsPassTest.php Outdated

@jderusse jderusse force-pushed the jderusse:env-default branch 2 times, most recently from b7e1db3 to 16461d6 Nov 5, 2018

@jderusse jderusse changed the title Add a fallback EnvProcessor [DI] Add a "default" EnvProcessor Nov 5, 2018

@jderusse jderusse force-pushed the jderusse:env-default branch from 16461d6 to 289f283 Nov 5, 2018

@ro0NL

ro0NL approved these changes Nov 5, 2018

@@ -18,8 +18,4 @@
*/
class EnvNotFoundException extends InvalidArgumentException

This comment has been minimized.

Copy link
@ro0NL

ro0NL Nov 5, 2018

Contributor

while at it.. extends RuntimeException?

This comment has been minimized.

Copy link
@jderusse

jderusse Nov 5, 2018

Author Contributor

If the variable is not found or a a key contain a typo, it make sens to be a logic exception.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

@jderusse your examples are great as always ... but would this feature require any changes in Symfony apps? (both when using it as a stand-alone component and when using it as part of a full-stack Symfony app). Thanks!

@jderusse

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

@javiereguiluz Same implementation thant #28975 😉
So the response is yes, it works out of the box

@nicolas-grekas
Copy link
Member

left a comment

please submit a doc PR or doc issue at least

@nicolas-grekas nicolas-grekas force-pushed the jderusse:env-default branch from 289f283 to aee4e33 Dec 1, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

Thank you @jderusse.

@nicolas-grekas nicolas-grekas merged commit aee4e33 into symfony:master Dec 1, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 1, 2018

feature #28976 [DI] Add a "default" EnvProcessor (jderusse)
This PR was squashed before being merged into the 4.3-dev branch (closes #28976).

Discussion
----------

[DI] Add a "default" EnvProcessor

| 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

This PR add a new fallback env processor in order to return a default value when the primary processor is not able to fetch a value (env variable, file or key does not exists)

```
#
default_host: localhost
host: '%env(default:default_host:OPTIONAL_ENV_VARIABLE)%"

default_secret: this secret is not secret
secret: '%env(default:default_secret:file:THIS_FILE_ONLY_EXIST_IN_PRODUCTION)%"

default_charset: utf8
charset: '%env(default:default_charset🔑charset:json:DATABASE_CONFIG)%"
```

Commits
-------

aee4e33 [DI] Add a \"default\" EnvProcessor

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jan 9, 2019

minor #10722 Add env default processor (jderusse)
This PR was merged into the master branch.

Discussion
----------

Add env default processor

This PR documents symfony/symfony#28976

Commits
-------

8d3afcc Add env default processor
@itscaro

This comment has been minimized.

Copy link

commented Jan 9, 2019

Hey guys, awesome work! thanks for this change.

But IMO, the syntax is a little bit illegible, if it would be great to use the syntax in shell %env(origin:-fallback)%

I can do a PR for that, wdyt?

fabpot added a commit that referenced this pull request Mar 10, 2019

feature #30504 [DI] replace "nullable" env processor by improving the…
… "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 @bpolaszek

Commits
-------

c50aad2 [DI] replace "nullable" env processor by improving the "default" one
@rubenrua

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

IMHO and my devops team default is a bad name. Looks like it returns the passed default value, not the value of other parameter. fallback is more coherent. (Check https://twig.symfony.com/doc/2.x/filters/default.html)

Let me know in a new pull request with this rename and creating a new default EnvProcessor.

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.