Skip to content

Commit 4ae50d3

Browse files
nicolas-grekasnicolas-grekas
committed
bug #1054 Don't remove still-referenced files when unconfiguring recipes (nicolas-grekas)
This PR was merged into the 2.x branch. Discussion ---------- Don't remove still-referenced files when unconfiguring recipes Instead of #706 Fixes broken logic added in #451, which didn't account for folders. The new logic uses only the symfony.lock file and not the recipe anymore to decide which files to remove. Commits ------- 6443e31 Don't remove still-referenced files when unconfiguring recipes
2 parents dcd0c73 + 6443e31 commit 4ae50d3

File tree

4 files changed

+71
-71
lines changed

4 files changed

+71
-71
lines changed

‎src/Configurator/CopyFromRecipeConfigurator.php‎

Lines changed: 7 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,13 @@ public function configure(Recipe $recipe, $config, Lock $lock, array $options =
3131
public function unconfigure(Recipe $recipe, $config, Lock $lock)
3232
{
3333
$this->write('Removing files from recipe');
34-
$this->removeFiles($config, $this->getRemovableFilesFromRecipeAndLock($recipe, $lock), $this->options->get('root-dir'));
34+
$rootDir = $this->options->get('root-dir');
35+
36+
foreach ($this->options->getRemovableFiles($recipe, $lock) as $file) {
37+
if ('.git' !== $file) { // never remove the main Git directory, even if it was created by a recipe
38+
$this->removeFile($this->path->concatenate([$rootDir, $file]));
39+
}
40+
}
3541
}
3642

3743
public function update(RecipeUpdate $recipeUpdate, array $originalConfig, array $newConfig): void
@@ -66,32 +72,6 @@ private function resolveTargetFolder(string $path, array $config): string
6672
return $path;
6773
}
6874

69-
private function getRemovableFilesFromRecipeAndLock(Recipe $recipe, Lock $lock): array
70-
{
71-
$lockedFiles = array_unique(
72-
array_reduce(
73-
array_column($lock->all(), 'files'),
74-
function (array $carry, array $package) {
75-
return array_merge($carry, $package);
76-
},
77-
[]
78-
)
79-
);
80-
81-
$removableFiles = $recipe->getFiles();
82-
83-
$lockedFiles = array_map('realpath', $lockedFiles);
84-
85-
// Compare file paths by their real path to abstract OS differences
86-
foreach (array_keys($removableFiles) as $file) {
87-
if (\in_array(realpath($file), $lockedFiles)) {
88-
unset($removableFiles[$file]);
89-
}
90-
}
91-
92-
return $removableFiles;
93-
}
94-
9575
private function copyFiles(array $manifest, array $files, array $options): array
9676
{
9777
$copiedFiles = [];
@@ -148,28 +128,6 @@ private function copyFile(string $to, string $contents, bool $executable, array
148128
return $copiedFile;
149129
}
150130

151-
private function removeFiles(array $manifest, array $files, string $to)
152-
{
153-
foreach ($manifest as $source => $target) {
154-
$target = $this->options->expandTargetDir($target);
155-
156-
if ('.git' === $target) {
157-
// never remove the main Git directory, even if it was created by a recipe
158-
continue;
159-
}
160-
161-
if ('/' === substr($source, -1)) {
162-
foreach (array_keys($files) as $file) {
163-
if (str_starts_with($file, $source)) {
164-
$this->removeFile($this->path->concatenate([$to, $target, substr($file, \strlen($source))]));
165-
}
166-
}
167-
} else {
168-
$this->removeFile($this->path->concatenate([$to, $target]));
169-
}
170-
}
171-
}
172-
173131
private function removeFile(string $to)
174132
{
175133
if (!file_exists($to)) {

‎src/Flex.php‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -111,13 +111,19 @@ class_exists(__NAMESPACE__.str_replace('/', '\\', substr($file, \strlen(__DIR__)
111111
$this->composer = $composer;
112112
$this->io = $io;
113113
$this->config = $composer->getConfig();
114+
115+
$composerFile = Factory::getComposerFile();
116+
$composerLock = 'json' === pathinfo($composerFile, \PATHINFO_EXTENSION) ? substr($composerFile, 0, -4).'lock' : $composerFile.'.lock';
117+
$symfonyLock = str_replace('composer', 'symfony', basename($composerLock));
118+
119+
$this->lock = new Lock(getenv('SYMFONY_LOCKFILE') ?: \dirname($composerLock).'/'.(basename($composerLock) !== $symfonyLock ? $symfonyLock : 'symfony.lock'));
114120
$this->options = $this->initOptions();
115121

116122
// if Flex is being upgraded, the original operations from the original Flex
117123
// instance are stored in the static property, so we can reuse them now.
118-
if (property_exists(self::class, 'storedOperations') && self::$storedOperations) {
119-
$this->operations = self::$storedOperations;
120-
self::$storedOperations = [];
124+
if (property_exists(Flex::class, 'storedOperations') && Flex::$storedOperations) {
125+
$this->operations = Flex::$storedOperations;
126+
Flex::$storedOperations = [];
121127
}
122128

123129
$symfonyRequire = preg_replace('/\.x$/', '.x-dev', getenv('SYMFONY_REQUIRE') ?: ($composer->getPackage()->getExtra()['symfony']['require'] ?? ''));
@@ -130,12 +136,7 @@ class_exists(__NAMESPACE__.str_replace('/', '\\', substr($file, \strlen(__DIR__)
130136
$this->filter = new PackageFilter($io, $symfonyRequire, $this->downloader);
131137
}
132138

133-
$composerFile = Factory::getComposerFile();
134-
$composerLock = 'json' === pathinfo($composerFile, \PATHINFO_EXTENSION) ? substr($composerFile, 0, -4).'lock' : $composerFile.'.lock';
135-
$symfonyLock = str_replace('composer', 'symfony', basename($composerLock));
136-
137139
$this->configurator = new Configurator($composer, $io, $this->options);
138-
$this->lock = new Lock(getenv('SYMFONY_LOCKFILE') ?: \dirname($composerLock).'/'.(basename($composerLock) !== $symfonyLock ? $symfonyLock : 'symfony.lock'));
139140

140141
$disable = true;
141142
foreach (array_merge($composer->getPackage()->getRequires() ?? [], $composer->getPackage()->getDevRequires() ?? []) as $link) {
@@ -210,8 +211,9 @@ class_exists(__NAMESPACE__.str_replace('/', '\\', substr($file, \strlen(__DIR__)
210211
*/
211212
public function deactivate(Composer $composer, IOInterface $io)
212213
{
213-
// store operations in case Flex is being upgraded
214-
self::$storedOperations = $this->operations;
214+
// Using `Flex::` instead of `self::` to avoid issues when
215+
// composer renames plugin classes when upgrading them
216+
Flex::$storedOperations = $this->operations;
215217
self::$activated = false;
216218
}
217219

@@ -707,7 +709,7 @@ private function initOptions(): Options
707709
'runtime' => $extra['runtime'] ?? [],
708710
], $extra);
709711

710-
return new Options($options, $this->io);
712+
return new Options($options, $this->io, $this->lock);
711713
}
712714

713715
private function formatOrigin(Recipe $recipe): string

‎src/Options.php‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,13 @@ class Options
2222
private $options;
2323
private $writtenFiles = [];
2424
private $io;
25+
private $lockData;
2526

26-
public function __construct(array $options = [], ?IOInterface $io = null)
27+
public function __construct(array $options = [], ?IOInterface $io = null, ?Lock $lock = null)
2728
{
2829
$this->options = $options;
2930
$this->io = $io;
31+
$this->lockData = $lock?->all() ?? [];
3032
}
3133

3234
public function get(string $name)
@@ -101,6 +103,38 @@ public function shouldWriteFile(string $file, bool $overwrite, bool $skipQuestio
101103
return $this->io && $this->io->askConfirmation(\sprintf('File "%s" has uncommitted changes, overwrite? [y/N] ', $name), false);
102104
}
103105

106+
public function getRemovableFiles(Recipe $recipe, Lock $lock): array
107+
{
108+
if (null === $removableFiles = $this->lockData[$recipe->getName()]['files'] ?? null) {
109+
$removableFiles = [];
110+
foreach (array_keys($recipe->getFiles()) as $source => $target) {
111+
if (str_ends_with($source, '/')) {
112+
$removableFiles[] = $this->expandTargetDir($target);
113+
}
114+
}
115+
}
116+
117+
unset($this->lockData[$recipe->getName()]);
118+
$lockedFiles = array_count_values(array_merge(...array_column($lock->all(), 'files')));
119+
120+
$nonRemovableFiles = [];
121+
foreach ($removableFiles as $i => $file) {
122+
if (isset($lockedFiles[$file])) {
123+
$nonRemovableFiles[] = $file;
124+
unset($removableFiles[$i]);
125+
}
126+
}
127+
128+
if ($nonRemovableFiles && $this->io) {
129+
$this->io?->writeError(' <warning>The following files are still referenced by other recipes, you might need to adjust them manually:</warning>');
130+
foreach ($nonRemovableFiles as $file) {
131+
$this->io?->writeError(' - '.$file);
132+
}
133+
}
134+
135+
return array_values($removableFiles);
136+
}
137+
104138
public function toArray(): array
105139
{
106140
return $this->options;

‎tests/Configurator/CopyFromRecipeConfiguratorTest.php‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public function testConfigureLocksFiles()
6161
public function testConfigureAndOverwriteFiles()
6262
{
6363
if (!file_exists($this->targetDirectory)) {
64-
mkdir($this->targetDirectory);
64+
@mkdir($this->targetDirectory, 0777, true);
6565
}
6666
file_put_contents($this->targetFile, '-');
6767
$lock = $this->getMockBuilder(Lock::class)->disableOriginalConstructor()->getMock();
@@ -99,17 +99,22 @@ public function testConfigure()
9999
public function testUnconfigureKeepsLockedFiles()
100100
{
101101
if (!file_exists($this->sourceDirectory)) {
102-
mkdir($this->sourceDirectory);
102+
@mkdir($this->sourceDirectory, 0777, true);
103+
}
104+
if (!file_exists($this->targetDirectory)) {
105+
@mkdir($this->targetDirectory, 0777, true);
103106
}
107+
file_put_contents($this->targetFile, '');
104108
file_put_contents($this->sourceFile, '-');
105-
$this->assertFileExists($this->sourceFile);
106109

107110
$lock = new Lock(FLEX_TEST_DIR.'/test.lock');
108-
$lock->set('other-recipe', ['files' => ['./'.$this->targetFileRelativePath]]);
111+
$lock->set('other-recipe', ['files' => [$this->targetFileRelativePath]]);
109112

113+
$this->recipe->method('getName')->willReturn('test-recipe');
110114
$this->createConfigurator()->unconfigure($this->recipe, [$this->targetFileRelativePath], $lock);
111115

112116
$this->assertFileExists($this->sourceFile);
117+
$this->assertFileExists($this->targetFile);
113118
}
114119

115120
public function testUnconfigure()
@@ -118,11 +123,12 @@ public function testUnconfigure()
118123
$this->io->expects($this->at(1))->method('writeError')->with([' Removed <fg=green>"./config/file"</>']);
119124

120125
if (!file_exists($this->targetDirectory)) {
121-
mkdir($this->targetDirectory);
126+
@mkdir($this->targetDirectory, 0777, true);
122127
}
123128
file_put_contents($this->targetFile, '');
124129
$this->assertFileExists($this->targetFile);
125130
$lock = $this->getMockBuilder(Lock::class)->disableOriginalConstructor()->getMock();
131+
$this->recipe->method('getName')->willReturn('test-recipe');
126132
$this->createConfigurator()->unconfigure($this->recipe, [$this->targetFileRelativePath], $lock);
127133
$this->assertFileDoesNotExist($this->targetFile);
128134
}
@@ -270,8 +276,6 @@ public function testUpdateResolveDirectories()
270276

271277
protected function setUp(): void
272278
{
273-
parent::setUp();
274-
275279
$this->sourceDirectory = FLEX_TEST_DIR.'/source';
276280
$this->sourceFileRelativePath = 'source/file';
277281
$this->sourceFile = $this->sourceDirectory.'/file';
@@ -294,14 +298,16 @@ protected function setUp(): void
294298

295299
protected function tearDown(): void
296300
{
297-
parent::tearDown();
298-
299301
$this->cleanUpTargetFiles();
300302
}
301303

302304
private function createConfigurator(): CopyFromRecipeConfigurator
303305
{
304-
return new CopyFromRecipeConfigurator($this->getMockBuilder(Composer::class)->getMock(), $this->io, new Options(['root-dir' => FLEX_TEST_DIR, 'config-dir' => 'config'], $this->io));
306+
$lock = new Lock(FLEX_TEST_DIR.'/test.lock');
307+
$lock->set('test-recipe', ['files' => [$this->targetFileRelativePath]]);
308+
$options = new Options(['root-dir' => FLEX_TEST_DIR, 'config-dir' => 'config'], $this->io, $lock);
309+
310+
return new CopyFromRecipeConfigurator($this->getMockBuilder(Composer::class)->getMock(), $this->io, $options);
305311
}
306312

307313
private function cleanUpTargetFiles()

0 commit comments

Comments
 (0)