From 1e9d87527fa56ea9fde94ab0c0f97d2f50f3e780 Mon Sep 17 00:00:00 2001 From: Michael Devery Date: Tue, 11 Oct 2016 04:25:54 +0100 Subject: [PATCH 1/4] [DependencyInjection] Fix duplication of placeholders when merging EnvPlaceholderParameterBags --- .../ParameterBag/EnvPlaceholderParameterBag.php | 10 +++++++++- .../EnvPlaceholderParameterBagTest.php | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php b/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php index 907acd9679cb0..b83234adb5880 100644 --- a/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php +++ b/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php @@ -62,9 +62,17 @@ public function getEnvPlaceholders() /** * Merges the env placeholders of another EnvPlaceholderParameterBag. + * + * @param EnvPlaceholderParameterBag $bag */ public function mergeEnvPlaceholders(self $bag) { - $this->envPlaceholders = array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders()); + $newPlaceholders = $bag->getEnvPlaceholders(); + + foreach ($newPlaceholders as $key => $newEntries) { + $existingEntries = isset($this->envPlaceholders[$key]) ? $this->envPlaceholders[$key] : array(); + $mergedEntries = array_merge($existingEntries, $newEntries); + $this->envPlaceholders[$key] = array_unique($mergedEntries); + } } } diff --git a/src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php b/src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php index 4412118b9006a..daa1f0459a99d 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php @@ -23,4 +23,19 @@ public function testGetThrowsInvalidArgumentExceptionIfEnvNameContainsNonWordCha $bag = new EnvPlaceholderParameterBag(); $bag->get('env(%foo%)'); } + + public function testMergeWillNotDuplicateIdenticalParameters() + { + $originalPlaceholders = array('database_host' => array('localhost')); + $firstBag = new EnvPlaceholderParameterBag($originalPlaceholders); + + // initialize placeholders + $firstBag->get('env(database_host)'); + $secondBag = clone $firstBag; + + $firstBag->mergeEnvPlaceholders($secondBag); + $mergedPlaceholders = $firstBag->getEnvPlaceholders(); + + $this->assertEquals(1, count($mergedPlaceholders['database_host'])); + } } From a0373ba5e95a6baff38626b164c34a2b3cbdfa69 Mon Sep 17 00:00:00 2001 From: Michael Devery Date: Tue, 11 Oct 2016 13:33:49 +0100 Subject: [PATCH 2/4] [DependencyInjection] Changes from code review in how placeholders are merged and set --- .../EnvPlaceholderParameterBag.php | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php b/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php index b83234adb5880..d81c908687fb8 100644 --- a/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php +++ b/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php @@ -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) { + return $placeholder; // return first result + } } if (preg_match('/\W/', $env)) { throw new InvalidArgumentException(sprintf('Invalid %s name: only "word" characters are allowed.', $name)); @@ -44,7 +46,11 @@ public function get($name) } } - return $this->envPlaceholders[$env][] = sprintf('env_%s_%s', $env, md5($name.uniqid(mt_rand(), true))); + $uniqueName = md5($name.uniqid(mt_rand(), true)); + $placeholder = sprintf('env_%s_%s', $env, $uniqueName); + $this->envPlaceholders[$env][$placeholder] = $placeholder; + + return $placeholder; } return parent::get($name); @@ -62,17 +68,12 @@ public function getEnvPlaceholders() /** * Merges the env placeholders of another EnvPlaceholderParameterBag. - * - * @param EnvPlaceholderParameterBag $bag */ public function mergeEnvPlaceholders(self $bag) { - $newPlaceholders = $bag->getEnvPlaceholders(); - - foreach ($newPlaceholders as $key => $newEntries) { - $existingEntries = isset($this->envPlaceholders[$key]) ? $this->envPlaceholders[$key] : array(); - $mergedEntries = array_merge($existingEntries, $newEntries); - $this->envPlaceholders[$key] = array_unique($mergedEntries); - } + $this->envPlaceholders = array_map( + 'array_unique', + array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders()) + ); } } From 92da498441839e32c71c4f5bc9afd338e105b5b5 Mon Sep 17 00:00:00 2001 From: Michael Devery Date: Tue, 11 Oct 2016 13:40:10 +0100 Subject: [PATCH 3/4] [DependencyInjection] Changes from code review to revert unnecessary change to mergeEnvPlaceholders method --- .../ParameterBag/EnvPlaceholderParameterBag.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php b/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php index d81c908687fb8..c0bfa33af43a5 100644 --- a/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php +++ b/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php @@ -71,9 +71,6 @@ public function getEnvPlaceholders() */ public function mergeEnvPlaceholders(self $bag) { - $this->envPlaceholders = array_map( - 'array_unique', - array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders()) - ); + $this->envPlaceholders = array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders()); } } From 574da5231b3c3582fe2904c78d3bac4f62cf276b Mon Sep 17 00:00:00 2001 From: Michael Devery Date: Tue, 11 Oct 2016 14:16:36 +0100 Subject: [PATCH 4/4] [DependencyInjection] Changes from code review to use assertCount instead of assertEquals and count --- .../Tests/ParameterBag/EnvPlaceholderParameterBagTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php b/src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php index daa1f0459a99d..2a85bbf99955a 100644 --- a/src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php +++ b/src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php @@ -36,6 +36,6 @@ public function testMergeWillNotDuplicateIdenticalParameters() $firstBag->mergeEnvPlaceholders($secondBag); $mergedPlaceholders = $firstBag->getEnvPlaceholders(); - $this->assertEquals(1, count($mergedPlaceholders['database_host'])); + $this->assertCount(1, $mergedPlaceholders['database_host']); } }