[DI] Update Container::getEnv phpdoc #23889
Conversation
|
Why? There is at least one case that is broken by the forced cast to string: default value identification. If the default of an env is eg null, userland code is able to spot that, when it matters. |
|
Spotted while playing around with it really :) i qualified it a bug.. at least unexpected.
Honestly i thought we only allowed scalars/null because we can cast them to string, and so is a real possible env value. Im not sure about getting different types depending on real var existence yes/no. But preserving default null as a feature makes sense 👍 (as you can already set an explicit empty string).
Yes (twice :)) |
|
@nicolas-grekas not sure about moving to ContainerBuilder.. Container is constructed with a EnvPlaceholderParameterBag so this can happen in Container as well. Ive added a isCompiled check for that. |
| @@ -94,9 +94,9 @@ public function resolve() | |||
| if (!isset($this->parameters[$name = strtolower("env($env)")])) { | |||
| continue; | |||
| } | |||
| if (is_numeric($default = $this->parameters[$name])) { | |||
| if (is_scalar($default = $this->parameters[$name])) { | |||
There was a problem hiding this comment.
👎 for changing this class, we should not cast to string. There is no reason to do so.
There was a problem hiding this comment.
Can you explain the rationale... it already casts numbers to string. I assumed this should be any scalar; also null doesnt happen here due isset().
At this point cant we simply remove EnvPlaceholderParameterBag::resolve()?
| @@ -444,11 +446,29 @@ protected function getEnv($name) | |||
| if (false !== $env = getenv($name)) { | |||
| return $this->envCache[$name] = $env; | |||
| } | |||
| if (!$this->hasParameter("env($name)")) { | |||
| if (!$this->isCompiled() && $this->parameterBag instanceof EnvPlaceholderParameterBag) { | |||
There was a problem hiding this comment.
I don't think that's correct: when the container is not compiled, env values should not be used - only placeholders should be, so that we can latter on know where an env has been injected.
There was a problem hiding this comment.
Im not sure; isnt getEnv() always about getting real env values? If it's a EnvPlaceholderParameterBag we always need to get real value from parent::get.
Doing this from ContainerBuilder::getEnv will be a pain.
There was a problem hiding this comment.
Updated so it keeps calling getParameter(); thus registers placeholders.
|
read again, I think this PR is wrong, there is nothing to fix here, that's on purpose. |
|
Hm to be clear you propose to pass types as is? If you configure it's as a bool you get a bool, if you define a string you get a string? Until a real var exists... To me that's a trap and i will go with explicit string configs then =/ but imo SF should just cast (except for null) and force the type consistency. |
|
the only thing that might be changed is the "scalar" return type on getEnv, which should be "mixed" yes |
|
Updated to mixed return, fair enough 👍 |
|
@nicolas-grekas i reverted the most and kept @mixed return. This basically enables #20276 whereas currently it can happen if you mess with the paramater bag :) Only EnvBag enforces scalars, thats fine. |
|
Last; i think resolveEnvPlaceholders should not escape if compiled.. but im not sure it cares about state here. |
|
Replaced by #23899 |
Uh oh!
There was an error while loading. Please reload this page.