[DependencyInjection] added a way to reference dynamic environment variables in service definitions #16403
Conversation
2fb77e3 to
26acc8f
Compare
| */ | ||
| public function __toString() | ||
| { | ||
| return (string) $this->id; |
There was a problem hiding this comment.
IMO is better to cast to a string in the constructor.
There was a problem hiding this comment.
or dont cast at all. The parameter is already defined as string.
|
It looks like this still dumps literal values into the container instead of I'm using your branch: My But the container has a literal value: If I remember correctly, that's the exact problem I had when I tried to implement this myself. I was looking for a solution where there's a Or am I doing something wrong? I used the symfony-demo project with a few |
|
@dzuelke That's intended (or at least, that's the way I see it): a parameter IS evaluated, an env variable is NOT. So, you should use an env variable directly in your definitions instead of assigning the env variable to a parameter first, in which case, it's evaluated. That might be confusing though, so the second commit does what you expected. The only "problem" I can see is that this new behavior introduces some inconsistencies: a parameter storing an env variable is not evaluated only when used as a constructor argument or a method call argument; the same parameter used for a class name, or an alias for instance will be evaluated (as the code expects static values for those elements, and I don't want to support dynamic values here). WDYT? |
|
If you only use it for service definitions you can simple use a custom Expression function. |
|
Which BC break? |
|
I think where a parameter is actually named |
|
Using an expression is indeed a very good idea and should work well. I'll have later. |
How would I do that, @fabpot?
That's what @ruudk suggested in #7555 (comment) and what @stof implemented in https://github.com/Incenteev/DynamicParametersBundle but the problem AFAICT is that every bundle out there would have to be modified to use that. |
|
Can't we change the syntax to |
| @@ -167,12 +168,12 @@ public function resolve() | ||
| * @throws ParameterCircularReferenceException if a circular reference if detected | ||
| * @throws RuntimeException when a given parameter has a type problem. | ||
| */ | ||
| public function resolveValue($value, array $resolving = array()) | ||
| public function resolveValue($value, array $resolving = array(), $protectEnvVariables = false) |
There was a problem hiding this comment.
The new protectEnvVariables param is missing from the docblock.
9ee8aab to
fd16950
Compare
|
@dzuelke Can you test the latest version of this patch? It should work fine now. |
|
Sure @fabpot, will do. I thought you were going to rewrite it to use expressions first, my bad :) |
| @@ -236,7 +245,7 @@ public function resolveString($value, array $resolving = array()) | ||
| $resolved = (string) $resolved; | ||
| $resolving[$key] = true; | ||
|
|
||
| return $self->isResolved() ? $resolved : $self->resolveString($resolved, $resolving); | ||
| return $self->isResolved() ? $resolved : $self->resolveString($resolved, $resolving, $protectEnvVariables); |
There was a problem hiding this comment.
The variable is missing from the use() statement!
|
The |
|
But even then, still no luck, @fabpot: I'm using a branch based on yours, with my fix (dzuelke/symfony@1cf6e98) applied: |
|
Also paging @stof for thoughts. Or am I doing something really wrong here? |
|
I also played around with various permutations around parameters, and The value there also gets written out to the cache literally and not as a |
| @@ -1377,6 +1380,8 @@ private function dumpValue($value, $interpolate = true) | ||
| // we do this to deal with non string values (Boolean, integer, ...) | ||
| // the preg_replace_callback converts them to strings | ||
| return $this->dumpParameter(strtolower($match[1])); | ||
| } elseif (preg_match('/^\$([^\$]+)\$$/', $value, $match)) { |
There was a problem hiding this comment.
This is will also match $foo'boom$ making it possible to unwillingly inject PHP code (as the value is not escaped or anything when it's used in dumpEnvVariable ).
And the $$ will not match, but is not normalized to a single $ in the else-block.
…riables in service definitions
|
Hello Problem with this format is that it clashes with eZ dynamic settings which uses the same. |
fd16950 to
22789cd
Compare
|
Conclusion: This is not possible, and by far. The semantic configuration used by bundle extensions is resolved before being passed to the extension. If we do not resolve it anymore, each extension would have to resolve values on a case-by-case basis AND would need to keep the non-resolved value. That might be a problem when a parameter has influences over the configuration. So, a generic system is just not possible without rethinking the whole system. The only thing I can think of is to allow the user to override some service arguments after the container has been compiled, but that would mean relying on conventions sometimes (when the service using the configuration has been dynamically created for instance). Another possibility is to only add support for some bundles like the DSN for the Doctrine bundle. That might be easier and would probably fix most of the issues. Closing for now. |
|
More or less in the same field, it's been a while that we, at eZ, think about contributing our siteaccess system to enable one to have context aware settings (e.g.Multisite inside one app). |
…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
When hosting a Symfony app on some PaaS like Heroku, or even if you are just using Docker, you need a way to reference environment variables in the container. That's already possible in Symfony but the values are dumped into the cache. This PR adds a way to reference an env variable that is resolved dynamically even on a compiled container.
ping @dzuelke