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] Update Container::getEnv phpdoc#23889
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
nicolas-grekas commentedAug 14, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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. |
ro0NL commentedAug 14, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 :)) |
ro0NL commentedAug 15, 2017
@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. |
| continue; | ||
| } | ||
| if (is_numeric($default =$this->parameters[$name])) { | ||
| if (is_scalar($default =$this->parameters[$name])) { |
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.
👎 for changing this class, we should not cast to string. There is no reason to do so.
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.
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 removeEnvPlaceholderParameterBag::resolve()?
| return$this->envCache[$name] =$env; | ||
| } | ||
| if (!$this->hasParameter("env($name)")) { | ||
| if (!$this->isCompiled() &&$this->parameterBaginstanceof EnvPlaceholderParameterBag) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated so it keeps callinggetParameter(); thus registers placeholders.
nicolas-grekas commentedAug 15, 2017
read again, I think this PR is wrong, there is nothing to fix here, that's on purpose. |
ro0NL commentedAug 15, 2017
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. |
nicolas-grekas commentedAug 15, 2017
the only thing that might be changed is the "scalar" return type on getEnv, which should be "mixed" yes |
ro0NL commentedAug 15, 2017
Updated to mixed return, fair enough 👍 |
ro0NL commentedAug 15, 2017
@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. |
ro0NL commentedAug 15, 2017
Last; i think resolveEnvPlaceholders should not escape if compiled.. but im not sure it cares about state here. |
nicolas-grekas commentedAug 16, 2017
Replaced by#23899 |
Uh oh!
There was an error while loading.Please reload this page.