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] Allow processing env vars #23901

Merged
merged 1 commit into from Sep 7, 2017
Merged

[DI] Allow processing env vars #23901

merged 1 commit into from Sep 7, 2017

Conversation

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Aug 16, 2017

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:

#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')))).

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 16, 2017
@nicolas-grekas 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
@marfillaster
Copy link
Contributor

@marfillaster marfillaster commented Aug 16, 2017

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:env-provider branch 4 times, most recently from ab01c8d to d75dcae Aug 16, 2017
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:env-provider branch from d75dcae to f3144e1 Aug 16, 2017
*
* @throws RuntimeException on error
*/
public function getEnv($prefix, $name, \Closure $getEnv);

This comment has been minimized.

@ro0NL

ro0NL Aug 16, 2017
Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 16, 2017
Author Member

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

This comment has been minimized.

@ro0NL

ro0NL Aug 16, 2017
Contributor

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

This comment has been minimized.

@ro0NL

ro0NL Aug 16, 2017
Contributor

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

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

This comment has been minimized.

@yceruto

yceruto Aug 16, 2017
Member

Can we fix #23527 (comment) somehow?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 16, 2017
Author Member

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

@mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Aug 16, 2017

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
Copy link
Member Author

@nicolas-grekas nicolas-grekas commented Aug 16, 2017

@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
Copy link
Contributor

@mnapoli mnapoli commented Aug 16, 2017

👍 sorry :)

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

This comment has been minimized.

@Pierstoval

Pierstoval Aug 16, 2017
Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 16, 2017
Author Member

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 nicolas-grekas force-pushed the nicolas-grekas:env-provider branch from f3144e1 to 62678b9 Aug 16, 2017
@nicolas-grekas 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 nicolas-grekas force-pushed the nicolas-grekas:env-provider branch from 62678b9 to 2baa573 Aug 16, 2017
@@ -43,6 +43,7 @@ public function __construct()
100 => array(
$resolveClassPass = new ResolveClassPass(),
new ResolveInstanceofConditionalsPass(),
new RegisterEnvProvidersPass(),

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 16, 2017
Author Member

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:env-provider branch from 2baa573 to 751e5f8 Aug 17, 2017
@Pierstoval
Copy link
Contributor

@Pierstoval Pierstoval commented Aug 17, 2017

I cannot agree more on this PR 👍

@ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Aug 17, 2017

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
Copy link
Member Author

@nicolas-grekas nicolas-grekas commented Aug 17, 2017

@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
Copy link
Contributor

@kbond kbond commented Aug 17, 2017

What is the reason for no bool prefix?

@nicolas-grekas
Copy link
Member Author

@nicolas-grekas nicolas-grekas commented Aug 18, 2017

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:env-provider branch from 751e5f8 to 6e10746 Aug 18, 2017
@dunglas
dunglas approved these changes Sep 5, 2017
Copy link
Member

@dunglas dunglas left a comment

Awesome, can't wait to get this merged :)

*/
class RegisterEnvProvidersPass implements CompilerPassInterface
{
private static $allowedTypes = array('array', 'bool', 'float', 'int', 'string');

This comment has been minimized.

@dunglas

dunglas Sep 5, 2017
Member

Why not /* private */ const ALLOWED_TYPES?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 5, 2017
Author Member

because that wouldn't be really private, whereas a private static has no such downside

}

if ($providers) {
if ($container->getParameterBag() instanceof EnvPlaceholderParameterBag) {

This comment has been minimized.

@dunglas

dunglas Sep 5, 2017
Member

Can be stored in a var to prevent an extra function call.

}
}

private static function validateProvidedTypes($types, $class)

This comment has been minimized.

@dunglas

dunglas Sep 5, 2017
Member

What's the benefit of making this function static, as it is already private?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 5, 2017
Author Member

none, but another reviewer asked for it, and there is neither no benefit not doing it static :)

}

if ('bool' === $prefix) {
return (bool) XmlUtils::phpize($env);

This comment has been minimized.

@dunglas

dunglas Sep 5, 2017
Member

Shouldn't symfony/config become a hard dependency?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 5, 2017
Author Member

a exception is now thrown if Config is missing

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:env-provider branch from 24dce14 to 575eae8 Sep 5, 2017
@@ -4,6 +4,7 @@ CHANGELOG
3.4.0
-----

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

This comment has been minimized.

@fabpot

fabpot Sep 6, 2017
Member

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.

This comment has been minimized.

@fabpot

fabpot Sep 6, 2017
Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 6, 2017
Author Member

done


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

This comment has been minimized.

@fabpot

fabpot Sep 6, 2017
Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 6, 2017
Author Member

not a blocker here, and no better idea :)

$i = strpos($name, ':');

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

This comment has been minimized.

@fabpot

fabpot Sep 6, 2017
Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Sep 6, 2017
Author Member

removed

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:env-provider branch 5 times, most recently from 26c28eb to 019a582 Sep 6, 2017
@@ -36,6 +36,7 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\EnvProviderInterface;

This comment has been minimized.

@fabpot

fabpot Sep 6, 2017
Member

Should be EnvVarProcessorInterface (same below)

@@ -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');

This comment has been minimized.

@fabpot

fabpot Sep 6, 2017
Member

should be renamed as well

@fabpot
Copy link
Member

@fabpot fabpot commented Sep 6, 2017

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:env-provider branch from 019a582 to 2499a76 Sep 7, 2017
@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:env-provider branch from 2499a76 to 1f92e45 Sep 7, 2017
@fabpot
Copy link
Member

@fabpot fabpot commented Sep 7, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 1f92e45 into symfony:3.4 Sep 7, 2017
2 of 3 checks passed
2 of 3 checks passed
fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabpot added a commit that referenced this pull request Sep 7, 2017
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
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:env-provider branch Sep 10, 2017
@sroze
Copy link
Member

@sroze sroze commented Sep 12, 2017

Great job @nicolas-grekas 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet