Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] resolve array env vars#27267
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
7af7268 to7af5d6cCompare| foreach ($envPlaceholdersas$env =>$placeholders) { | ||
| foreach ($placeholdersas$placeholder) { | ||
| if (false !==stripos($value,$placeholder)) { | ||
| if (\is_string($value) &&false !==stripos($value,$placeholder)) { |
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.
Is the added check useful? I feel like the added break is enough, isn't it?
jamesthomasonjrMay 14, 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.
Yeah, the break should be enough. I included theis_string() as a sanity check for thestripos() call, but it shouldn't be necessary. I can remove it if that's preferred.
nicolas-grekasMay 15, 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.
yes, let's remove the check as it might make ppl spend time wondering when this can be false, when it cannot :)
| /** | ||
| * @expectedException \Symfony\Component\DependencyInjection\Exception\RuntimeException | ||
| * @expectedExceptionMessage A string value must be composed of strings and/or numbers, but found parameter "env(ARRAY)" of type array inside string value "ABC%env(ARRAY)%". | ||
| * @expectedExceptionMessage A string value must be composed of strings and/or numbers, but found parameter "env(json:ARRAY)" of type array inside string value "ABCenv_json_ARRAY_ |
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.
isenv_json_ARRAY_ correct? Shouldn't it be"ABC %env(ARRAY)%"?
jamesthomasonjrMay 15, 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.
My first thought was thatenv_json_ARRAY_%s was expected due to the below code ofEnvPlaceholderParameterBag, but now I'm seeing it should only be called for environment variables that resolve to strings.
$uniqueName =md5($name.uniqid(mt_rand(),true));$placeholder =sprintf('env_%s_%s',str_replace(':','_',$env),$uniqueName);
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.
That's a bug that was hidden by the previous test case then.
$value should be processed by resolveEnvPlaceholders before being put in the exception message (with the default $format = null)
chalasr left a comment
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.
Nice first contribution
d084f73 to4c3b950Comparefabpot commentedMay 18, 2018
Thank you@jamesthomasonjr. |
…njr)This PR was squashed before being merged into the 3.4 branch (closes#27267).Discussion----------[DependencyInjection] resolve array env vars| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27239| License | MIT| Doc PR | n/a## WhyThis bugfix solves a problem where environment variables resolved as an array would cause an error while compiling the container if they aren't the last parameter in the ParameterBag: the next parameter to be resolved would fail at the `stripos()` check. More information about the bug is available at#27239## Tests- This PR modifies existing ContainerBuilder tests to make use of the EnvVarProcessor to resolve json strings into arrays, instead of relying upon a TestingEnvPlaceholderParameterBag class. - I would liked to have kept EnvVarProcessor logic out of the ContainerBuilder tests, but it was the interaction between the ContainerBuilder and EnvVarProcessor that caused the bug- This PR adds a new ContainerBuilder test to verify that an environment variable resolved into an array doesn't cause an error when the next variable attempts to be resolved## Code- ~This PR adds an `\is_string()` sanity check before the `stripos()` method call so that only a string are passed into `stripos()`~- This PR also adds a `$completed` flag so that completely resolved environment variables (currently only determined by `$placeholder === $value`) can break out of the loop early (handled via `break 2;`Commits-------4c3b950 [DependencyInjection] resolve array env vars
Uh oh!
There was an error while loading.Please reload this page.
Why
This bugfix solves a problem where environment variables resolved as an array would cause an error while compiling the container if they aren't the last parameter in the ParameterBag: the next parameter to be resolved would fail at the
stripos()check. More information about the bug is available at#27239Tests
Code
This PR adds an\is_string()sanity check before thestripos()method call so that only a string are passed intostripos()$completedflag so that completely resolved environment variables (currently only determined by$placeholder === $value) can break out of the loop early (handled viabreak 2;