Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Make ContainerBuilder::resolveEnvPlaceholders() able to inline the values of referenced env vars.#20618
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
56f0257 tobbcae3cComparebbcae3c to64bd54aCompare| // generate a unique namespace for the given application | ||
| $env =$container->getParameter('kernel.root_dir').$container->getParameter('kernel.environment'); | ||
| $env =''; | ||
| foreach (array('kernel.name','kernel.environment','kernel.debug','cache.prefix.seed')as$key) { |
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 kernel name, environment and debug don't need to resolve env parameters, as these are special parameters set by the Kernel itself (redefining them is not allowed, as the value set by the kernel will win anyway)
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.
Doing so makes the code simpler. Do you think this can have any adverse side effect?
| * @param array $resolving An array of keys that are being resolved (used internally to detect circular references) | ||
| * | ||
| * @return mixed The resolved value | ||
| */ |
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.
array $resolving = array() should not be exposed in the public API IMO. Use a private method for the recursive call
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 makes the resolution stateless.ParameterBag::resolveValue has the same strategy on this topic. I'd prefer keeping it this way.
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.
Well, I'm not talking about adding a private property. I'm talking about using a private methoddoResolveValue being exactly your currentresolveValue method (except for the renaming of course) and then having this for resolveValue:
publicfunctionresolveValue($value,$resolveEnvValues =false){return$this->doResolveValue($value,$resolveEnvValues,array());}
this way, we don't leak the internal argument to the public API (the fact that ParameterBag is leaking it is a mistake we made).
c5e3ef9 to9f55ad4Compare| * Replaces parameter placeholders (%name%) and unescapes percent signs. | ||
| * | ||
| * @param mixed $value A value | ||
| * @param mixed $resolveEnvValues Whether %env(VAR)% parameters should be replaced by the value of the corresponding environment variable or not |
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.
should be boolean, not mixed
cc1bd7d to0a7ec76Compare58d827e to6b7bf5cCompare6b7bf5c to74f0f00Compare…he values of referenced env vars.
74f0f00 to713b081Comparefabpot commentedDec 17, 2016
Thank you@nicolas-grekas. |
…ble to inline the values of referenced env vars. (nicolas-grekas)This PR was merged into the 3.3-dev branch.Discussion----------[DI] Make ContainerBuilder::resolveEnvPlaceholders() able to inline the values of referenced env vars.| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Being able to resolve environment variables at compile time as a replacement for `SYMFONY__` special env vars, unlocking their deprecation (see#20100).Commits-------713b081 [DI] Make ContainerBuilder::resolveEnvPlaceholders() able to inline the values of referenced env vars.
| } | ||
| if ($container->hasParameter('cache.prefix.seed')) { | ||
| // Inline any env vars referenced in the parameter | ||
| $container->setParameter('cache.prefix.seed',$container->resolveEnvPlaceholders($container->getParameter('cache.prefix.seed'),true)); |
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.
shouldn't this be done directly in theif above ?
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.
technically, it's possible to define the param before the framework extension, which is handled with this style
Uh oh!
There was an error while loading.Please reload this page.
Being able to resolve environment variables at compile time as a replacement for
SYMFONY__special env vars, unlocking their deprecation (see#20100).