bug #942 Fixing bug where removed parameters were not handled correct…

…ly during recipes update (weaverryan) This PR was merged into the 1.x branch. Discussion ---------- Fixing bug where removed parameters were not handled correctly during recipes update Hi again! Reported at https://symfonycasts.com/screencast/symfony6-upgrade/upgrade-recipes2#comment-5935619544 Just an oversight - the original implementation was a bit naive, not taking into account a situation where the `container` configurator in the OLD recipe contained a parameter, but this parameter was *removed* in the updated recipe. Cheers! Commits ------- 3032d76 Fixing bug where removed parameters were not handled correctly during recipes update
symfony · Aug 7, 2022 · acd9e39 · acd9e39
2 parents d1a6923 + 3032d76
commit acd9e39
Show file tree

Hide file tree

Showing 2 changed files with 94 additions and 26 deletions.
diff --git a/src/Configurator/ContainerConfigurator.php b/src/Configurator/ContainerConfigurator.php
@@ -34,31 +34,32 @@ public function unconfigure(Recipe $recipe, $parameters, Lock $lock)
     {
         $this->write('Unsetting parameters');
         $target = $this->options->get('root-dir').'/'.$this->getServicesPath();
-        $lines = [];
-        foreach (file($target) as $line) {
-            if ($this->removeParameters(1, $parameters, $line)) {
-                continue;
-            }
-            $lines[] = $line;
-        }
+        $lines = $this->removeParametersFromLines(file($target), $parameters);
         file_put_contents($target, implode('', $lines));
     }
 
     public function update(RecipeUpdate $recipeUpdate, array $originalConfig, array $newConfig): void
     {
-        if ($originalConfig) {
-            $recipeUpdate->setOriginalFile(
-                $this->getServicesPath(),
-                $this->configureParameters($originalConfig, true)
-            );
+        $recipeUpdate->setOriginalFile(
+            $this->getServicesPath(),
+            $this->configureParameters($originalConfig, true)
+        );
+
+        // for the new file, we need to update any values *and* remove any removed values
+        $removedParameters = [];
+        foreach ($originalConfig as $name => $value) {
+            if (!isset($newConfig[$name])) {
+                $removedParameters[$name] = $value;
+            }
         }
 
-        if ($newConfig) {
-            $recipeUpdate->setNewFile(
-                $this->getServicesPath(),
-                $this->configureParameters($newConfig, true)
-            );
-        }
+        $updatedFile = $this->configureParameters($newConfig, true);
+        $lines = $this->removeParametersFromLines(explode("\n", $updatedFile), $removedParameters);
+
+        $recipeUpdate->setNewFile(
+            $this->getServicesPath(),
+            implode("\n", $lines)
+        );
     }
 
     private function configureParameters(array $parameters, bool $update = false): string
@@ -114,6 +115,19 @@ private function configureParameters(array $parameters, bool $update = false): s
         return implode('', $lines);
     }
 
+    private function removeParametersFromLines(array $sourceLines, array $parameters): array
+    {
+        $lines = [];
+        foreach ($sourceLines as $line) {
+            if ($this->removeParameters(1, $parameters, $line)) {
+                continue;
+            }
+            $lines[] = $line;
+        }
+
+        return $lines;
+    }
+
     private function removeParameters($level, $params, $line)
     {
         foreach ($params as $key => $value) {

diff --git a/tests/Configurator/ContainerConfiguratorTest.php b/tests/Configurator/ContainerConfiguratorTest.php
@@ -57,7 +57,7 @@ public function testConfigure()
 services:
 
 EOF
-        , file_get_contents($config));
+            , file_get_contents($config));
 
         $configurator->unconfigure($recipe, ['locale' => 'en'], $lock);
         $this->assertEquals(<<<EOF
@@ -67,7 +67,7 @@ public function testConfigure()
 services:
 
 EOF
-        , file_get_contents($config));
+            , file_get_contents($config));
     }
 
     public function testConfigureWithoutParametersKey()
@@ -95,7 +95,7 @@ public function testConfigureWithoutParametersKey()
 services:
 
 EOF
-        , file_get_contents($config));
+            , file_get_contents($config));
 
         $configurator->unconfigure($recipe, ['locale' => 'en'], $lock);
         $this->assertEquals(<<<EOF
@@ -104,7 +104,7 @@ public function testConfigureWithoutParametersKey()
 services:
 
 EOF
-        , file_get_contents($config));
+            , file_get_contents($config));
     }
 
     public function testConfigureWithoutDuplicated()
@@ -135,7 +135,7 @@ public function testConfigureWithoutDuplicated()
 services:
 
 EOF
-        , file_get_contents($config));
+            , file_get_contents($config));
 
         $configurator->unconfigure($recipe, ['locale' => 'en'], $lock);
         $this->assertEquals(<<<EOF
@@ -144,7 +144,7 @@ public function testConfigureWithoutDuplicated()
 services:
 
 EOF
-        , file_get_contents($config));
+            , file_get_contents($config));
     }
 
     public function testConfigureWithComplexContent()
@@ -184,7 +184,7 @@ public function testConfigureWithComplexContent()
 services:
 
 EOF
-        , file_get_contents($config));
+            , file_get_contents($config));
 
         $configurator->unconfigure($recipe, ['locale' => 'en', 'foobar' => 'baz'], $lock);
         $this->assertEquals(<<<EOF
@@ -197,7 +197,7 @@ public function testConfigureWithComplexContent()
 services:
 
 EOF
-        , file_get_contents($config));
+            , file_get_contents($config));
     }
 
     public function testConfigureWithComplexContent2()
@@ -356,6 +356,60 @@ public function testUpdate()
 
 services:
 
+EOF
+        ], $recipeUpdate->getNewFiles());
+    }
+
+    public function testUpdateWithNoRemovedKeysInUpdate()
+    {
+        $configurator = new ContainerConfigurator(
+            $this->createMock(Composer::class),
+            $this->createMock(IOInterface::class),
+            new Options(['config-dir' => 'config', 'root-dir' => FLEX_TEST_DIR])
+        );
+
+        $recipeUpdate = new RecipeUpdate(
+            $this->createMock(Recipe::class),
+            $this->createMock(Recipe::class),
+            $this->createMock(Lock::class),
+            FLEX_TEST_DIR
+        );
+
+        @mkdir(FLEX_TEST_DIR.'/config');
+        file_put_contents(
+            FLEX_TEST_DIR.'/config/services.yaml',
+            <<<EOF
+parameters:
+    locale: es
+    something: else
+
+services:
+    foo_router: '@router'
+EOF
+        );
+
+        $configurator->update(
+            $recipeUpdate,
+            ['locale' => 'en'],
+            []
+        );
+
+        $this->assertSame(['config/services.yaml' => <<<EOF
+parameters:
+    locale: en
+    something: else
+
+services:
+    foo_router: '@router'
+EOF
+        ], $recipeUpdate->getOriginalFiles());
+
+        $this->assertSame(['config/services.yaml' => <<<EOF
+parameters:
+    something: else
+
+services:
+    foo_router: '@router'
 EOF
         ], $recipeUpdate->getNewFiles());
     }