-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] Fix duplication of placeholders #20199
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…vPlaceholderParameterBags
@@ -62,9 +62,17 @@ public function getEnvPlaceholders() | |||
|
|||
/** | |||
* Merges the env placeholders of another EnvPlaceholderParameterBag. | |||
* | |||
* @param EnvPlaceholderParameterBag $bag |
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.
should be remove as it doesn't add any value when considering the method signature
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 added it because it looks like most of the docblocks for methods in the Symfony core either use {@inheritdoc}
or an explicit description of parameters. You're right (and I'm sure the clean code principles would agree with you) that it doesn't add anything by looking at it, although inside an IDE like PHPStorm people will use the quick documentation feature which makes it a little bit more understandable what the type parameter type is, but of course I can remove it - just let me know
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.
We generally prefer removing those, eventhough the code base is not always consistent on this aspect.
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.
Sure, I'll change it then. I'll wait a bit for your response to the other part before committing and pushing.
$this->envPlaceholders = array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders()); | ||
$newPlaceholders = $bag->getEnvPlaceholders(); | ||
|
||
foreach ($newPlaceholders as $key => $newEntries) { |
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.
What about this? should do the same isn't it?
$this->envPlaceholders = array_map('array_unique', array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders()));
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.
or what about another approach like this?
@@ -30,7 +30,9 @@ class EnvPlaceholderParameterBag extends ParameterBag
$env = substr($name, 4, -1);
if (isset($this->envPlaceholders[$env])) {
- return $this->envPlaceholders[$env][0];
+ foreach ($this->envPlaceholders[$env] as $placeholder) {
+ return $placeholder;
+ }
}
if (preg_match('/\W/', $env)) {
throw new InvalidArgumentException(sprintf('Invalid %s name: only "word" characters are allowed.', $name));
@@ -43,8 +45,9 @@ class EnvPlaceholderParameterBag extends ParameterBag
throw new RuntimeException(sprintf('The default value of an env() parameter must be scalar, but "%s" given to "%s".', gettype($defaultValue), $name));
}
}
+ $placeholder = sprintf('env_%s_%s', $env, md5($name.uniqid(mt_rand(), true)));
- return $this->envPlaceholders[$env][] = sprintf('env_%s_%s', $env, md5($name.uniqid(mt_rand(), true)));
+ return $this->envPlaceholders[$env][$placeholder] = $placeholder;
}
return parent::get($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.
About the array_map
, it works and it cuts some lines of code, but it is needless use of recursive functions and maybe not as clear, is it really needed?
For the changes inside get()
, why would you change from returning the 0th index to using foreach? If you think this should be changed how about something more readable like
return array_values($this->envPlaceholders[$placeholder])[0];
Finally for the changes in setting $this->envPlaceholders
I don't see the point in an array that will always have the structure
[
'apple' => 'apple',
'orange' => 'orange'
]
Plus (and I know it's going outside the scope of this PR) I don't think that assignment in a return statement is very clear and would prefer something like:
$uniqueName = md5($name.uniqid(mt_rand(), true));
$placeholder = sprintf('env_%s_%s', $environmentVariableName, $uniqueName);
$this->envPlaceholders[$environmentVariableName][] = $placeholder;
return $placeholder;
I made some naming changes locally because I also found env
confusing as it conflicts with Symfony environments but these don't have to be included in any PR changes.
Sorry to sound like a dick and that I don't want to change it, but I think I should at least be sure about the changes before I make them. I think it's a really cool feature and the only reason I'm here is because when I was working on changing my own small project to use the SYMFONY__ENV
variable style I came across this and thought it looks much better.
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.
needless use of recursive functions
not needless at all: the array is recursive (width two depth levels)
array_values($this->envPlaceholders[$placeholder])[0]
this would create a full copy of the entire array just to get the first element. foreach is the most efficient way to do so when not knowing the first key.
don't see the point in an array that will always have the structure
the point is using keys as a precomputed deduplication index, so that e.g. array_unique (slow compared to indexes) is not needed anymore.
I don't think that assignment in a return statement
no pb for that change, go for it
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 can see your point for most of those, but really using a 3 line foreach
to get the first element of an array seems like overkill. How about
return reset($this->envPlaceholders[$env]); // return first result
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.
reset
has two drawbacks: it works by reference, which I prefer to avoid, and it changes the internal state of the array, which is a not wanted side effect. Even if these could be of no importance in this case, I consider using it a worse practice...
@@ -65,6 +71,9 @@ public function getEnvPlaceholders() | |||
*/ | |||
public function mergeEnvPlaceholders(self $bag) | |||
{ | |||
$this->envPlaceholders = array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders()); | |||
$this->envPlaceholders = array_map( |
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.
this change is not needed anymore: now that keys are strings, they are already unique
…change to mergeEnvPlaceholders method
👍 |
$firstBag->mergeEnvPlaceholders($secondBag); | ||
$mergedPlaceholders = $firstBag->getEnvPlaceholders(); | ||
|
||
$this->assertEquals(1, count($mergedPlaceholders['database_host'])); |
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.
assertCount()
@@ -30,7 +30,9 @@ public function get($name) | |||
$env = substr($name, 4, -1); | |||
|
|||
if (isset($this->envPlaceholders[$env])) { | |||
return $this->envPlaceholders[$env][0]; | |||
foreach ($this->envPlaceholders[$env] as $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.
current()
?
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.
see #20199 (comment), same applies to current
…tead of assertEquals and count
Thank you @mickadoo. |
…kadoo) This PR was squashed before being merged into the 3.2-dev branch (closes #20199). Discussion ---------- [DependencyInjection] Fix duplication of placeholders | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? |no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20198 | License | MIT | Doc PR | reference to the documentation PR, if any Fixes performance issues related to merging parameter bags when using the `env()` parameters introduced in #19681 Commits ------- 124f30d [DependencyInjection] Fix duplication of placeholders
…adoo) This PR was squashed before being merged into the 3.2-dev branch (closes #20214). Discussion ---------- Fix/broken merging of parameter bag env placeholders | Q | A | ------------- | --- | Branch? | "master" | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20213 | License | MIT | Doc PR | reference to the documentation PR, if any In PR #20199 I made changes after review that broke the use of env variables, sorry about that - should have checked it a bit more before making the changes. @nicolas-grekas, I know you're very busy with all that merging, but if you could take a look at this it would be great since you know most about it. Commits ------- b53e0de Fix/broken merging of parameter bag env placeholders
Fixes performance issues related to merging parameter bags when using the
env()
parameters introduced in #19681