Fixing bug where removed parameters were not handled correctly during recipes update

Author: weaverryan

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) {

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());
     }