-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Make accessing environment variables in configuration more convenient. #18155
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
Conversation
Is it possible to include unit tests? |
{ | ||
protected function resolve($value) | ||
{ | ||
if (is_string($value) && strlen($value) > 0 && $value[0] == '@') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of strlen() > 0
you should use '' !== $value
, also strict check for this: '@' === $value[0]
.
@patrick-mcdougle: Sure, I'll be happy to write some unit tests if you folks think this approach has merit :) |
I don't like the idea of having the same If an option allows to pass an expression, it should just allow the expression without any need for crazy prefixes imo. See for instance the options allowing expressions in Symfony. |
@wouterj Without any form of prefix? How would you distinguish between an expression and just a string value? |
I really like the idea which is a step further in supporting the twelve factor-app. Would be nice to ear about @symfony/deciders point of view |
@magnusnordlander Can you give us a usage of the trait? I don't understand how that works. |
@fabpot The YAML loader itself only runs in service definitions. This functionality is indeed USING the expression language. It simply registers a new expression function that gets compiled to a |
I don't really like the trait for the extension. Using However, if an option has a scalar, I think it makes sense to allow passing an expression in there. I don't think we should reuse service configuration syntax here though. |
{ | ||
if (is_string($value) && '' !== $value && '@' === $value[0]) { | ||
if (strlen($value) > 1 && $value[1] === '=') { | ||
return new Expression(substr($value, 2)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since ES is a soft dep of DI, would it make sense to throw an exception here before triggering a fatal error when this syntax is used but ES is not available? We do so in other parts AFAIK
Okay, I'm absolutely open to an alternate syntax. How about just skipping the acme_bundle:
some_value: "Just another scalar"
some_other_value: "=env('PATH')" # or any other valid service container expression
some_third_value: "== to escape strings starting with = (similar to the %% syntax)" Reasonable? |
I still don't get it. |
@fabpot If I undestand this PR correctly, @magnusnordlander wants to support this format everywhere in the config. E.g. |
What is the benefit of reading ENV on every run? And why SYMFONY__ENV vars can't be used instead? |
Because in some deployment environments (Heroku, Docker etc.), environment variables can change after the container has been built. If you're using SYMFONY__ENV vars, they'll be cached in the dumped container, necessitating a cache rebuild on environment changes. |
Docker env does not change after container was started.If it does - then issue is not in the symfony itself. However Symfony as it is now does not fit dockerised ENV. |
It is an issue in Symfony. No other language or framework I know does such caching. The whole point of Docker, Heroku, and all the Twelve-Factor systems is that configuration happens through the environment, and immediately gets applied. Some people have cache warmup procedures that run for minutes, not seconds (don't ask me why), and doing that on each container run defeats the whole purpose. |
12 factor approach does not tell anything about caching of the env vars. As well as about the approach when they should chenge between the deployments, but it mentions:
It means that there should be some deploy procedure, which includes clear prod cache. As for cache warmers - why should framework add a workaround for bad app design? And if your config changes between deployments - probably other config storage will suit better. |
This is not possible without a major API break for DI extensions or serious runtime performance penalties.
When using semantic bundle configuration, there is no possibility for another config storage without a major API break. All configuration gets processed by DI extensions and compiler passes, then cached in the compiled DIC. Furthermore, I quite like the elegance of the Heroku build system. Your application is turned into an immutable slug of code, with an already warmed cache, configured only using environment variables. This should be preferable in a Dockerized environment as well, to avoid the overhead of warming the cache on start, and instead have an already hot version in your container. |
Yes it does. A build runs once, and is combined with configuration from the environment into a release. If the config changes, no new build is performed. That's factor number V: http://12factor.net/build-release-run The whole purpose of env vars for runtime configuration is that you do not need to re-build your code. Emergency/maintenance/readonly modes, split factors for A/B testing, database credentials on failover - these are things you want, and should be able, to change quickly. Docker has separate build time ( Back when the config cache got designed for Symfony 2, the possibility of config to change without a code/cache re-build was simply not considered. That's no big deal, but it doesn't mean it doesn't need fixing. |
Symfony guide us to use Consider prod cache as safety net. Imagine we've disabled caching for env vars - now, if we rename env var or remove it from your heroku admin panel - should all the hosted apps die? It means that to prevent it - we have to maintain both configurations in the code, what does not make any sense for our releases. Just brings more complexity and room for bugs. What about rollback? If env is changed how to automate this without a cache? Sure it's possible to do it without a cache, but solution is already there. Using cache we can easily change env and then trigger deploy procedure with new clean version of app. Or just clear the cache. Another question is - what about other hostings? Heroku is not the only possible hosting. Especially it's important when app is deployed by some script which does not export its ENV to the global ENV, like chef for example. Then right after deployment app wont work, because outside of the script ENV is empty, correct? What about console scripts?
It explicitly says - combine your code with config once on release(deploy). And once it combined - symfony caches the result. Then runtime stage does not change ENV anymore.
It looks like you trying to use ENV for your business logic configuration, when it should be used only for app/infrastructure layer configuration. Have you thought about using tools like ZooKeeper instead? Or implement your own admin backend? 12 factor app:
|
Symfony is not an infallible deity that gives gifts and offers us advice. Symfony is a set of tools, a very good set of tools, useful in a lot of cases, but which in this particular case is lacking. There's nothing wrong with wanting to make the framework better by making it work well under more circumstances. What you are describing does not work well when you pass configuration via environment variables, and those environment variables can change at runtime. A major hosting provider uses this scheme. A very popular container format is better off using this scheme. There is no reason why we shouldn't attempt to accomodate it.
Yes. I personally do not feel the need for any such safety nets.
That is the responsibility of the hosting provider using such a scheme (and indeed on Heroku, rollback is already very much possible, you just press a button).
If you're not passing configuration through environment variables, this does not affect you. If you are passing environment variables through IncenteevParameterHandler or |
i'm not against new features, i'm for correct responsibilities and obvious ways of usage. Once there was a framework, which had multiple ways to implement same thing. And developers were not able to use their framework knowledge to start with maintenance of existing apps fast, because every previous developer had his own way. Allowing this kind of configuration - we automatically allow it to be used in any other config. I expect once this feature is added - people will use it instead of SYMFONY_VARS, as it look simpler on development stage. And, because developers does not read documentation, but copy examples, until they stacked for a day with debug - it will lead to a lot of confusion, when those vars will disappear at some moment. It will need dangerous mark in documentation. People will use them in different config files and it will become a hell, to find which bundle needs which env var. Because people use short cuts quite often. It's hard to find a good bundle now. And it will become even harder. Symfony already has issues with developer experience. Should we add more? So if configuration should change during runtime - maybe it would be better to use another configuration provider, and don't use ENV in this case? And probably, we have to improve sf2-3 docs and explain what should be in ENV, and what should not? Anyway, with all being said - let others decide: @stof ? |
So, personal opinions on the respective merits of dynamic environment variables aside, let me try to restate the issue and what this PR attempts to do to alleviate it. Presuppositions
The problem with just adding the functionUsing the function in a DI Extension is fairly easy, and could look something like this:
That's easy from a bundle author's point of view. The trouble comes when you're actually writing the configuration tree. How do you want to allow your users to pass you either a regular configuration value (be it a constant or a DI parameter), or an environment variable? Do you invent a new syntax (sort of like some bundles have done when referencing a service ID in their configuration)? Should you be really explicit and have a structure with type and value (again, like some bundles do when you are allowed to reference a service)? I believe that in order for this to be widely adopted, there needs to be a convention on how to accept different types of configuration values. My proposalI propose that we standardize on one syntax for this. To be honest, I don't really care which syntax we choose, as long at it works. A non-exhaustive list of suggestions include:
And for XML:
These are just examples. The PR does have a suggested syntax, but I'm by no means opposed to changing it to some other syntax. I do hope this clears up the intent and purpose of this PR. |
Actually, the |
c3a4064
to
2698254
Compare
@nicolas-grekas reading the env directly is fine IMO. this is what I did in https://github.com/Incenteev/DynamicParametersBundle. No need to create a dedicated service. Btw, the function reading environment variable in the expression language works fine. My bundle failed to solve this use case, because of the point solved here: allowing DI extensions to accept expressions directly instead of trying to hook in the existing usage of parameters to make them dynamic. So a big 👍 for adding the ExpressionNode, and then a 👍 for adding a way to read the environment (and IMO, we should not add a public method in the container for that) |
|
||
return sprintf('$this->getEnvironmentVariable(%s, %s)', $arg, $default); | ||
}, function (array $variables, $value, $default = null) { | ||
if (2 > func_num_args()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong number of arguments. the distinction is between 2 and 3 here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
@stof: The only way I see around having a public method on the container is either a (possibly namespaced and/or static, but still globally available) function, inlining a quite big chunk of code, or coupling the expression language function to a specially "blessed" service in the container. Out of these options, I would consider the public method to be the lesser of evils, but maybe I'm missing some option? |
2698254
to
f505de8
Compare
Actually, I've changed my mind, I'm making it into a static method instead. |
947823d
to
297546f
Compare
} | ||
|
||
return sprintf('Symfony\\Component\\DependencyInjection\\Environment::getVariable(%s, %s)', $arg, $default); | ||
}, function (array $variables, $value, $default = null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of the Environment class, my personal preference to avoid a public method would to rebind this closure.
\Closure::bind(..., null, Container::class)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, cool, I did not know that was possible. I'll try it out, and update the PR tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to work nicely.
d64daf4
to
5d2feb0
Compare
LGTM, could you add some test cases please? |
5d2feb0
to
ff8a2d9
Compare
ff8a2d9
to
4c803ff
Compare
There, now at least the interesting parts are being tested :) |
public function setUp() | ||
{ | ||
if (!class_exists(Expression::class)) { | ||
$this->markTestSkipped('The ExpressionLanguage component is not installed.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for this one: EL is always here, thx to composer require-dev
BUT you need to fix the lowest required EL version in Config's composer.json (to fix travis)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
4c803ff
to
526fd90
Compare
👍 |
Can everyone here please review PR #19681 which contains a backward compatible alternative to this PR? |
The difference with #19681 from a user pov is the matter of responsibility: who says "here, use this env var". In this PR, env vars are referenced by bundle authors: the user can't use env vars where the bundle author did not allow to do so. This is the limitation that made me search for another way and led to #19681, where the user decides where dynamic env vars are injected. Yet, this does not disqualify this PR: having an ExpressionNode in configurations might still be useful! |
#19681 works great from my point of view, |
Closing in favor of #19681 |
…env(MY_ENV_VAR)% (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [DI] Allow injecting ENV parameters at runtime using %env(MY_ENV_VAR)% | Q | A | ------------- | --- | Branch? | master | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #10138, #7555, #16403, #18155 | License | MIT | Doc PR | symfony/symfony-docs#6918 This is an alternative approach to #18155 for injecting env vars into container configurations. With this PR, anywhere parameters are allowed, one can use `%env(ENV_VAR)%` to inject a dynamic env var. Additionally, if one sets a value to such parameters in e.g. the `parameter.yml` file (`env(ENV_VAR): foo`), this value will be used as a default value when the env var is not defined. If no default value is specified, an `EnvVarNotFoundException` will be thrown at runtime. Unlike previous attempts that also used parameters (#16403), the implementation is compatible with DI extensions: before being dumped, env vars are resolved to uniquely identifiable string placeholders that can get through DI extensions manipulations. When dumped, these unique placeholders are replaced by dynamic calls to a getEnv method.. ping @magnusnordlander @dzuelke @fabpot Commits ------- bac2132 [DI] Allow injecting ENV parameters at runtime using %env(MY_ENV_VAR)% syntax
There have been several attempts to solve the issue of dynamic environment variables in Symfony (the issues are outlined in #10138, #7555). The latest stab at the issue that I am aware of was #16403, but that was closed as Fabien concluded that "a generic system is just not possible without rethinking the whole system", and that "another possibility is to only add support for some bundles like the DSN for the Doctrine bundle" (see #16403 (comment)).
In order to make it easier on bundles to support allowing environment variables in their configuration, I propose adding an expression language function to get environment variables from service expressions, and a trait to more easily parse such an expression from the bundle extension.
This could easily be made in a separate bundle (see my own https://github.com/fervo/EnvironmentBundle), but I feel that adoption will be far higher if we do it in core. Using the trait, configuration with an environment variable is quite simple, something along the following line works brilliantly:
If the extension uses the configuration values as service arguments verbatim, all it has to do is something like the following:
It does require opt-in from all bundles wishing to use this, and for all configuration values it is used on, but I think that is the best way forward.