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] Remove default env type check on validate#27470
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
| 'scalar_node' =>'%env(NULLED)%', | ||
| 'int_node' =>'%env(int:FOO)%', | ||
| 'float_node' =>'%env(float:BAR)%', | ||
| 'float_node' =>'%env(FLOATISH)%', |
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.
currently this fails, im not sure what we expect here. Env without a prefix is now assumed to be a scalar which is not allowed for float-only node.
nicolas-grekasJun 1, 2018 • 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.
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 we useis_numeric() somewhere?
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.
It's a scalar issue in general, directly related to
symfony/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php
Lines 52 to 54 inc81f88f
| if (null !==$defaultValue && !is_scalar($defaultValue)) { | |
| thrownewRuntimeException(sprintf('The default value of an env() parameter must be scalar or null, but "%s" given to "%s".',gettype($defaultValue),$name)); | |
| } |
im not sure what's the behavior we're seeking... what does "env without prefix" mean on its value? It if can be any scalar then using it for a float node is invalid, technically.
to make it work we should simply avoid the type check in config for this case, but it makes validation less strict.
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.
Would it be possible to prefix a processor here? Referencing the envelope bar with float: prepended?
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, the thing is, without thefloat cast in the env var retrieval, the service would not receive a float, but a string. Sofloat_node not accepting%env(FLOATISH)% is expected to me.
ro0NL commentedJun 3, 2018
bc break resolved, instead of using a string typed placeholder it now preserves the actual env type |
nicolas-grekas commentedJun 3, 2018
Thank you@ro0NL. |
teohhanhui commentedJun 19, 2018
Will you add any tests to prevent regressions? :D |
ro0NL commentedJun 19, 2018
@teohhanhui what do you propose? tests are updated as such ... |
teohhanhui commentedJun 19, 2018
Sorry, I somehow overlooked that. 🙇♂️ |
SoboLAN commentedOct 30, 2018
I'm sorry, but this just broke my project and I had to spend like a few hours trying to figure out what was going on. In my config files I had stuff like: Those env-vars were defined in Upgrading Only by adding the Breaking BC like this isNOT OK, especially when incrementing only patch version !! |
ro0NL commentedOct 30, 2018 • 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.
it had to be done to prevent another BC break between 3.x and 4.x:#27455 so 4.1.0 is the wrong behavior, and fixed as of 4.1.6.. but i understand being in between was not convenient for you :( |
This PR was merged into the 4.3-dev branch.Discussion----------[DI] Deprecate non-string default envs| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes-ish| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | yes| Tests pass? | no <!-- please add some, will be required by reviewers -->| Fixed tickets | #27680,symfony/symfony#27470 (comment)| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->This is a failing test to further clarify the issues raised in #27680So givensymfony/symfony#27680 (comment)> We should be sure this solves a real-world issue.I think it solves a real bug :)Commits-------2311437c9f [DI] Deprecate non-string default envs
This PR was merged into the 4.3-dev branch.Discussion----------[DI] Deprecate non-string default envs| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes-ish| New feature? | yes| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | yes| Tests pass? | no <!-- please add some, will be required by reviewers -->| Fixed tickets |#27680,#27470 (comment)| License | MIT| Doc PR | symfony/symfony-docs#... <!-- required for new features -->This is a failing test to further clarify the issues raised in#27680So given#27680 (comment)> We should be sure this solves a real-world issue.I think it solves a real bug :)Commits-------2311437 [DI] Deprecate non-string default envs
The default env is already validated to be a scalar or null on
get()