Skip to content

Don't remove still-referenced files when updating recipes #1057

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from
Open

Don't remove still-referenced files when updating recipes #1057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Command/UpdateRecipesCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ 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());
Expand Down
28 changes: 25 additions & 3 deletions src/Update/RecipePatcher.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -39,11 +42,30 @@ public function __construct(string $rootDir, IOInterface $io)
public function applyPatch(RecipePatch $patch): bool
{
$withConflicts = $this->_applyPatchFile($patch);
$lockedFiles = array_count_values(array_merge(...array_column($this->symfonyLock->all(), '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])) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this be always true, since the file will be listed all the time because the currently updated recipe has it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would have to exclude the patched package from the creation of lockedFiles, so that only other recipes lock a file.

$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;
Expand Down
11 changes: 6 additions & 5 deletions tests/Update/RecipePatcherTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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"));
Expand Down Expand Up @@ -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']);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -261,7 +262,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) {
Expand Down Expand Up @@ -390,7 +391,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'],
Expand Down