[DI] Allow processing env vars #23901

nicolas-grekas · 2017-08-16T14:41:03Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	see description
License	MIT
Doc PR	-

This PR is an updated version of #20276 ~~(it embeds #23899 for now.)~~

It superscedes/closes:

[DI] Add support for secrets [DI] Add support for secrets #23621 ping @dunglas
Runtime container parameter not found event filter Runtime container parameter not found event filter #23669 ping @marfillaster
[DependencyInjection] [DX] Support for JSON string environment variables [DependencyInjection] [DX] Support for JSON string environment variables #23823 ping @Pierstoval
add support for composite environment variables add support for composite environment variables #17689 ping @greg0ire
[DI] ENV parameters at runtime with PHP 7 strict types not working properly [DI] ENV parameters at runtime with PHP 7 strict types not working properly #20434 ping @sandrokeil
Issue when using a SQLite database and the DATABASE_URL env var Issue when using a SQLite database and the DATABASE_URL env var #23527 ping @javiereguiluz

#22151 is another story, so not fixed here.

The way it works is via %env(foo:BAR)% prefixes, where "foo" can be bound to any services you'd like.
By default, the following prefixes are supported:

bool, int, float, string, base64
const (for referencing PHP constants)
json (supporting only json arrays for type predictability)
file (eg for access to secrets stored in files.)
resolve (for processing parameters inside env vars.)

New prefixes can be added by implementing the new EnvProviderInterface, and tagging with container.env_provider (see Rot13EnvProvider in tests.)

Prefixes can be combined to chain processing, eg.
%env(json:base64:file:FOO)% will be roughly equivalent to
json_decode(base64_decode(file_get_content(getenv('FOO')))).

marfillaster · 2017-08-16T14:52:29Z

👍 I totally approve this solution. The chained processing is just genius.

ro0NL · 2017-08-16T15:53:40Z

src/Symfony/Component/DependencyInjection/EnvProviderInterface.php

+     *
+     * @throws RuntimeException on error
+     */
+    public function getEnv($prefix, $name, \Closure $getEnv);


preferably the namespace parsing happens in Container::getEnv no? Basically you need to do the same check each time implementing it. What about getEnv($prefix, $name, $value, \Closure $getEnv = null)?

namespace parsing already happens in Container::getEnv.
What you see here is forwarding, which is something not all providers are required to do.

never mind, thats simply $value = $getEnv($name) currently. Basically the trick is to not forget return $getEnv($name) for defaulting.

and i missed your comment in between.. but you're right the current implem is fine.

yceruto · 2017-08-16T16:06:12Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

                        $resolved = sprintf($format, $env);
+                    } elseif ($placeholder === $resolved = $bag->escapeValue($this->getEnv($env))) {
+                        $resolved = $bag->all()[strtolower("env($env)")];


Can we fix #23527 (comment) somehow?

@yceruto here we are, with a new resolve handler to the rescue.

mnapoli · 2017-08-16T16:14:37Z

Is %env(json:base64:file:FOO)% really more understandable than json_decode(base64_decode(file_get_content(getenv('FOO'))))?
This is yet another custom syntax that Symfony users will need to learn, whereas plain old PHP would do the same with a few more characters. Maybe these kind of problems could be solved with #22407/#23834 and allowing to write some entries as PHP closures? This is a more complex solution though ("easy to say"), but the end result would be maybe much simpler for users?

nicolas-grekas · 2017-08-16T17:09:15Z

@mnapoli I think you're forgetting about how things work. Despite your claim in #22407, a PHP DSL would not help at all reading from the env. The reason is compilation of the container. The DSL is read once, at compile time. %env(FOO)% is the syntax that we already use to be able to declare that intention of compiling down to dynamic calls. The namespaced env vars only build on top of this to add more processing capabilities to env, nothing more. It's not only about syntactic sugar.

mnapoli · 2017-08-16T17:22:58Z

👍 sorry :)

Pierstoval · 2017-08-16T17:48:52Z

src/Symfony/Component/DependencyInjection/Compiler/RegisterEnvProvidersPass.php

+                if (isset($attr['prefix'])) {
+                    $providers[$attr['prefix']] = new Reference($id);
+                } elseif (!$r = $container->getReflectionClass($class = $container->getDefinition($id)->getClass())) {
+                    throw new InvalidArgumentException(sprintf('Class "%s" used for service "%s" cannot be found.', $class, $id));


Does this mean that the service class existence is not resolved at compile time?

Yes, as done commonly in other passes: when it's not required at compile time, we usually don't validate that kind of things.

nicolas-grekas · 2017-08-16T18:06:22Z

src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php

@@ -43,6 +43,7 @@ public function __construct()
            100 => array(
                $resolveClassPass = new ResolveClassPass(),
                new ResolveInstanceofConditionalsPass(),
+                new RegisterEnvProvidersPass(),


registered before optimization passes so that env values can be used at compile time (typically using $container->resolveEnvPlaceholders().) This makes env providers a bit less flexible (they can't easily be manipulated by DI extensions) but that's consistent with the provided feature to me (env vars should be light to fetch.)

Pierstoval · 2017-08-17T14:47:46Z

I cannot agree more on this PR 👍

ro0NL · 2017-08-17T16:28:00Z

The str_ireplace seems to trigger an unexpected Array to string conversion

$_ENV['REAL'] = '{"real":true}';
$c = new ContainerBuilder();
$c->setParameter('json_real', '%env(json:REAL)%');

$c->register('foo', 'stdClass')->setProperties(array(
    'json_real' => '%json_real%',
));

$c->compile(true);

Somehow real and default envs get mixed up, probably due reference generation in EnvBag::get()

$_ENV['REAL'] = 'runtime';
$c = new ContainerBuilder();
$c->setParameter('env(REAL)', 'compiletime');
$c->setParameter('real', '%env(REAL)%');

$c->register('teatime', 'stdClass')->setProperties(array(
    'real' => '%real%',
    'real_inline' => '%env(REAL)%',
));

$c->compile(true);

dump($c->get('teatime'));

{#136
  +"real": "runtime"
  +"real_inline": "compiletime"
}

This looks bad 🤔

What im worried about is the current default strategy. With this feature things may work unexpected due the exact lookup. setParameter('env(REAL)', ...) will not be used if combined with any prefix, you need to expose all variants :(

Also related; if in above example you leave out the default ("compiletime"); you'll get

The service "teatime" has a dependency on a non-existent parameter "env(real)"

Yet we did a real lookup already.. so what's the deal here?

Otherwise it seems to work just great :)

nicolas-grekas · 2017-08-17T20:38:18Z

@ro0NL thanks for the detailed report. Good news, both (the 3 in fact) your examples apply to 3.3 also, which means you found bugs. Nothing related to this PR specifically. Those happen only in compile(true) mode, which I'm going to fix in another PR on 3.3.

kbond · 2017-08-17T20:43:56Z

What is the reason for no bool prefix?

nicolas-grekas · 2017-08-18T07:18:04Z

@kbond no reason, fixed.
Now with a new const filter also.

fabpot · 2017-09-06T01:28:39Z

src/Symfony/Component/DependencyInjection/CHANGELOG.md

@@ -4,6 +4,7 @@ CHANGELOG
 3.4.0
 -----

+ * added `EnvProviderInterface` and corresponding "container.env_provider" tag for processing env vars


I don't like the "provider" name here as implementations do not provide anything. They process env vars, they filter them (Twig's name). So, I would use something like EnvProcessorInterface and container.env_processor.

To be even more precise, it's processors for env vars. So, could be EnvVarProcessorInterface.

fabpot · 2017-09-06T01:35:52Z

src/Symfony/Component/DependencyInjection/EnvProvider.php

+
+    private static function phpize($value)
+    {
+        if (!class_exists(XmlUtils::class)) {


Probably a smell that this class should be renamed, or at least some part of it extracted into a more generic class.

not a blocker here, and no better idea :)

fabpot · 2017-09-06T01:38:53Z

src/Symfony/Component/DependencyInjection/EnvProvider.php

+        $i = strpos($name, ':');
+
+        if ('file' === $prefix) {
+            if (0 < $i && strspn($name, '0123456789') === $i) {


Does support for a length really useful? What is the use case? Apart from the unit test that uses /dev/urandom?

fabpot · 2017-09-06T21:22:37Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -36,6 +36,7 @@
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\EnvProviderInterface;


Should be EnvVarProcessorInterface (same below)

fabpot · 2017-09-06T21:23:02Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -283,6 +284,8 @@ public function load(array $configs, ContainerBuilder $container)
            ->addTag('console.command');
        $container->registerForAutoconfiguration(ResourceCheckerInterface::class)
            ->addTag('config_cache.resource_checker');
+        $container->registerForAutoconfiguration(EnvProviderInterface::class)
+            ->addTag('container.env_provider');


should be renamed as well

fabpot · 2017-09-06T21:23:44Z

Still some provider usages (like variable names for instance).

fabpot · 2017-09-07T14:25:01Z

Thank you @nicolas-grekas.

@dunglas

This PR was merged into the 3.4 branch. Discussion ---------- [DI] Allow processing env vars | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | see description | License | MIT | Doc PR | - This PR is an updated version of #20276 ~~(it embeds #23899 for now.)~~ It superscedes/closes: - [DI] Add support for secrets #23621 ping @dunglas - Runtime container parameter not found event filter #23669 ping @marfillaster - [DependencyInjection] [DX] Support for JSON string environment variables #23823 ping @Pierstoval - add support for composite environment variables #17689 ping @greg0ire - [DI] ENV parameters at runtime with PHP 7 strict types not working properly #20434 ping @sandrokeil - Issue when using a SQLite database and the DATABASE_URL env var #23527 ping @javiereguiluz #22151 is another story, so not fixed here. The way it works is via `%env(foo:BAR)%` prefixes, where "foo" can be bound to any services you'd like. By default, the following prefixes are supported: - `bool`, `int`, `float`, `string`, `base64` - `const` (for referencing PHP constants) - `json` (supporting only json **arrays** for type predictability) - `file` (eg for access to secrets stored in files.) - `resolve` (for processing parameters inside env vars.) New prefixes can be added by implementing the new `EnvProviderInterface`, and tagging with `container.env_provider` (see `Rot13EnvProvider` in tests.) Prefixes can be combined to chain processing, eg. `%env(json:base64:file:FOO)%` will be roughly equivalent to `json_decode(base64_decode(file_get_content(getenv('FOO'))))`. Commits ------- 1f92e45 [DI] Allow processing env vars

sroze · 2017-09-12T06:39:58Z

Great job @nicolas-grekas 👏

…ameter (yceruto) This PR was merged into the master branch. Discussion ---------- Workaround to use DATABASE_URL with %kernel.project_dir% parameter Setting `DATABASE_URL=sqlite:///var/data/blog.sqlite` as default, causes this known error through `index.php`: ``` An exception occurred in driver: SQLSTATE[HY000] [14] unable to open database file ``` So the right relative path in this case should be `sqlite:///../var/data/blog.sqlite` BUT then it wouldn't work for `bin/console` entries. Then, relative paths shouldn't be the right way but absolute paths. The right thing is that soon (in 3.4 & 4.0) we can use environment variables with configuration parameters symfony/symfony#23901, so the final configuration would be the one proposed. For now a workaround to make both scenarios work `index.php` and `bin/console`. Commits ------- e45b5d5 Workaround to use DATABASE_URL with %kernel.project_dir% parameter

nicolas-grekas added this to the 3.4 milestone Aug 16, 2017

carsonbot added Status: Needs Review DependencyInjection Feature labels Aug 16, 2017

nicolas-grekas changed the title ~~[DI] Allow processing env vars - handles int/float/file/array/base64 + "container.env_provider"-tagged services~~ [DI] Allow processing env vars - handles int/float/file/json/base64 + "container.env_provider"-tagged services Aug 16, 2017

nicolas-grekas force-pushed the env-provider branch 4 times, most recently from ab01c8d to d75dcae Compare August 16, 2017 15:13

nicolas-grekas mentioned this pull request Aug 16, 2017

[DI] Validate env vars in config #23888

Closed

nicolas-grekas force-pushed the env-provider branch from d75dcae to f3144e1 Compare August 16, 2017 15:50

ro0NL reviewed Aug 16, 2017

View reviewed changes

yceruto reviewed Aug 16, 2017

View reviewed changes

Pierstoval reviewed Aug 16, 2017

View reviewed changes

nicolas-grekas mentioned this pull request Aug 16, 2017

Issue when using a SQLite database and the DATABASE_URL env var #23527

Closed

nicolas-grekas force-pushed the env-provider branch from f3144e1 to 62678b9 Compare August 16, 2017 17:57

nicolas-grekas changed the title ~~[DI] Allow processing env vars - handles int/float/file/json/base64 + "container.env_provider"-tagged services~~ [DI] Allow processing env vars Aug 16, 2017

nicolas-grekas force-pushed the env-provider branch from 62678b9 to 2baa573 Compare August 16, 2017 18:03

nicolas-grekas commented Aug 16, 2017

View reviewed changes

nicolas-grekas force-pushed the env-provider branch from 2baa573 to 751e5f8 Compare August 17, 2017 13:48

nicolas-grekas force-pushed the env-provider branch from 751e5f8 to 6e10746 Compare August 18, 2017 07:18

fabpot requested changes Sep 6, 2017

View reviewed changes

nicolas-grekas force-pushed the env-provider branch 5 times, most recently from 26c28eb to 019a582 Compare September 6, 2017 21:02

fabpot requested changes Sep 6, 2017

View reviewed changes

fabpot reviewed Sep 6, 2017

View reviewed changes

nicolas-grekas force-pushed the env-provider branch from 019a582 to 2499a76 Compare September 7, 2017 06:06

[DI] Allow processing env vars

1f92e45

nicolas-grekas force-pushed the env-provider branch from 2499a76 to 1f92e45 Compare September 7, 2017 06:08

fabpot merged commit 1f92e45 into symfony:3.4 Sep 7, 2017

nicolas-grekas deleted the env-provider branch September 10, 2017 16:11

ro0NL mentioned this pull request Sep 10, 2017

Update %env()% documentation symfony/symfony-docs#8382

Closed

fabpot mentioned this pull request Sep 11, 2017

Added a note about container parameters and env vars symfony/symfony-docs#8181

Closed

teohhanhui mentioned this pull request Sep 11, 2017

[lexik/jwt-authentication-bundle] gitignore JWT keys symfony/recipes#89

Closed

dunglas mentioned this pull request Sep 20, 2017

[FrameworkBundle] Add a env vars to configure trusted proxies and hosts symfony/recipes#182

Merged

mathroc mentioned this pull request Sep 21, 2017

using env var fails in framework.translator.paths (cache) #24270

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

peter-gribanov mentioned this pull request Apr 5, 2018

[DI] Environment variables lack type #26812

Closed

marfillaster mentioned this pull request Apr 20, 2018

Runtime container parameter not found event filter #23669

Closed

ro0NL mentioned this pull request Jul 2, 2018

[DI] Deprecate non-string default envs #27808

Merged

bendavies mentioned this pull request Jul 27, 2018

Feature Request: Enhance Env var processing with recursive env var resolution #28078

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Allow processing env vars #23901

[DI] Allow processing env vars #23901

nicolas-grekas commented Aug 16, 2017 •

edited

Loading

marfillaster commented Aug 16, 2017

ro0NL Aug 16, 2017

nicolas-grekas Aug 16, 2017 •

edited

Loading

ro0NL Aug 16, 2017

ro0NL Aug 16, 2017

yceruto Aug 16, 2017 •

edited

Loading

nicolas-grekas Aug 16, 2017 •

edited

Loading

mnapoli commented Aug 16, 2017 •

edited

Loading

nicolas-grekas commented Aug 16, 2017 •

edited

Loading

mnapoli commented Aug 16, 2017

Pierstoval Aug 16, 2017

nicolas-grekas Aug 16, 2017

nicolas-grekas Aug 16, 2017 •

edited

Loading

Pierstoval commented Aug 17, 2017

ro0NL commented Aug 17, 2017 •

edited

Loading

nicolas-grekas commented Aug 17, 2017

kbond commented Aug 17, 2017

nicolas-grekas commented Aug 18, 2017

fabpot Sep 6, 2017

fabpot Sep 6, 2017

nicolas-grekas Sep 6, 2017

fabpot Sep 6, 2017

nicolas-grekas Sep 6, 2017

fabpot Sep 6, 2017

nicolas-grekas Sep 6, 2017

fabpot Sep 6, 2017

fabpot Sep 6, 2017

fabpot commented Sep 6, 2017

fabpot commented Sep 7, 2017

sroze commented Sep 12, 2017

[DI] Allow processing env vars #23901

[DI] Allow processing env vars #23901

Conversation

nicolas-grekas commented Aug 16, 2017 • edited Loading

marfillaster commented Aug 16, 2017

Choose a reason for hiding this comment

nicolas-grekas Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yceruto Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

mnapoli commented Aug 16, 2017 • edited Loading

nicolas-grekas commented Aug 16, 2017 • edited Loading

mnapoli commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Pierstoval commented Aug 17, 2017

ro0NL commented Aug 17, 2017 • edited Loading

nicolas-grekas commented Aug 17, 2017

kbond commented Aug 17, 2017

nicolas-grekas commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Sep 6, 2017

fabpot commented Sep 7, 2017

sroze commented Sep 12, 2017

nicolas-grekas commented Aug 16, 2017 •

edited

Loading

nicolas-grekas Aug 16, 2017 •

edited

Loading

yceruto Aug 16, 2017 •

edited

Loading

nicolas-grekas Aug 16, 2017 •

edited

Loading

mnapoli commented Aug 16, 2017 •

edited

Loading

nicolas-grekas commented Aug 16, 2017 •

edited

Loading

nicolas-grekas Aug 16, 2017 •

edited

Loading

ro0NL commented Aug 17, 2017 •

edited

Loading