Skip to content

Commit b33301a

Browse files
weaverryanfabpot
weaverryan
authored and
fabpot
committed
[recipes:update] Fixing bug where files failed to delete that were modified previously
1 parent 22c14c5 commit b33301a

File tree

7 files changed

+97
-76
lines changed

7 files changed

+97
-76
lines changed

‎.github/workflows/ci.yaml

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ jobs:
2020
matrix:
2121
include:
2222
- php: '7.1'
23+
composer: 2.2.x
2324
- php: '7.2'
2425
- php: '7.3'
2526
- php: '7.4'
@@ -33,11 +34,11 @@ jobs:
3334
uses: actions/[email protected]
3435

3536
- name: "Install PHP with extensions"
36-
uses: shivammathur/setup-php@2.7.0
37+
uses: shivammathur/setup-php@2.18.0
3738
with:
3839
coverage: "none"
3940
php-version: ${{ matrix.php }}
40-
tools: composer:v2
41+
tools: composer:${{ matrix.composer }}
4142

4243
- name: "Validate composer.json"
4344
run: "composer validate --strict --no-check-lock"

‎src/Configurator/BundlesConfigurator.php

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,20 +44,20 @@ public function unconfigure(Recipe $recipe, $bundles, Lock $lock)
4444

4545
public function update(RecipeUpdate $recipeUpdate, array $originalConfig, array $newConfig): void
4646
{
47-
$originalBundles = $this->configureBundles($originalConfig);
47+
$originalBundles = $this->configureBundles($originalConfig, true);
4848
$recipeUpdate->setOriginalFile(
4949
$this->getLocalConfFile(),
5050
$this->buildContents($originalBundles)
5151
);
5252

53-
$newBundles = $this->configureBundles($newConfig);
53+
$newBundles = $this->configureBundles($newConfig, true);
5454
$recipeUpdate->setNewFile(
5555
$this->getLocalConfFile(),
5656
$this->buildContents($newBundles)
5757
);
5858
}
5959

60-
private function configureBundles(array $bundles): array
60+
private function configureBundles(array $bundles, bool $resetEnvironments = false): array
6161
{
6262
$file = $this->getConfFile();
6363
$registered = $this->load($file);
@@ -70,7 +70,15 @@ private function configureBundles(array $bundles): array
7070
}
7171
foreach ($classes as $class => $envs) {
7272
// do not override existing configured envs for a bundle
73-
if (!isset($registered[$class])) {
73+
if (!isset($registered[$class]) || $resetEnvironments) {
74+
if ($resetEnvironments) {
75+
// used during calculating an "upgrade"
76+
// here, we want to "undo" the bundle's configuration entirely
77+
// then re-add it fresh, in case some environments have been
78+
// removed in an updated version of the recipe
79+
$registered[$class] = [];
80+
}
81+
7482
foreach ($envs as $env) {
7583
$registered[$class][$env] = true;
7684
}

‎src/Update/RecipePatch.php

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,14 @@ class RecipePatch
1515
{
1616
private $patch;
1717
private $blobs;
18+
private $deletedFiles;
1819
private $removedPatches;
1920

20-
public function __construct(string $patch, array $blobs, array $removedPatches = [])
21+
public function __construct(string $patch, array $blobs, array $deletedFiles, array $removedPatches = [])
2122
{
2223
$this->patch = $patch;
2324
$this->blobs = $blobs;
25+
$this->deletedFiles = $deletedFiles;
2426
$this->removedPatches = $removedPatches;
2527
}
2628

@@ -34,6 +36,11 @@ public function getBlobs(): array
3436
return $this->blobs;
3537
}
3638

39+
public function getDeletedFiles(): array
40+
{
41+
return $this->deletedFiles;
42+
}
43+
3744
/**
3845
* Patches for modified files that were removed because the file
3946
* has been deleted in the user's project.

‎src/Update/RecipePatcher.php

Lines changed: 50 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -38,40 +38,15 @@ public function __construct(string $rootDir, IOInterface $io)
3838
*/
3939
public function applyPatch(RecipePatch $patch): bool
4040
{
41-
if (!$patch->getPatch()) {
42-
// nothing to do!
43-
return true;
44-
}
45-
46-
$addedBlobs = $this->addMissingBlobs($patch->getBlobs());
47-
48-
$patchPath = $this->rootDir.'/_flex_recipe_update.patch';
49-
file_put_contents($patchPath, $patch->getPatch());
41+
$withConflicts = $this->_applyPatchFile($patch);
5042

51-
try {
52-
$this->execute('git update-index --refresh', $this->rootDir);
53-
54-
$output = '';
55-
$statusCode = $this->processExecutor->execute('git apply "_flex_recipe_update.patch" -3', $output, $this->rootDir);
56-
57-
if (0 === $statusCode) {
58-
// successful with no conflicts
59-
return true;
60-
}
61-
62-
if (false !== strpos($this->processExecutor->getErrorOutput(), 'with conflicts')) {
63-
// successful with conflicts
64-
return false;
65-
}
66-
67-
throw new \LogicException('Error applying the patch: '.$this->processExecutor->getErrorOutput());
68-
} finally {
69-
unlink($patchPath);
70-
// clean up any temporary blobs
71-
foreach ($addedBlobs as $filename) {
72-
unlink($filename);
43+
foreach ($patch->getDeletedFiles() as $deletedFile) {
44+
if (file_exists($this->rootDir.'/'.$deletedFile)) {
45+
$this->execute(sprintf('git rm %s', ProcessExecutor::escape($deletedFile)), $this->rootDir);
7346
}
7447
}
48+
49+
return $withConflicts;
7550
}
7651

7752
public function generatePatch(array $originalFiles, array $newFiles): RecipePatch
@@ -84,10 +59,13 @@ public function generatePatch(array $originalFiles, array $newFiles): RecipePatc
8459
return null !== $file;
8560
});
8661

87-
// find removed files and add them so they will be deleted
62+
$deletedFiles = [];
63+
// find removed files & record that they are deleted
64+
// unset them from originalFiles to avoid unnecessary blobs being added
8865
foreach ($originalFiles as $file => $contents) {
8966
if (!isset($newFiles[$file])) {
90-
$newFiles[$file] = null;
67+
$deletedFiles[] = $file;
68+
unset($originalFiles[$file]);
9169
}
9270
}
9371

@@ -130,6 +108,7 @@ public function generatePatch(array $originalFiles, array $newFiles): RecipePatc
130108
return new RecipePatch(
131109
$patchString,
132110
$blobs,
111+
$deletedFiles,
133112
$removedPatches
134113
);
135114
} finally {
@@ -223,4 +202,42 @@ private function getBlobPath(string $hash): string
223202

224203
return '.git/objects/'.$hashStart.'/'.$hashEnd;
225204
}
205+
206+
private function _applyPatchFile(RecipePatch $patch)
207+
{
208+
if (!$patch->getPatch()) {
209+
// nothing to do!
210+
return true;
211+
}
212+
213+
$addedBlobs = $this->addMissingBlobs($patch->getBlobs());
214+
215+
$patchPath = $this->rootDir.'/_flex_recipe_update.patch';
216+
file_put_contents($patchPath, $patch->getPatch());
217+
218+
try {
219+
$this->execute('git update-index --refresh', $this->rootDir);
220+
221+
$output = '';
222+
$statusCode = $this->processExecutor->execute('git apply "_flex_recipe_update.patch" -3', $output, $this->rootDir);
223+
224+
if (0 === $statusCode) {
225+
// successful with no conflicts
226+
return true;
227+
}
228+
229+
if (false !== strpos($this->processExecutor->getErrorOutput(), 'with conflicts')) {
230+
// successful with conflicts
231+
return false;
232+
}
233+
234+
throw new \LogicException('Error applying the patch: '.$this->processExecutor->getErrorOutput());
235+
} finally {
236+
unlink($patchPath);
237+
// clean up any temporary blobs
238+
foreach ($addedBlobs as $filename) {
239+
unlink($filename);
240+
}
241+
}
242+
}
226243
}

‎tests/Configurator/BundlesConfiguratorTest.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ public function testUpdate()
176176
177177
return [
178178
BarBundle::class => ['prod' => false, 'all' => true],
179-
FooBundle::class => ['dev' => true, 'test' => true],
179+
FooBundle::class => ['all' => true],
180180
BazBundle::class => ['all' => true],
181181
NewBundle::class => ['all' => true],
182182
];

‎tests/Update/RecipePatchTest.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,14 @@ public function testBasicFunctioning()
2020
{
2121
$thePatch = 'the patch';
2222
$blobs = ['blob1', 'blob2', 'beware of the blob'];
23+
$deletedFiles = ['old_file.txt'];
2324
$removedPatches = ['foo' => 'some diff'];
2425

25-
$patch = new RecipePatch($thePatch, $blobs, $removedPatches);
26+
$patch = new RecipePatch($thePatch, $blobs, $deletedFiles, $removedPatches);
2627

2728
$this->assertSame($thePatch, $patch->getPatch());
2829
$this->assertSame($blobs, $patch->getBlobs());
30+
$this->assertSame($deletedFiles, $patch->getDeletedFiles());
2931
$this->assertSame($removedPatches, $patch->getRemovedPatches());
3032
}
3133
}

‎tests/Update/RecipePatcherTest.php

Lines changed: 20 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ protected function setUp(): void
3131
/**
3232
* @dataProvider getGeneratePatchTests
3333
*/
34-
public function testGeneratePatch(array $originalFiles, array $newFiles, string $expectedPatch)
34+
public function testGeneratePatch(array $originalFiles, array $newFiles, string $expectedPatch, array $expectedDeletedFiles = [])
3535
{
3636
$this->getFilesystem()->remove(FLEX_TEST_DIR);
3737
$this->getFilesystem()->mkdir(FLEX_TEST_DIR);
@@ -44,6 +44,7 @@ public function testGeneratePatch(array $originalFiles, array $newFiles, string
4444

4545
$patch = $patcher->generatePatch($originalFiles, $newFiles);
4646
$this->assertSame($expectedPatch, rtrim($patch->getPatch(), "\n"));
47+
$this->assertSame($expectedDeletedFiles, $patch->getDeletedFiles());
4748

4849
// find all "index 7d30dc7.." in patch
4950
$matches = [];
@@ -121,31 +122,15 @@ public function getGeneratePatchTests(): iterable
121122
yield 'file_deleted_in_update_because_missing' => [
122123
['file1.txt' => 'New file'],
123124
[],
124-
<<<EOF
125-
diff --git a/file1.txt b/file1.txt
126-
deleted file mode 100644
127-
index b78ca63..0000000
128-
--- a/file1.txt
129-
+++ /dev/null
130-
@@ -1 +0,0 @@
131-
-New file
132-
\ No newline at end of file
133-
EOF
125+
'',
126+
['file1.txt'],
134127
];
135128

136129
yield 'file_deleted_in_update_because_null' => [
137130
['file1.txt' => 'New file'],
138131
['file1.txt' => null],
139-
<<<EOF
140-
diff --git a/file1.txt b/file1.txt
141-
deleted file mode 100644
142-
index b78ca63..0000000
143-
--- a/file1.txt
144-
+++ /dev/null
145-
@@ -1 +0,0 @@
146-
-New file
147-
\ No newline at end of file
148-
EOF
132+
'',
133+
['file1.txt'],
149134
];
150135

151136
yield 'mixture_of_added_updated_removed' => [
@@ -169,15 +154,9 @@ public function getGeneratePatchTests(): iterable
169154
@@ -0,0 +1 @@
170155
+file to create
171156
\ No newline at end of file
172-
diff --git a/will_be_deleted.txt b/will_be_deleted.txt
173-
deleted file mode 100644
174-
index 98ff166..0000000
175-
--- a/will_be_deleted.txt
176-
+++ /dev/null
177-
@@ -1 +0,0 @@
178-
-file to delete
179-
\ No newline at end of file
180157
EOF
158+
,
159+
['will_be_deleted.txt'],
181160
];
182161
}
183162

@@ -243,7 +222,8 @@ public function getApplyPatchTests(): iterable
243222
['.env' => $dotEnvClean['in_app']],
244223
new RecipePatch(
245224
$dotEnvClean['patch'],
246-
[$dotEnvClean['hash'] => $dotEnvClean['blob']]
225+
[$dotEnvClean['hash'] => $dotEnvClean['blob']],
226+
[]
247227
),
248228
['.env' => $dotEnvClean['expected']],
249229
false,
@@ -253,7 +233,8 @@ public function getApplyPatchTests(): iterable
253233
['package.json' => $packageJsonConflict['in_app']],
254234
new RecipePatch(
255235
$packageJsonConflict['patch'],
256-
[$packageJsonConflict['hash'] => $packageJsonConflict['blob']]
236+
[$packageJsonConflict['hash'] => $packageJsonConflict['blob']],
237+
[]
257238
),
258239
['package.json' => $packageJsonConflict['expected']],
259240
true,
@@ -265,6 +246,7 @@ public function getApplyPatchTests(): iterable
265246
new RecipePatch(
266247
$webpackEncoreAdded['patch'],
267248
// no blobs needed for a new file
249+
[],
268250
[]
269251
),
270252
['config/packages/webpack_encore.yaml' => $webpackEncoreAdded['expected']],
@@ -274,8 +256,9 @@ public function getApplyPatchTests(): iterable
274256
yield 'removed_one_file' => [
275257
['config/packages/security.yaml' => $securityRemoved['in_app']],
276258
new RecipePatch(
277-
$securityRemoved['patch'],
278-
[$securityRemoved['hash'] => $securityRemoved['blob']]
259+
'',
260+
[$securityRemoved['hash'] => $securityRemoved['blob']],
261+
['config/packages/security.yaml']
279262
),
280263
// expected to be deleted
281264
['config/packages/security.yaml' => null],
@@ -290,12 +273,15 @@ public function getApplyPatchTests(): iterable
290273
'config/packages/security.yaml' => $securityRemoved['in_app'],
291274
],
292275
new RecipePatch(
293-
$dotEnvClean['patch']."\n".$packageJsonConflict['patch']."\n".$webpackEncoreAdded['patch']."\n".$securityRemoved['patch'],
276+
$dotEnvClean['patch']."\n".$packageJsonConflict['patch']."\n".$webpackEncoreAdded['patch'],
294277
[
295278
$dotEnvClean['hash'] => $dotEnvClean['blob'],
296279
$packageJsonConflict['hash'] => $packageJsonConflict['blob'],
297280
$webpackEncoreAdded['hash'] => $webpackEncoreAdded['blob'],
298281
$securityRemoved['hash'] => $securityRemoved['blob'],
282+
],
283+
[
284+
'config/packages/security.yaml',
299285
]
300286
),
301287
[

0 commit comments

Comments
 (0)