bug #895 [recipes:update] Fixing bug where files failed to delete tha…

…t were modified previously (weaverryan) This PR was squashed before being merged into the 1.x branch. Discussion ---------- [recipes:update] Fixing bug where files failed to delete that were modified previously Hi! This fixes TWO `recipes:update` bugs: ## Bug 1️⃣ : sometimes deleted files caused patch to fail Small bug fix. The mystery is how I didn't catch this before... and how nobody seems to have hit this. The problem is fairly simple: A) The user gets a file (a long time ago) from a recipe (e.g. `config/bootstrap.php`). B) The user modifies (and commits) some change. C) A recipe update *deletes* that file. This, oddly, fails because the patch can't be applied. For example, when `config/packages/dev/framework.yaml` is deleted in `symfony/framework-bundle` recipe, this patch is correctly generated ```diff diff --git a/config/routes/dev/framework.yaml b/config/routes/dev/framework.yaml deleted file mode 100644 index bcbbf13..0000000 --- a/config/routes/dev/framework.yaml +++ /dev/null @@ -1,3 +0,0 @@ -_errors: - resource: '@FrameworkBundle/Resources/config/routing/errors.xml' - prefix: /_error ``` However, if the user's `framework.yaml` doesn't look EXACTLY like this, the patch will fail (not with a conflict like you might expect, it just completely fails to apply). The fix is quite simple: if a recipe update is *deleting* a file, instead of generating a "delete patch" for it, we run `git rm <filename>`. The downside is that the user won't get a nice "file conflict" if they ever modified the file... but apparently that is not possible. And the user will still review this change before they commit. ## Bug 2️⃣ : `bundles.php` environments didn't change If an upgraded recipe changed the environments that a bundle is configured in (e.g. https://github.com/symfony/recipes/pull/940/files), this was previously not taken into account: the update did not update the environments. Fixed now. Tested locally on a fairly complex project. Thanks! Commits ------- b33301a [recipes:update] Fixing bug where files failed to delete that were modified previously
symfony · Apr 15, 2022 · 2c857c0 · 2c857c0
2 parents 22c14c5 + b33301a
commit 2c857c0
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 76 deletions.
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -20,6 +20,7 @@ jobs:
             matrix:
                 include:
                   - php: '7.1'
+                    composer: 2.2.x
                   - php: '7.2'
                   - php: '7.3'
                   - php: '7.4'
@@ -33,11 +34,11 @@ jobs:
               uses: actions/[email protected]
 
             - name: "Install PHP with extensions"
-              uses: shivammathur/setup-php@2.7.0
+              uses: shivammathur/setup-php@2.18.0
               with:
                   coverage: "none"
                   php-version: ${{ matrix.php }}
-                  tools: composer:v2
+                  tools: composer:${{ matrix.composer }}
 
             - name: "Validate composer.json"
               run: "composer validate --strict --no-check-lock"

diff --git a/src/Configurator/BundlesConfigurator.php b/src/Configurator/BundlesConfigurator.php
@@ -44,20 +44,20 @@ public function unconfigure(Recipe $recipe, $bundles, Lock $lock)
 
     public function update(RecipeUpdate $recipeUpdate, array $originalConfig, array $newConfig): void
     {
-        $originalBundles = $this->configureBundles($originalConfig);
+        $originalBundles = $this->configureBundles($originalConfig, true);
         $recipeUpdate->setOriginalFile(
             $this->getLocalConfFile(),
             $this->buildContents($originalBundles)
         );
 
-        $newBundles = $this->configureBundles($newConfig);
+        $newBundles = $this->configureBundles($newConfig, true);
         $recipeUpdate->setNewFile(
             $this->getLocalConfFile(),
             $this->buildContents($newBundles)
         );
     }
 
-    private function configureBundles(array $bundles): array
+    private function configureBundles(array $bundles, bool $resetEnvironments = false): array
     {
         $file = $this->getConfFile();
         $registered = $this->load($file);
@@ -70,7 +70,15 @@ private function configureBundles(array $bundles): array
         }
         foreach ($classes as $class => $envs) {
             // do not override existing configured envs for a bundle
-            if (!isset($registered[$class])) {
+            if (!isset($registered[$class]) || $resetEnvironments) {
+                if ($resetEnvironments) {
+                    // used during calculating an "upgrade"
+                    // here, we want to "undo" the bundle's configuration entirely
+                    // then re-add it fresh, in case some environments have been
+                    // removed in an updated version of the recipe
+                    $registered[$class] = [];
+                }
+
                 foreach ($envs as $env) {
                     $registered[$class][$env] = true;
                 }

diff --git a/src/Update/RecipePatch.php b/src/Update/RecipePatch.php
@@ -15,12 +15,14 @@ class RecipePatch
 {
     private $patch;
     private $blobs;
+    private $deletedFiles;
     private $removedPatches;
 
-    public function __construct(string $patch, array $blobs, array $removedPatches = [])
+    public function __construct(string $patch, array $blobs, array $deletedFiles, array $removedPatches = [])
     {
         $this->patch = $patch;
         $this->blobs = $blobs;
+        $this->deletedFiles = $deletedFiles;
         $this->removedPatches = $removedPatches;
     }
 
@@ -34,6 +36,11 @@ public function getBlobs(): array
         return $this->blobs;
     }
 
+    public function getDeletedFiles(): array
+    {
+        return $this->deletedFiles;
+    }
+
     /**
      * Patches for modified files that were removed because the file
      * has been deleted in the user's project.

diff --git a/src/Update/RecipePatcher.php b/src/Update/RecipePatcher.php
@@ -38,40 +38,15 @@ public function __construct(string $rootDir, IOInterface $io)
      */
     public function applyPatch(RecipePatch $patch): bool
     {
-        if (!$patch->getPatch()) {
-            // nothing to do!
-            return true;
-        }
-
-        $addedBlobs = $this->addMissingBlobs($patch->getBlobs());
-
-        $patchPath = $this->rootDir.'/_flex_recipe_update.patch';
-        file_put_contents($patchPath, $patch->getPatch());
+        $withConflicts = $this->_applyPatchFile($patch);
 
-        try {
-            $this->execute('git update-index --refresh', $this->rootDir);
-
-            $output = '';
-            $statusCode = $this->processExecutor->execute('git apply "_flex_recipe_update.patch" -3', $output, $this->rootDir);
-
-            if (0 === $statusCode) {
-                // successful with no conflicts
-                return true;
-            }
-
-            if (false !== strpos($this->processExecutor->getErrorOutput(), 'with conflicts')) {
-                // successful with conflicts
-                return false;
-            }
-
-            throw new \LogicException('Error applying the patch: '.$this->processExecutor->getErrorOutput());
-        } finally {
-            unlink($patchPath);
-            // clean up any temporary blobs
-            foreach ($addedBlobs as $filename) {
-                unlink($filename);
+        foreach ($patch->getDeletedFiles() as $deletedFile) {
+            if (file_exists($this->rootDir.'/'.$deletedFile)) {
+                $this->execute(sprintf('git rm %s', ProcessExecutor::escape($deletedFile)), $this->rootDir);
             }
         }
+
+        return $withConflicts;
     }
 
     public function generatePatch(array $originalFiles, array $newFiles): RecipePatch
@@ -84,10 +59,13 @@ public function generatePatch(array $originalFiles, array $newFiles): RecipePatc
             return null !== $file;
         });
 
-        // find removed files and add them so they will be deleted
+        $deletedFiles = [];
+        // find removed files & record that they are deleted
+        // unset them from originalFiles to avoid unnecessary blobs being added
         foreach ($originalFiles as $file => $contents) {
             if (!isset($newFiles[$file])) {
-                $newFiles[$file] = null;
+                $deletedFiles[] = $file;
+                unset($originalFiles[$file]);
             }
         }
 
@@ -130,6 +108,7 @@ public function generatePatch(array $originalFiles, array $newFiles): RecipePatc
             return new RecipePatch(
                 $patchString,
                 $blobs,
+                $deletedFiles,
                 $removedPatches
             );
         } finally {
@@ -223,4 +202,42 @@ private function getBlobPath(string $hash): string
 
         return '.git/objects/'.$hashStart.'/'.$hashEnd;
     }
+
+    private function _applyPatchFile(RecipePatch $patch)
+    {
+        if (!$patch->getPatch()) {
+            // nothing to do!
+            return true;
+        }
+
+        $addedBlobs = $this->addMissingBlobs($patch->getBlobs());
+
+        $patchPath = $this->rootDir.'/_flex_recipe_update.patch';
+        file_put_contents($patchPath, $patch->getPatch());
+
+        try {
+            $this->execute('git update-index --refresh', $this->rootDir);
+
+            $output = '';
+            $statusCode = $this->processExecutor->execute('git apply "_flex_recipe_update.patch" -3', $output, $this->rootDir);
+
+            if (0 === $statusCode) {
+                // successful with no conflicts
+                return true;
+            }
+
+            if (false !== strpos($this->processExecutor->getErrorOutput(), 'with conflicts')) {
+                // successful with conflicts
+                return false;
+            }
+
+            throw new \LogicException('Error applying the patch: '.$this->processExecutor->getErrorOutput());
+        } finally {
+            unlink($patchPath);
+            // clean up any temporary blobs
+            foreach ($addedBlobs as $filename) {
+                unlink($filename);
+            }
+        }
+    }
 }
diff --git a/tests/Configurator/BundlesConfiguratorTest.php b/tests/Configurator/BundlesConfiguratorTest.php
@@ -176,7 +176,7 @@ public function testUpdate()
 
 return [
     BarBundle::class => ['prod' => false, 'all' => true],
-    FooBundle::class => ['dev' => true, 'test' => true],
+    FooBundle::class => ['all' => true],
     BazBundle::class => ['all' => true],
     NewBundle::class => ['all' => true],
 ];

diff --git a/tests/Update/RecipePatchTest.php b/tests/Update/RecipePatchTest.php
@@ -20,12 +20,14 @@ public function testBasicFunctioning()
     {
         $thePatch = 'the patch';
         $blobs = ['blob1', 'blob2', 'beware of the blob'];
+        $deletedFiles = ['old_file.txt'];
         $removedPatches = ['foo' => 'some diff'];
 
-        $patch = new RecipePatch($thePatch, $blobs, $removedPatches);
+        $patch = new RecipePatch($thePatch, $blobs, $deletedFiles, $removedPatches);
 
         $this->assertSame($thePatch, $patch->getPatch());
         $this->assertSame($blobs, $patch->getBlobs());
+        $this->assertSame($deletedFiles, $patch->getDeletedFiles());
         $this->assertSame($removedPatches, $patch->getRemovedPatches());
     }
 }
diff --git a/tests/Update/RecipePatcherTest.php b/tests/Update/RecipePatcherTest.php
@@ -31,7 +31,7 @@ protected function setUp(): void
     /**
      * @dataProvider getGeneratePatchTests
      */
-    public function testGeneratePatch(array $originalFiles, array $newFiles, string $expectedPatch)
+    public function testGeneratePatch(array $originalFiles, array $newFiles, string $expectedPatch, array $expectedDeletedFiles = [])
     {
         $this->getFilesystem()->remove(FLEX_TEST_DIR);
         $this->getFilesystem()->mkdir(FLEX_TEST_DIR);
@@ -44,6 +44,7 @@ public function testGeneratePatch(array $originalFiles, array $newFiles, string
 
         $patch = $patcher->generatePatch($originalFiles, $newFiles);
         $this->assertSame($expectedPatch, rtrim($patch->getPatch(), "\n"));
+        $this->assertSame($expectedDeletedFiles, $patch->getDeletedFiles());
 
         // find all "index 7d30dc7.." in patch
         $matches = [];
@@ -121,31 +122,15 @@ public function getGeneratePatchTests(): iterable
         yield 'file_deleted_in_update_because_missing' => [
             ['file1.txt' => 'New file'],
             [],
-            <<<EOF
-diff --git a/file1.txt b/file1.txt
-deleted file mode 100644
-index b78ca63..0000000
---- a/file1.txt
-+++ /dev/null
-@@ -1 +0,0 @@
--New file
-\ No newline at end of file
-EOF
+            '',
+            ['file1.txt'],
         ];
 
         yield 'file_deleted_in_update_because_null' => [
             ['file1.txt' => 'New file'],
             ['file1.txt' => null],
-            <<<EOF
-diff --git a/file1.txt b/file1.txt
-deleted file mode 100644
-index b78ca63..0000000
---- a/file1.txt
-+++ /dev/null
-@@ -1 +0,0 @@
--New file
-\ No newline at end of file
-EOF
+            '',
+            ['file1.txt'],
         ];
 
         yield 'mixture_of_added_updated_removed' => [
@@ -169,15 +154,9 @@ public function getGeneratePatchTests(): iterable
 @@ -0,0 +1 @@
 +file to create
 \ No newline at end of file
-diff --git a/will_be_deleted.txt b/will_be_deleted.txt
-deleted file mode 100644
-index 98ff166..0000000
---- a/will_be_deleted.txt
-+++ /dev/null
-@@ -1 +0,0 @@
--file to delete
-\ No newline at end of file
 EOF
+        ,
+            ['will_be_deleted.txt'],
         ];
     }
 
@@ -243,7 +222,8 @@ public function getApplyPatchTests(): iterable
             ['.env' => $dotEnvClean['in_app']],
             new RecipePatch(
                 $dotEnvClean['patch'],
-                [$dotEnvClean['hash'] => $dotEnvClean['blob']]
+                [$dotEnvClean['hash'] => $dotEnvClean['blob']],
+                []
             ),
             ['.env' => $dotEnvClean['expected']],
             false,
@@ -253,7 +233,8 @@ public function getApplyPatchTests(): iterable
             ['package.json' => $packageJsonConflict['in_app']],
             new RecipePatch(
                 $packageJsonConflict['patch'],
-                [$packageJsonConflict['hash'] => $packageJsonConflict['blob']]
+                [$packageJsonConflict['hash'] => $packageJsonConflict['blob']],
+                []
             ),
             ['package.json' => $packageJsonConflict['expected']],
             true,
@@ -265,6 +246,7 @@ public function getApplyPatchTests(): iterable
             new RecipePatch(
                 $webpackEncoreAdded['patch'],
                 // no blobs needed for a new file
+                [],
                 []
             ),
             ['config/packages/webpack_encore.yaml' => $webpackEncoreAdded['expected']],
@@ -274,8 +256,9 @@ public function getApplyPatchTests(): iterable
         yield 'removed_one_file' => [
             ['config/packages/security.yaml' => $securityRemoved['in_app']],
             new RecipePatch(
-                $securityRemoved['patch'],
-                [$securityRemoved['hash'] => $securityRemoved['blob']]
+                '',
+                [$securityRemoved['hash'] => $securityRemoved['blob']],
+                ['config/packages/security.yaml']
             ),
             // expected to be deleted
             ['config/packages/security.yaml' => null],
@@ -290,12 +273,15 @@ public function getApplyPatchTests(): iterable
                 'config/packages/security.yaml' => $securityRemoved['in_app'],
             ],
             new RecipePatch(
-                $dotEnvClean['patch']."\n".$packageJsonConflict['patch']."\n".$webpackEncoreAdded['patch']."\n".$securityRemoved['patch'],
+                $dotEnvClean['patch']."\n".$packageJsonConflict['patch']."\n".$webpackEncoreAdded['patch'],
                 [
                     $dotEnvClean['hash'] => $dotEnvClean['blob'],
                     $packageJsonConflict['hash'] => $packageJsonConflict['blob'],
                     $webpackEncoreAdded['hash'] => $webpackEncoreAdded['blob'],
                     $securityRemoved['hash'] => $securityRemoved['blob'],
+                ],
+                [
+                    'config/packages/security.yaml',
                 ]
             ),
             [