-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
{{ message }}
Fix/broken merging of parameter bag env placeholders #20214
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Nice! Works like a charm on my project ;) |
| public function mergeEnvPlaceholders(self $bag) | ||
| { | ||
| $this->envPlaceholders = array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders()); | ||
| $newPlaceholders = $bag->getEnvPlaceholders(); |
Member
There was a problem hiding this comment.
I propose keeping the above $placeholder index and use this here:
@@ -71,6 +71,12 @@ class EnvPlaceholderParameterBag extends ParameterBag
*/
public function mergeEnvPlaceholders(self $bag)
{
- $this->envPlaceholders = array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders());
+ if ($newPlaceholders = $bag->getEnvPlaceholders()) {
+ $this->envPlaceholders += $newPlaceholders;
+
+ foreach ($newPlaceholders as $env => $placeholders) {
+ $this->envPlaceholders[$env] += $placeholders;
+ }
+ }
}
}
Contributor
Author
There was a problem hiding this comment.
@nicolas-grekas Can you maybe explain the reason for the change here a bit more, I'm not seeing it.
With $this->envPlaceholders += $newPlaceholders; you're saying merge the new into the existing, fair enough. Then it's looping through each of the new ones and merging the existing ones with it? Won't this already have been done by the first merge?
Member
There was a problem hiding this comment.
The array is recursive, but the + isn't. So: the 1st + adds the new keys to the property, then + in the foreach adds the new entries to previously existing keys.
Contributor
Author
There was a problem hiding this comment.
In that case why not just use a single array_merge then?
(edit) bad suggestion. that would not merge the individual elements from, instead just overwrite them using the second value
Contributor
Author
There was a problem hiding this comment.
@nicolas-grekas I was trying to make a test for this but I need your input.
param bag A has placeholders:
'DB_USER' => ['env_DB_USER_1']
param bag B has placeholders:
'DB_USER' => ['env_DB_USER_1'], 'DB_HOST_1' => ['env_DB_HOST_1]
After the merge I would expect param bag A to have placeholders:
'DB_USER' => ['env_DB_USER_1'], 'DB_HOST_1' => ['env_DB_HOST_1]
right?
Now lets make things a little more interesting with param bag C which has a different unique identifier for one of the placeholders:
'DB_USER' => ['env_DB_USER_2']
So now we go and merge that into param bag A (after merging B). Do you expect:
A) 'DB_USER' => ['env_DB_USER_2'], 'DB_HOST_1' => ['env_DB_HOST_1]
or
B) 'DB_USER' => ['env_DB_USER_1', 'env_DB_USER_2'], 'DB_HOST_1' => ['env_DB_HOST_1]
If you're expecting B) like I am then I'll happily make your changes (which work as expected) once I reintroduce the $this->envPlaceholders[$env][$placeholder] = $placeholder; from above. If I left it without it your suggestion wouldn't work. The changes on my branch as they are now work in either case. I'm happy with both, using += looks nice and neat.
Just let me know the decision and i'll push the changes and the new test.
Sorry for the delay, just want to be sure I get this right this time.
Member
There was a problem hiding this comment.
B) is the only right answer. We though array_merge_recursive would do it, but you proved us wrong. Now we're trying to achieve B), nothing else :)
just want to be sure I get this right this time.
I ran my proposal with your tests, so we're safe, isn't it ;)
Contributor
Author
There was a problem hiding this comment.
Pushed changes, and this time I actually tried it out on a project. I think I put too much faith in the tests last time. That or I didn't write enough tests...
Are there tests for actual retrieving of a parameter after the bag has been merged?
Member
There was a problem hiding this comment.
Are there tests for actual retrieving of a parameter after the bag has been merged?
I don't think so
Member
|
👍 |
1 similar comment
Member
|
👍 |
Member
|
Thank you @mickadoo. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.