bug #1057 Don't remove still-referenced files when updating recipes (wouterj)

This PR was merged into the 2.x branch.

Discussion
----------

Don't remove still-referenced files when updating recipes

A twin to #1054

When a recipe removes a file in a newer recipe version, we should also check if other recipes still reference that file.

Real use-case: This occurs when you need to keep the PhpunitBridge in 7.3 (e.g. you need the clock mock).

Commits
-------

03ac318 Don't remove still-referenced files when updating recipes

Author: fabpot

src/Command/UpdateRecipesCommand.php

@@ -153,11 +153,11 @@ protected function execute(InputInterface $input, OutputInterface $output): int
         $recipeUpdate = new RecipeUpdate($originalRecipe, $newRecipe, $symfonyLock, $this->rootDir);
         $this->configurator->populateUpdate($recipeUpdate);
         $originalComposerJsonHash = $this->flex->getComposerJsonHash();
-        $patcher = new RecipePatcher($this->rootDir, $io);
+        $patcher = new RecipePatcher($this->rootDir, $io, $symfonyLock);
 
         try {
             $patch = $patcher->generatePatch($recipeUpdate->getOriginalFiles(), $recipeUpdate->getNewFiles());
-            $hasConflicts = !$patcher->applyPatch($patch);
+            $hasConflicts = !$patcher->applyPatch($patch, $packageName);
         } catch (\Throwable $throwable) {
             $io->writeError([
                 '<bg=red;fg=white>There was an error applying the recipe update patch</>',

src/Update/RecipePatcher.php

@@ -15,19 +15,22 @@
 use Composer\Util\ProcessExecutor;
 use Symfony\Component\Filesystem\Exception\IOException;
 use Symfony\Component\Filesystem\Filesystem;
+use Symfony\Flex\Lock;
 
 class RecipePatcher
 {
     private $rootDir;
     private $filesystem;
     private $io;
     private $processExecutor;
+    private $symfonyLock;
 
-    public function __construct(string $rootDir, IOInterface $io)
+    public function __construct(string $rootDir, IOInterface $io, Lock $symfonyLock)
     {
         $this->rootDir = $rootDir;
         $this->filesystem = new Filesystem();
         $this->io = $io;
+        $this->symfonyLock = $symfonyLock;
         $this->processExecutor = new ProcessExecutor($io);
     }
 
@@ -36,14 +39,33 @@ public function __construct(string $rootDir, IOInterface $io)
      *
      * @return bool returns true if fully successful, false if conflicts were encountered
      */
-    public function applyPatch(RecipePatch $patch): bool
+    public function applyPatch(RecipePatch $patch, ?string $packageName = null): bool
     {
         $withConflicts = $this->_applyPatchFile($patch);
+        $lockedFiles = $packageName ? array_count_values(array_merge(...array_column(array_filter($this->symfonyLock->all(), fn ($package) => $package !== $packageName, \ARRAY_FILTER_USE_KEY), 'files'))) : [];
 
+        $nonRemovableFiles = [];
         foreach ($patch->getDeletedFiles() as $deletedFile) {
-            if (file_exists($this->rootDir.'/'.$deletedFile)) {
-                $this->execute(\sprintf('git rm %s', ProcessExecutor::escape($deletedFile)), $this->rootDir);
+            if (!file_exists($this->rootDir.'/'.$deletedFile)) {
+                continue;
+            }
+
+            if (isset($lockedFiles[$deletedFile])) {
+                $nonRemovableFiles[] = $deletedFile;
+
+                continue;
             }
+
+            $this->execute(\sprintf('git rm %s', ProcessExecutor::escape($deletedFile)), $this->rootDir);
+        }
+
+        if ($nonRemovableFiles) {
+            $this->io->writeError('  <warning>The following files were removed in the recipe, but are still referenced by other recipes. You might need to adjust them manually:</warning>');
+            foreach ($nonRemovableFiles as $file) {
+                $this->io->writeError('      - '.$file);
+            }
+
+            $this->io->writeError('');
         }
 
         return $withConflicts;

tests/Update/RecipePatcherTest.php

@@ -15,6 +15,7 @@
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Filesystem\Filesystem;
 use Symfony\Component\Process\Process;
+use Symfony\Flex\Lock;
 use Symfony\Flex\Update\RecipePatch;
 use Symfony\Flex\Update\RecipePatcher;
 
@@ -52,7 +53,7 @@ public function testGeneratePatch(array $originalFiles, array $newFiles, string
             (new Process(['git', 'commit', '-m', '"original files"'], FLEX_TEST_DIR))->mustRun();
         }
 
-        $patcher = new RecipePatcher(FLEX_TEST_DIR, $this->createMock(IOInterface::class));
+        $patcher = new RecipePatcher(FLEX_TEST_DIR, $this->createMock(IOInterface::class), $this->createMock(Lock::class));
 
         $patch = $patcher->generatePatch($originalFiles, $newFiles);
         $this->assertSame($expectedPatch, rtrim($patch->getPatch(), "\n"));
@@ -189,7 +190,7 @@ public function testGeneratePatchOnDeletedFile()
         $this->getFilesystem()->remove(FLEX_TEST_DIR);
         $this->getFilesystem()->mkdir(FLEX_TEST_DIR);
 
-        $patcher = new RecipePatcher(FLEX_TEST_DIR, $this->createMock(IOInterface::class));
+        $patcher = new RecipePatcher(FLEX_TEST_DIR, $this->createMock(IOInterface::class), $this->createMock(Lock::class));
 
         // try to update a file that does not exist in the project
         $patch = $patcher->generatePatch(['.env' => 'original contents'], ['.env' => 'new contents']);
@@ -217,7 +218,7 @@ public function testApplyPatch(array $filesCurrentlyInApp, RecipePatch $recipePa
             (new Process(['git', 'commit', '-m', 'Committing original files'], FLEX_TEST_DIR))->mustRun();
         }
 
-        $patcher = new RecipePatcher(FLEX_TEST_DIR, $this->createMock(IOInterface::class));
+        $patcher = new RecipePatcher(FLEX_TEST_DIR, $this->createMock(IOInterface::class), $this->createMock(Lock::class));
         $hadConflicts = !$patcher->applyPatch($recipePatch);
 
         foreach ($expectedFiles as $file => $expectedContents) {
@@ -233,6 +234,38 @@ public function testApplyPatch(array $filesCurrentlyInApp, RecipePatch $recipePa
         $this->assertSame($expectedConflicts, $hadConflicts);
     }
 
+    public function testApplyPatchFileOwnedByMultipleRecipes()
+    {
+        (new Process(['git', 'init'], FLEX_TEST_DIR))->mustRun();
+        (new Process(['git', 'config', 'user.name', 'Unit test'], FLEX_TEST_DIR))->mustRun();
+        (new Process(['git', 'config', 'user.email', ''], FLEX_TEST_DIR))->mustRun();
+
+        $dir = FLEX_TEST_DIR.'/config/packages';
+        @mkdir($dir, 0777, true);
+        file_put_contents($dir.'/security.yaml', '# contents');
+        (new Process(['git', 'add', '-A'], FLEX_TEST_DIR))->mustRun();
+        (new Process(['git', 'commit', '-m', 'Committing original files'], FLEX_TEST_DIR))->mustRun();
+
+
+        $lock = $this->createMock(Lock::class);
+        $lock->expects($this->any())->method('all')->willReturn([
+            'symfony/security-bundle' => ['files' => ['config/packages/security.yaml']],
+            'symfony/security' => ['files' => ['config/packages/security.yaml']],
+        ]);
+        $patcher = new RecipePatcher(FLEX_TEST_DIR, $this->createMock(IOInterface::class), $lock);
+
+        $patchData = $this->generatePatchData('config/packages/security.yaml', '# contents', null);
+        $hadConflicts = !$patcher->applyPatch(new RecipePatch(
+            '',
+            [$patchData['hash'] => $patchData['blob']],
+            ['config/packages/security.yaml']
+        ), 'symfony/security-bundle');
+
+        $this->assertFileExists($dir.'/security.yaml');
+        $this->assertSame('# contents', file_get_contents($dir.'/security.yaml'));
+        $this->assertFalse($hadConflicts);
+    }
+
     /**
      * @dataProvider getApplyPatchTests
      */
@@ -261,7 +294,7 @@ public function testApplyPatchOnSubfolder(array $filesCurrentlyInApp, RecipePatc
             (new Process(['git', 'commit', '-m', 'Committing original files'], $subProjectPath))->mustRun();
         }
 
-        $patcher = new RecipePatcher($subProjectPath, $this->createMock(IOInterface::class));
+        $patcher = new RecipePatcher($subProjectPath, $this->createMock(IOInterface::class), $this->createMock(Lock::class));
         $hadConflicts = !$patcher->applyPatch($recipePatch);
 
         foreach ($expectedFiles as $file => $expectedContents) {
@@ -390,7 +423,7 @@ public function testIntegration(bool $useNullForMissingFiles)
         (new Process(['git', 'add', '-A'], FLEX_TEST_DIR))->mustRun();
         (new Process(['git', 'commit', '-m', 'committing in app start files'], FLEX_TEST_DIR))->mustRun();
 
-        $patcher = new RecipePatcher(FLEX_TEST_DIR, $this->createMock(IOInterface::class));
+        $patcher = new RecipePatcher(FLEX_TEST_DIR, $this->createMock(IOInterface::class), $this->createMock(Lock::class));
         $originalFiles = [
             '.env' => $files['dot_env_clean']['original_recipe'],
             'package.json' => $files['package_json_conflict']['original_recipe'],