Merge pull request #207 from j0k3r/fix-bad-merge-find-replace-string

Change the way to merge config for find & replace string
j0k3r · Jun 27, 2019 · da49da0 · da49da0
2 parents 36a0f61 + 9336ff5
commit da49da0
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 4 deletions.
diff --git a/src/SiteConfig/ConfigBuilder.php b/src/SiteConfig/ConfigBuilder.php
@@ -308,10 +308,24 @@ public function mergeConfig(SiteConfig $currentConfig, SiteConfig $newConfig)
             }
         }
 
-        // treat find_string and replace_string separately (don't apply array_unique) (thanks fabrizio!)
-        foreach (['find_string', 'replace_string'] as $var) {
-            // append array elements for this config variable from $newConfig to this config
-            $currentConfig->$var = array_merge($currentConfig->$var, $newConfig->$var);
+        // Complex solution to ensure find_string & replace_string aren't duplicated when merging config multiple times
+        // We can't perform an array_unique on these values mostly because replace_string can have same values, example:
+        //      find_string: <amp-img
+        //      replace_string: <img
+        //      find_string: <other-img
+        //      replace_string: <img
+        // To fix that issue, we combine find & replace as key & value in one array, we merge them and then rebuild find & replace string in the current config
+        $findReplaceCurrentConfig = array_combine($currentConfig->find_string, $currentConfig->replace_string);
+        $findReplaceNewConfig = array_combine($newConfig->find_string, $newConfig->replace_string);
+        $findReplaceMerged = array_merge((array) $findReplaceCurrentConfig, (array) $findReplaceNewConfig);
+
+        // start from scratch
+        $currentConfig->find_string = [];
+        $currentConfig->replace_string = [];
+
+        foreach ($findReplaceMerged as $findString => $replaceString) {
+            array_push($currentConfig->find_string, $findString);
+            array_push($currentConfig->replace_string, $replaceString);
         }
 
         // merge http_header array from currentConfig into newConfig

diff --git a/tests/SiteConfig/ConfigBuilderTest.php b/tests/SiteConfig/ConfigBuilderTest.php
@@ -250,4 +250,28 @@ public function testWithBadHost()
 
         $this->assertInstanceOf('Graby\SiteConfig\SiteConfig', $res);
     }
+
+    /**
+     * Ensure merging config multiples times doesn't generate duplicate in replace_string / find_string.
+     */
+    public function testMergeConfigMultipleTimes()
+    {
+        $configBuilder = new ConfigBuilder([
+            'site_config' => [__DIR__ . '/../fixtures/site_config'],
+        ]);
+
+        $config1 = new SiteConfig();
+        $config1->find_string = ['toto'];
+        $config1->replace_string = ['titi'];
+
+        $config2 = new SiteConfig();
+        $config2->find_string = ['papa'];
+        $config2->replace_string = ['popo'];
+
+        $config3 = $configBuilder->mergeConfig($config1, $config2);
+        $config4 = $configBuilder->mergeConfig($config3, $config2);
+
+        $this->assertCount(2, $config4->find_string);
+        $this->assertCount(2, $config4->replace_string);
+    }
 }