[DI] Add a "default" EnvProcessor #28976

jderusse · 2018-10-25T07:15:22Z

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

ro0NL · 2018-10-25T07:35:07Z

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

jderusse · 2018-10-25T08:10:43Z

@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 · 2018-10-25T08:29:12Z

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

nicolas-grekas · 2018-10-25T13:28:42Z

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.

ro0NL · 2018-11-05T09:25:24Z

src/Symfony/Component/DependencyInjection/CHANGELOG.md

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


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

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

src/Symfony/Component/DependencyInjection/EnvVarProcessor.php

src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterEnvVarProcessorsPassTest.php

ro0NL · 2018-11-05T09:27:39Z

src/Symfony/Component/DependencyInjection/EnvVarProcessor.php

+            }
+
+            $next = substr($name, 0, $i);
+            $default = substr($name, $i + 1);


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.

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

src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php

jderusse · 2018-11-05T09:55:28Z

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

src/Symfony/Component/DependencyInjection/EnvVarProcessor.php

nicolas-grekas · 2018-11-05T10:00:36Z

src/Symfony/Component/DependencyInjection/EnvVarProcessor.php

+                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);


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

should it be a runtime exception btw?

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

src/Symfony/Component/DependencyInjection/EnvVarProcessor.php

src/Symfony/Component/DependencyInjection/Tests/Compiler/RegisterEnvVarProcessorsPassTest.php

ro0NL · 2018-11-05T13:22:27Z

src/Symfony/Component/DependencyInjection/Exception/EnvNotFoundException.php

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


while at it.. extends RuntimeException?

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

javiereguiluz · 2018-11-07T12:07:37Z

@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 · 2018-11-07T15:05:27Z

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

nicolas-grekas

please submit a doc PR or doc issue at least

nicolas-grekas · 2018-12-01T09:02:09Z

Thank you @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:key:charset:json:DATABASE_CONFIG)%" ``` Commits ------- aee4e33 [DI] Add a \"default\" EnvProcessor

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 · 2019-01-09T13:54:31Z

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?

@jderusse

… "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 · 2019-04-16T17:33:57Z

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.

carsonbot added Status: Needs Review Feature labels Oct 25, 2018

jderusse force-pushed the env-default branch from c196fa7 to e643fa8 Compare October 25, 2018 07:20

andersonamuller mentioned this pull request Oct 25, 2018

[DI] Add an url EnvProcessor #28975

Merged

xabbuh added the DependencyInjection label Oct 25, 2018

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

jderusse force-pushed the env-default branch from 15e6699 to ec186d2 Compare November 5, 2018 09:13

ro0NL reviewed Nov 5, 2018

View reviewed changes

jderusse force-pushed the env-default branch from ec186d2 to 342ebdd Compare November 5, 2018 09:52

nicolas-grekas reviewed Nov 5, 2018

View reviewed changes

jderusse force-pushed the env-default branch 2 times, most recently from b7e1db3 to 16461d6 Compare November 5, 2018 10:43

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

jderusse force-pushed the env-default branch from 16461d6 to 289f283 Compare November 5, 2018 13:05

nicolas-grekas approved these changes Nov 5, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 5, 2018

ro0NL approved these changes Nov 5, 2018

View reviewed changes

nicolas-grekas approved these changes Dec 1, 2018

View reviewed changes

[DI] Add a \"default\" EnvProcessor

aee4e33

nicolas-grekas force-pushed the env-default branch from 289f283 to aee4e33 Compare December 1, 2018 09:02

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

jderusse mentioned this pull request Dec 1, 2018

Add env default processor symfony/symfony-docs#10722

Merged

nicolas-grekas mentioned this pull request Jan 27, 2019

Nullable environment variable processor #29767

Merged

nicolas-grekas mentioned this pull request Mar 9, 2019

[DI] replace "nullable" env processor by improving the "default" one #30504

Merged

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

fabpot mentioned this pull request May 9, 2019

Release v4.3.0-BETA1 #31435

Merged

jderusse deleted the env-default branch August 2, 2019 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Add a "default" EnvProcessor #28976

[DI] Add a "default" EnvProcessor #28976

jderusse commented Oct 25, 2018 •

edited

Loading

ro0NL commented Oct 25, 2018

jderusse commented Oct 25, 2018

ro0NL commented Oct 25, 2018

nicolas-grekas commented Oct 25, 2018 •

edited

Loading

ro0NL Nov 5, 2018 •

edited

Loading

jderusse Nov 5, 2018 •

edited

Loading

ro0NL Nov 5, 2018

jderusse Nov 5, 2018

jderusse commented Nov 5, 2018

nicolas-grekas Nov 5, 2018

nicolas-grekas Nov 5, 2018

jderusse Nov 5, 2018

ro0NL Nov 5, 2018

jderusse Nov 5, 2018

javiereguiluz commented Nov 7, 2018

jderusse commented Nov 7, 2018

nicolas-grekas left a comment

nicolas-grekas commented Dec 1, 2018

itscaro commented Jan 9, 2019 •

edited

Loading

rubenrua commented Apr 16, 2019

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

[DI] Add a "default" EnvProcessor #28976

[DI] Add a "default" EnvProcessor #28976

Conversation

jderusse commented Oct 25, 2018 • edited Loading

ro0NL commented Oct 25, 2018

jderusse commented Oct 25, 2018

ro0NL commented Oct 25, 2018

nicolas-grekas commented Oct 25, 2018 • edited Loading

ro0NL Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

jderusse Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

ro0NL Nov 5, 2018

Choose a reason for hiding this comment

jderusse Nov 5, 2018

Choose a reason for hiding this comment

jderusse commented Nov 5, 2018

nicolas-grekas Nov 5, 2018

Choose a reason for hiding this comment

nicolas-grekas Nov 5, 2018

Choose a reason for hiding this comment

jderusse Nov 5, 2018

Choose a reason for hiding this comment

ro0NL Nov 5, 2018

Choose a reason for hiding this comment

jderusse Nov 5, 2018

Choose a reason for hiding this comment

javiereguiluz commented Nov 7, 2018

jderusse commented Nov 7, 2018

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Dec 1, 2018

itscaro commented Jan 9, 2019 • edited Loading

rubenrua commented Apr 16, 2019

jderusse commented Oct 25, 2018 •

edited

Loading

nicolas-grekas commented Oct 25, 2018 •

edited

Loading

ro0NL Nov 5, 2018 •

edited

Loading

jderusse Nov 5, 2018 •

edited

Loading

itscaro commented Jan 9, 2019 •

edited

Loading