[DI] Make ContainerBuilder::resolveEnvPlaceholders() able to inline the values of referenced env vars. #20618
Conversation
56f0257 to
bbcae3c
Compare
bbcae3c to
64bd54a
Compare
| @@ -384,9 +384,18 @@ protected function loadCacheDriver($cacheName, $objectManagerName, array $cacheD | |||
|
|
|||
| if (!isset($cacheDriver['namespace'])) { | |||
| // 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.
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.
Doing so makes the code simpler. Do you think this can have any adverse side effect?
| * | ||
| * @return mixed The resolved value | ||
| */ | ||
| public function resolveValue($value, $resolveEnvValues = false, array $resolving = array()) |
There was a problem hiding this comment.
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.
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.
Well, I'm not talking about adding a private property. I'm talking about using a private method doResolveValue being exactly your current resolveValue method (except for the renaming of course) and then having this for resolveValue:
public function resolveValue($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 to
9f55ad4
Compare
| * 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 |
cc1bd7d to
0a7ec76
Compare
58d827e to
6b7bf5c
Compare
6b7bf5c to
74f0f00
Compare
…he values of referenced env vars.
74f0f00 to
713b081
Compare
|
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.
| @@ -1241,6 +1241,10 @@ private function registerCacheConfiguration(array $config, ContainerBuilder $con | |||
| if (isset($config['prefix_seed'])) { | |||
| $container->setParameter('cache.prefix.seed', $config['prefix_seed']); | |||
| } | |||
| 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.
shouldn't this be done directly in the if above ?
There was a problem hiding this comment.
technically, it's possible to define the param before the framework extension, which is handled with this style
Being able to resolve environment variables at compile time as a replacement for
SYMFONY__special env vars, unlocking their deprecation (see #20100).