Skip to content

Commit 2c857c0

Browse files
fabpotfabpot
committed
bug #895 [recipes:update] Fixing bug where files failed to delete that 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
2 parents 22c14c5 + b33301a commit 2c857c0

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)