Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] added a way to reference dynamic environment variables in service definitions#16403
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
2fb77e3 to26acc8fCompareThere 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.
IMO is better to cast to a string in the constructor.
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.
or dont cast at all. The parameter is already defined as string.
dzuelke commentedOct 31, 2015
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 |
fabpot commentedOct 31, 2015
@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? |
sstok commentedOct 31, 2015
If you only use it for service definitions you can simple use a custom Expression function. |
fabpot commentedOct 31, 2015
Which BC break? |
linaori commentedOct 31, 2015
I think where a parameter is actually named |
fabpot commentedOct 31, 2015
Using an expression is indeed a very good idea and should work well. I'll have later. |
dzuelke commentedOct 31, 2015
How would I do that,@fabpot?
That's what@ruudk suggested in#7555 (comment) and what@stof implemented inhttps://github.com/Incenteev/DynamicParametersBundle but the problem AFAICT is that every bundle out there would have to be modified to use that. |
wouterj commentedNov 1, 2015
Can't we change the syntax to |
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.
The newprotectEnvVariables param is missing from the docblock.
9ee8aab tofd16950Comparefabpot commentedNov 4, 2015
@dzuelke Can you test the latest version of this patch? It should work fine now. |
dzuelke commentedNov 4, 2015
Sure@fabpot, will do. I thought you were going to rewrite it to use expressions first, my bad :) |
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.
The variable is missing from theuse() statement!
dzuelke commentedNov 4, 2015
The |
dzuelke commentedNov 4, 2015
But even then, still no luck,@fabpot: I'm using a branch based on yours, with my fix (dzuelke/symfony@1cf6e98) applied: |
dzuelke commentedNov 4, 2015
Also paging@stof for thoughts. Or am I doing something really wrong here? |
dzuelke commentedNov 4, 2015
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 |
dzuelke commentedNov 5, 2015
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.
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 indumpEnvVariable ).
And the $$ will not match, but is not normalized to a single $ in the else-block.
…riables in service definitions
lolautruche commentedNov 11, 2015
Hello Problem with this format is that it clashes with eZ dynamic settings which uses the same. |
fabpot commentedNov 12, 2015
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. |
lolautruche commentedNov 12, 2015
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#6918This 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@fabpotCommits-------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