[DI] Allow fetching env vars with lookup-dedicated services #20276

nicolas-grekas · 2016-10-22T16:33:12Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	(not written yet)
Fixed tickets	-
License	MIT
Doc PR	(if feat. accepted)

Thinking about issues like e.g. https://github.com/platformsh/platformsh-example-symfony/issues/16, that need some preprocessing to get env vars values, one can already create a custom base container class and override the getEnv() protected method to achieve it.
But unless I missed something, one couldn't write a bundle to package this for easy reuse.
I propose to ship 3.2 with the attached patch, that adds a way to reference env var using this syntax:
%env(var@service)%, that would turn into a dynamic call to $container->get('service')->getEnv('var') (well, not exactly so that it works with private services also, but that's the spirit).
Before writing any tests and fine tuning the feature, WDYT of the idea?

ro0NL · 2016-10-22T16:53:16Z

What about a function-like syntax %env(VAR[, lookup_service])%?

javiereguiluz · 2016-10-22T17:12:01Z

I can't propose a better alternative, but the proposed syntax looks ugly to me. Besides, Symfony 3.2 entered the feature freeze period a month ago, so I'm not sure if it's a good idea to consider this new feature for 3.2.

nicolas-grekas · 2016-10-22T19:04:36Z

@javiereguiluz I'd love to have a better proposal! The feat. freeze perdiod is exactly for this: fine tuning new features (and this feat. may qualify as such), and tweaking their API.
For ex., if you think that %env(FOO)% should be %env$FOO%, and %env(service:FOO)% be %env$service$FOO%, it's now, of never :)

javiereguiluz · 2016-10-23T07:56:21Z

After thinking a bit about this, I like the syntax proposed by @ro0NL:

# normal env variable
%env(DB_PASSWORD)%

# env variable with a lookup service
%env(DB_PASSWORD, @app.lookup_service)%

nicolas-grekas · 2016-10-23T08:21:17Z

You added a space, then a @: this is the beginning of a DSL... I understand it looks sexy, but that's overkill to me here. I prefer seeing this as a namespaced identifier.

ro0NL · 2016-10-23T09:04:19Z

Btw, i proposed it like that because the current syntax already was like something(value), looks like a function to me ;-) That was the beginning of a DSL imo.

However agree that this syntax may not be desired given the context; represent a value placeholder.

What about array-like syntax then?

%env[FOO]%
%env:lookup_service[FOO]%

If we go one step further it could be %<service_id>[<key_to_get>]%;

%env[FOO]% # use the built-in 'env' service to get 'FOO'
%something[FOO]% # use the 'something' service to get 'FOO'

ro0NL · 2016-10-23T10:24:25Z

And last but not least, we should consider to differ between a lookup and a decorating service.

# lookup service
%env[key]%
%lookup_service[key]%

# decorating service
%env:decorating_service[key]
%lookup_service:decorating_service[key]%

Decorating services receive the key as well as the value (from the lookup service).

chalasr · 2016-10-23T10:27:08Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -419,6 +419,14 @@ protected function getEnv($name)
        if (false !== $env = getenv($name)) {
            return $this->envCache[$name] = $env;
        }
+        if (0 < $i = strpos($name, ':')) {


Shouldn't this check be the first?

It could. I made it this way on purpose though, looks more flexible to me.

chalasr · 2016-10-23T10:32:18Z

src/Symfony/Component/DependencyInjection/Container.php

+            $id = substr($name, 0, $i);
+            $service = isset($this->services[$id]) ? $this->services[$id] : (isset($this->methodMap[$id]) ? $this->{$this->methodMap[$id]}() : $this->get($id));
+
+            if (null !== $env = $service->getEnv(substr($name, 1 + $i))) {


Don't we need an interface for services with such ability, ensuring the getEnv() method?

With my last example, i propose to have a ParameterLookupInterface::lookup and ParamaterDecoratorInterface::decorate. In case we want to decouple from env specific vars.

Adding the interface was next on my list. So is a compiler pass that checks that the referenced services exist and implement it.

What about processEnv($name, $value) then? And continue with the %env:service(key)% syntax (or square brackets, i dont mind). We should avoid the term lookup.

Besides you can still process $value based on $key if needed (ie. do a lookup). But i think simply decorating $value as given is most common.

interface & compiler pass added

nicolas-grekas · 2016-10-23T14:39:38Z

If we go one step further it could be %<service_id>[<key_to_get>]%;

We have to remember that the env-related syntax is potentially conflicting with existing parameters into the wild. Having such a generic syntax makes the collision much more likely to me.

decorating service

I don't see the need for these. Lookup services are required for operations such as json-decoding some env var. There is no point into architecturing for greater extensibility here IMHO.

ro0NL · 2016-10-23T15:03:48Z

Having such a generic syntax makes the collision much more likely to me.

We could always check those first right? But im fine with a env prefix as well because of EnvPlaceholderParameterBag.

Just saying if we allow to change the underlying implementation it might not be env related anymore, ie. %env:lookup_from_db[key]%, which perhaps makes less sense then having %db[key]%.

Perhaps consider deprecating the reserved syntax first, instead of silently ingoring params then.

decorating service

I thought of it in terms of letting symfony lookup the value from the env, and my application decorate it. E.g. do things as string("true") > bool(true).

nicolas-grekas · 2016-10-23T15:51:36Z

I'm giving up the idea to merge this in 3.2.
Syntax updated to ``%env(var@service)%`.

it might not be env related anymore

true, but should we care? env-like is enough. Your example (db) would be considered a bad practice: env lookups should be blazing-fast, and available early at bootstrap (because they are for configuring services, such as eg. ... db connections ;-) ).

things as string("true") > bool(true).

So, totally unrelated to this PR. And I fail to see what issue this would solve btw (but please discuss this in a dedicated issue then if you have one).

ro0NL · 2016-10-23T16:00:28Z

Lookup services are required for operations such as json-decoding some env var.

Sound like decorating to me ;-) assuming symfony still provides the env var.

But i get your point.. the related issue https://github.com/platformsh/platformsh-example-symfony/issues/16#issuecomment-248900141 is actually lookup related;

reading from a base64+json-encoded env var

ro0NL

I'm giving up the idea to merge this in 3.2.

Dont :) This is good. Syntax is fine as well imo.

ro0NL · 2016-10-23T16:16:28Z

src/Symfony/Component/DependencyInjection/Container.php

@@ -419,6 +419,14 @@ protected function getEnv($name)
        if (false !== $env = getenv($name)) {
            return $this->envCache[$name] = $env;
        }
+        if (false !== $i = strpos($name, '@')) {


What about a default implementation of GetEnvInterface? and be explicit here (in terms of priority);

if () { $service->getEnv(); } else { $default->getEnv(); }

We already have a default here: it's the code surrounding the new block.

I mean a GetEnv class for the getenv/$_ENV lookup.

Yep, that's what I thought, and that would require a class, which means a service, thus a service definition. But the container comes without any service preconfigured...

This is about if we should allow to override the lookup service by having FOO@service in our $_ENV var for example. I've no strong opinion on this, but it could make sense to do:

if (false !== $i = strpos($name, '@')) { $value = $service->getEnv(substr($name, 0, $i)); } else { $value = (new GetEnv())->getEnv($name); }

Ie. consistently rely on 1 lookup implementation.

Oh and shouldnt this be strrpos? Not sure ;-)

edit: never mind, it's no expected value anyway.

ro0NL · 2016-10-23T16:18:21Z

src/Symfony/Component/DependencyInjection/Container.php

+            if (null !== $env = $service->getEnv(substr($name, 0, $i))) {
+                return $this->envCache[$name] = $env;
+            }
+        }
        if (!$this->hasParameter("env($name)")) {


Shouldnt we use the normalized name here? And provide a default value regarding the lookup service?

By normalized name, you mean with lowercased service id? If yes, it's not required: parameters are already case insensitive in the base ParameterBag.
And no, we don't want a default service: the container comes with no service by default, yet env vars should work. See comment above.

I mean the normalized key: substr($name, 0, $i), ie.

env(FOO): default env(FOO@service): default

The first could work in all cases right? Making the second obsolete.

But these are really two separate variable identifiers.

You're right, lets stay true to the parameter architecture. (opposed to what im thinking towards;)

parameters: key: value env: FOO: bar

nicolas-grekas · 2016-10-24T07:46:09Z

Tests added

chalasr

LGTM 👍

javiereguiluz · 2016-11-07T15:01:06Z

Regarding the custom syntax ... let's try to make it compatible with the idea of adding new features in the future. For example, in #20434 they are asking us for a way to transform env vars to other types different than strings (e.g. bool, int, float).

If we decide to use the same syntax as the ParameterBag, we could allow for things like:

%env(getBoolean(SF_DEBUG))%
%env(getInt(SF_MAX_CONNECTIONS))%

nicolas-grekas · 2016-11-07T15:11:11Z

This is not a DSL, but a namespaced string.
So could be env(int:FOO).
BUT let's not plan for something that might never happen.
#20434 is far from being ready nor easy nor confirmed...

nicolas-grekas · 2017-01-16T10:59:07Z

Closing this one, because it may not be interesting enough - or it may be better for people to align on how env vars works.
Could be resurrected if someone else comes with an actual use case we want to support.

The version 3.2.0 and 3.2.1 of reflection-docblock is broken and lower version as 3.1 miss some tags

…nfigure (nicolas-grekas) This PR was merged into the 3.3 branch. Discussion ---------- [DI] Fix YamlDumper not dumping abstract and autoconfigure | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- 685ff0e [DI] Fix YamlDumper not dumping abstract and autoconfigure

This PR was merged into the 3.3 branch. Discussion ---------- restrict reflection doc block | Q | A | ------------- | --- | Branch? | 3.1, 3.2, 3.3, master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT I think we should have same behavior on 2.8 and 3.0 but there `phpdocumentor/reflection` is required and i whanted to avoid conflicts. ### Reason: The version 3.2.0 and 3.2.1 of phpdocumentor/reflection-docblock is really broken. All Annotations lide `@api`, means without any bracket, leads to errors like `Uninitialized string offset: 0`. We in the CMF do not require that bundle directly, but symfony does and so we get braking builds. Commits ------- 6760127 restrict reflection doc block

…grekas) This PR was merged into the 2.7 branch. Discussion ---------- [DI] Fix dumping abstract with YamlDumper | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Commits ------- c396e8c [DI] Fix dumping abstract with YamlDumper

* 2.7: [DI] Fix dumping abstract with YamlDumper

* 2.8: [DI] Fix dumping abstract with YamlDumper

* 3.3: [DI] Fix dumping abstract with YamlDumper restrict reflection doc block [DI] Fix YamlDumper not dumping abstract and autoconfigure

@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

carsonbot added Status: Needs Review Feature labels Oct 22, 2016

chalasr reviewed Oct 23, 2016

View reviewed changes

nicolas-grekas force-pushed the env-provider branch from d2b3cf7 to b15e1ea Compare October 23, 2016 15:44

ro0NL reviewed Oct 23, 2016

View reviewed changes

nicolas-grekas force-pushed the env-provider branch from b15e1ea to 7aee5b6 Compare October 24, 2016 07:40

nicolas-grekas added the DependencyInjection label Oct 24, 2016

nicolas-grekas force-pushed the env-provider branch 3 times, most recently from 0898c86 to 8c2a3c5 Compare October 27, 2016 09:20

chalasr reviewed Nov 1, 2016

View reviewed changes

nicolas-grekas mentioned this pull request Nov 5, 2016

[DependencyInjection] Introduce a new ParameterResolver #17192

Closed

javiereguiluz added this to the 3.3 milestone Nov 7, 2016

nicolas-grekas force-pushed the env-provider branch 2 times, most recently from 84d4b98 to ee189c1 Compare November 8, 2016 09:28

nicolas-grekas removed this from the 3.x milestone Dec 15, 2016

nicolas-grekas force-pushed the env-provider branch from 105c09b to 468b4b5 Compare January 7, 2017 16:05

nicolas-grekas closed this Jan 16, 2017

nicolas-grekas deleted the env-provider branch January 16, 2017 10:59

nicolas-grekas mentioned this pull request May 1, 2017

Environment Variables lack type - break built-in Symfony configuration #22151

Closed

nicolas-grekas mentioned this pull request Jul 22, 2017

[DI] Add support for secrets #23621

Closed

marfillaster mentioned this pull request Jul 26, 2017

Runtime container parameter not found event filter #23669

Closed

nicolas-grekas mentioned this pull request Aug 9, 2017

[DependencyInjection] [DX] Support for JSON string environment variables #23823

Closed

nicolas-grekas restored the env-provider branch August 9, 2017 15:15

nicolas-grekas reopened this Aug 9, 2017

nicolas-grekas modified the milestones: 3.4, 3.3 Aug 9, 2017

nicolas-grekas and others added 11 commits August 10, 2017 13:45

[DI] Fix YamlDumper not dumping abstract and autoconfigure

685ff0e

restrict reflection doc block

6760127

The version 3.2.0 and 3.2.1 of reflection-docblock is broken and lower version as 3.1 miss some tags

[DI] Fix dumping abstract with YamlDumper

c396e8c

Merge branch '2.7' into 2.8

daa0251

* 2.7: [DI] Fix dumping abstract with YamlDumper

Merge branch '2.8' into 3.3

72791d6

* 2.8: [DI] Fix dumping abstract with YamlDumper

Merge branch '3.3' into 3.4

d1c754d

* 3.3: [DI] Fix dumping abstract with YamlDumper restrict reflection doc block [DI] Fix YamlDumper not dumping abstract and autoconfigure

fix merge

eeaea83

[DI] Allow fetching env vars with lookup-dedicated services

2a84059

nicolas-grekas force-pushed the env-provider branch from 468b4b5 to 2a84059 Compare August 14, 2017 19:02

ro0NL mentioned this pull request Aug 15, 2017

[DI] Update Container::getEnv phpdoc #23889

Closed

nicolas-grekas closed this Aug 16, 2017

nicolas-grekas deleted the env-provider branch August 16, 2017 12:46

nicolas-grekas mentioned this pull request Aug 16, 2017

[DI] Allow processing env vars #23901

Merged

Uh oh!

[DI] Allow fetching env vars with lookup-dedicated services #20276

[DI] Allow fetching env vars with lookup-dedicated services #20276

Uh oh!

Conversation

nicolas-grekas commented Oct 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented Oct 22, 2016

Uh oh!

javiereguiluz commented Oct 22, 2016

Uh oh!

nicolas-grekas commented Oct 22, 2016

Uh oh!

javiereguiluz commented Oct 23, 2016

Uh oh!

nicolas-grekas commented Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented Oct 23, 2016

Uh oh!

ro0NL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ro0NL Oct 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

nicolas-grekas commented Oct 22, 2016 •

edited

Loading

nicolas-grekas commented Oct 23, 2016 •

edited

Loading

ro0NL commented Oct 23, 2016 •

edited

Loading

ro0NL commented Oct 23, 2016 •

edited

Loading

ro0NL Oct 23, 2016 •

edited

Loading

ro0NL Oct 23, 2016 •

edited

Loading

nicolas-grekas commented Oct 23, 2016 •

edited

Loading

ro0NL commented Oct 23, 2016 •

edited

Loading

nicolas-grekas commented Oct 23, 2016 •

edited

Loading

ro0NL Oct 23, 2016 •

edited

Loading

ro0NL Oct 24, 2016 •

edited

Loading

ro0NL Oct 23, 2016 •

edited

Loading

ro0NL Oct 23, 2016 •

edited

Loading

ro0NL Oct 23, 2016 •

edited

Loading

nicolas-grekas commented Nov 7, 2016 •

edited

Loading

nicolas-grekas commented Jan 16, 2017 •

edited

Loading