Fix/broken merging of parameter bag env placeholders

mickadoo · nicolas-grekas · commit b53e0de8f02b · 2016-10-14T11:11:06.000+02:00
diff --git a/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php b/src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php
@@ -71,6 +71,12 @@ public function getEnvPlaceholders()
      */
     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;
+            }
+        }
     }
 }
diff --git a/src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php b/src/Symfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php
@@ -26,16 +26,87 @@ public function testGetThrowsInvalidArgumentExceptionIfEnvNameContainsNonWordCha
 
     public function testMergeWillNotDuplicateIdenticalParameters()
     {
-        $originalPlaceholders = array('database_host' => array('localhost'));
-        $firstBag = new EnvPlaceholderParameterBag($originalPlaceholders);
+        $envVariableName = 'DB_HOST';
+        $parameter = sprintf('env(%s)', $envVariableName);
+        $firstBag = new EnvPlaceholderParameterBag();
 
         // initialize placeholders
-        $firstBag->get('env(database_host)');
+        $firstBag->get($parameter);
         $secondBag = clone $firstBag;
 
         $firstBag->mergeEnvPlaceholders($secondBag);
         $mergedPlaceholders = $firstBag->getEnvPlaceholders();
 
-        $this->assertCount(1, $mergedPlaceholders['database_host']);
+        $placeholderForVariable = $mergedPlaceholders[$envVariableName];
+        $placeholder = array_values($placeholderForVariable)[0];
+
+        $this->assertCount(1, $placeholderForVariable);
+        $this->assertInternalType('string', $placeholder);
+        $this->assertContains($envVariableName, $placeholder);
+    }
+
+    public function testMergeWhereFirstBagIsEmptyWillWork()
+    {
+        $envVariableName = 'DB_HOST';
+        $parameter = sprintf('env(%s)', $envVariableName);
+        $firstBag = new EnvPlaceholderParameterBag();
+        $secondBag = new EnvPlaceholderParameterBag();
+
+        // initialize placeholder only in second bag
+        $secondBag->get($parameter);
+
+        $this->assertEmpty($firstBag->getEnvPlaceholders());
+
+        $firstBag->mergeEnvPlaceholders($secondBag);
+        $mergedPlaceholders = $firstBag->getEnvPlaceholders();
+
+        $placeholderForVariable = $mergedPlaceholders[$envVariableName];
+        $placeholder = array_values($placeholderForVariable)[0];
+
+        $this->assertCount(1, $placeholderForVariable);
+        $this->assertInternalType('string', $placeholder);
+        $this->assertContains($envVariableName, $placeholder);
+    }
+
+    public function testMergeWherePlaceholderOnlyExistsInSecond()
+    {
+        $uniqueEnvName = 'DB_HOST';
+        $commonEnvName = 'DB_USER';
+
+        $uniqueParamName = sprintf('env(%s)', $uniqueEnvName);
+        $commonParamName = sprintf('env(%s)', $commonEnvName);
+
+        $firstBag = new EnvPlaceholderParameterBag();
+        // initialize common placeholder
+        $firstBag->get($commonParamName);
+        $secondBag = clone $firstBag;
+
+        // initialize unique placeholder
+        $secondBag->get($uniqueParamName);
+
+        $firstBag->mergeEnvPlaceholders($secondBag);
+        $merged = $firstBag->getEnvPlaceholders();
+
+        $this->assertCount(1, $merged[$uniqueEnvName]);
+        // second bag has same placeholder for commonEnvName
+        $this->assertCount(1, $merged[$commonEnvName]);
+    }
+
+    public function testMergeWithDifferentIdentifiersForPlaceholders()
+    {
+        $envName = 'DB_USER';
+        $paramName = sprintf('env(%s)', $envName);
+
+        $firstBag = new EnvPlaceholderParameterBag();
+        $secondBag = new EnvPlaceholderParameterBag();
+        // initialize placeholders
+        $firstPlaceholder = $firstBag->get($paramName);
+        $secondPlaceholder = $secondBag->get($paramName);
+
+        $firstBag->mergeEnvPlaceholders($secondBag);
+        $merged = $firstBag->getEnvPlaceholders();
+
+        $this->assertNotEquals($firstPlaceholder, $secondPlaceholder);
+        $this->assertCount(2, $merged[$envName]);
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -71,6 +71,12 @@ public function getEnvPlaceholders()`
`71`	`71`	`*/`
`72`	`72`	`public function mergeEnvPlaceholders(self $bag)`
`73`	`73`	`{`
`74`		`- $this->envPlaceholders = array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders());`
	`74`	`+ if ($newPlaceholders = $bag->getEnvPlaceholders()) {`
	`75`	`+ $this->envPlaceholders += $newPlaceholders;`
	`76`	`+`
	`77`	`+ foreach ($newPlaceholders as $env => $placeholders) {`
	`78`	`+ $this->envPlaceholders[$env] += $placeholders;`
	`79`	`+ }`
	`80`	`+ }`
`75`	`81`	`}`
`76`	`82`	`}`