Skip to content

Commit a2554c7

Browse files
fabpotfabpot
committed
bug #988 [AddLines] Fix update process to send changes to RecipeUpdate vs make them immediately (weaverryan)
This PR was squashed before being merged into the 1.x branch. Discussion ---------- [AddLines] Fix update process to send changes to RecipeUpdate vs make them immediately Hi! This was an oversight when I create `AddLinesConfigurator`. During `recipes:update`, the purpose of the `update()` method in each configurator is to record what the "original" contents of a file would look like from the original recipe & what the "new" contents of a file would look like with the new recipe. Then, LATER, the patcher system creates a diff from these. The `AddLinesConfigurator` was going rogue and updating the files immediately, which caused the "patcher" system later to explode because this file was unexpectedly already modified. The changes are less drastic than they look, as code needed to be moved from some public methods -> private methods. Cheers! Commits ------- afd6260 Fixing bug where update changes were made immediately vs reported to RecipeUpdate 7e8967a Avoid exploding on conflict of controllers.json 6592349 Internal: moving most of method to a private method, no changes 436bad9 Moving guts of method to a private method - no real change 94a3e56 Internal: changing private functions to return a string
2 parents 1a482f5 + afd6260 commit a2554c7

File tree

3 files changed

+110
-63
lines changed

3 files changed

+110
-63
lines changed

‎src/Configurator/AddLinesConfigurator.php

Lines changed: 89 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,61 @@ class AddLinesConfigurator extends AbstractConfigurator
2323
self::POSITION_AFTER_TARGET,
2424
];
2525

26+
/**
27+
* Holds file contents for files that have been loaded.
28+
* This allows us to "change" the contents of a file multiple
29+
* times before we actually write it out.
30+
*
31+
* @var string[]
32+
*/
33+
private $fileContents = [];
34+
2635
public function configure(Recipe $recipe, $config, Lock $lock, array $options = []): void
36+
{
37+
$this->fileContents = [];
38+
$this->executeConfigure($recipe, $config);
39+
40+
foreach ($this->fileContents as $file => $contents) {
41+
$this->write(sprintf('[add-lines] Patching file "%s"', $this->relativize($file)));
42+
file_put_contents($file, $contents);
43+
}
44+
}
45+
46+
public function unconfigure(Recipe $recipe, $config, Lock $lock): void
47+
{
48+
$this->fileContents = [];
49+
$this->executeUnconfigure($recipe, $config);
50+
51+
foreach ($this->fileContents as $file => $change) {
52+
$this->write(sprintf('[add-lines] Reverting file "%s"', $this->relativize($file)));
53+
file_put_contents($file, $change);
54+
}
55+
}
56+
57+
public function update(RecipeUpdate $recipeUpdate, array $originalConfig, array $newConfig): void
58+
{
59+
// manually check for "requires", as unconfigure ignores it
60+
$originalConfig = array_filter($originalConfig, function ($item) {
61+
return !isset($item['requires']) || $this->isPackageInstalled($item['requires']);
62+
});
63+
64+
// reset the file content cache
65+
$this->fileContents = [];
66+
$this->executeUnconfigure($recipeUpdate->getOriginalRecipe(), $originalConfig);
67+
$this->executeConfigure($recipeUpdate->getNewRecipe(), $newConfig);
68+
$newFiles = [];
69+
$originalFiles = [];
70+
foreach ($this->fileContents as $file => $contents) {
71+
// set the original file to the current contents
72+
$originalFiles[$this->relativize($file)] = file_get_contents($file);
73+
// and the new file where the old recipe was unconfigured, and the new configured
74+
$newFiles[$this->relativize($file)] = $contents;
75+
}
76+
$recipeUpdate->addOriginalFiles($originalFiles);
77+
$recipeUpdate->addNewFiles($newFiles);
78+
}
79+
80+
public function executeConfigure(Recipe $recipe, $config): void
2781
{
2882
foreach ($config as $patch) {
2983
if (!isset($patch['file'])) {
@@ -57,8 +111,6 @@ public function configure(Recipe $recipe, $config, Lock $lock, array $options =
57111
continue;
58112
}
59113

60-
$this->write(sprintf('Patching file "%s"', $patch['file']));
61-
62114
if (!isset($patch['position'])) {
63115
$this->write(sprintf('The "position" key is required for the "add-lines" configurator for recipe "%s". Skipping', $recipe->getName()));
64116

@@ -78,11 +130,12 @@ public function configure(Recipe $recipe, $config, Lock $lock, array $options =
78130
}
79131
$target = isset($patch['target']) ? $patch['target'] : null;
80132

81-
$this->patchFile($file, $content, $position, $target, $warnIfMissing);
133+
$newContents = $this->getPatchedContents($file, $content, $position, $target, $warnIfMissing);
134+
$this->fileContents[$file] = $newContents;
82135
}
83136
}
84137

85-
public function unconfigure(Recipe $recipe, $config, Lock $lock): void
138+
public function executeUnconfigure(Recipe $recipe, $config): void
86139
{
87140
foreach ($config as $patch) {
88141
if (!isset($patch['file'])) {
@@ -106,51 +159,17 @@ public function unconfigure(Recipe $recipe, $config, Lock $lock): void
106159
}
107160
$value = $patch['content'];
108161

109-
$this->unPatchFile($file, $value);
162+
$newContents = $this->getUnPatchedContents($file, $value);
163+
$this->fileContents[$file] = $newContents;
110164
}
111165
}
112166

113-
public function update(RecipeUpdate $recipeUpdate, array $originalConfig, array $newConfig): void
167+
private function getPatchedContents(string $file, string $value, string $position, ?string $target, bool $warnIfMissing): string
114168
{
115-
$originalConfig = array_filter($originalConfig, function ($item) {
116-
return !isset($item['requires']) || $this->isPackageInstalled($item['requires']);
117-
});
118-
$newConfig = array_filter($newConfig, function ($item) {
119-
return !isset($item['requires']) || $this->isPackageInstalled($item['requires']);
120-
});
121-
122-
$filterDuplicates = function (array $sourceConfig, array $comparisonConfig) {
123-
$filtered = [];
124-
foreach ($sourceConfig as $sourceItem) {
125-
$found = false;
126-
foreach ($comparisonConfig as $comparisonItem) {
127-
if ($sourceItem['file'] === $comparisonItem['file'] && $sourceItem['content'] === $comparisonItem['content']) {
128-
$found = true;
129-
break;
130-
}
131-
}
132-
if (!$found) {
133-
$filtered[] = $sourceItem;
134-
}
135-
}
136-
137-
return $filtered;
138-
};
139-
140-
// remove any config where the file+value is the same before & after
141-
$filteredOriginalConfig = $filterDuplicates($originalConfig, $newConfig);
142-
$filteredNewConfig = $filterDuplicates($newConfig, $originalConfig);
143-
144-
$this->unconfigure($recipeUpdate->getOriginalRecipe(), $filteredOriginalConfig, $recipeUpdate->getLock());
145-
$this->configure($recipeUpdate->getNewRecipe(), $filteredNewConfig, $recipeUpdate->getLock());
146-
}
147-
148-
private function patchFile(string $file, string $value, string $position, ?string $target, bool $warnIfMissing)
149-
{
150-
$fileContents = file_get_contents($file);
169+
$fileContents = $this->readFile($file);
151170

152171
if (false !== strpos($fileContents, $value)) {
153-
return; // already includes value, skip
172+
return $fileContents; // already includes value, skip
154173
}
155174

156175
switch ($position) {
@@ -188,15 +207,15 @@ private function patchFile(string $file, string $value, string $position, ?strin
188207
break;
189208
}
190209

191-
file_put_contents($file, $fileContents);
210+
return $fileContents;
192211
}
193212

194-
private function unPatchFile(string $file, $value)
213+
private function getUnPatchedContents(string $file, $value): string
195214
{
196-
$fileContents = file_get_contents($file);
215+
$fileContents = $this->readFile($file);
197216

198217
if (false === strpos($fileContents, $value)) {
199-
return; // value already gone!
218+
return $fileContents; // value already gone!
200219
}
201220

202221
if (false !== strpos($fileContents, "\n".$value)) {
@@ -206,9 +225,8 @@ private function unPatchFile(string $file, $value)
206225
}
207226

208227
$position = strpos($fileContents, $value);
209-
$fileContents = substr_replace($fileContents, '', $position, \strlen($value));
210228

211-
file_put_contents($file, $fileContents);
229+
return substr_replace($fileContents, '', $position, \strlen($value));
212230
}
213231

214232
private function isPackageInstalled($packages): bool
@@ -227,4 +245,26 @@ private function isPackageInstalled($packages): bool
227245

228246
return true;
229247
}
248+
249+
private function relativize(string $path): string
250+
{
251+
$rootDir = $this->options->get('root-dir');
252+
if (0 === strpos($path, $rootDir)) {
253+
$path = substr($path, \strlen($rootDir) + 1);
254+
}
255+
256+
return ltrim($path, '/\\');
257+
}
258+
259+
private function readFile(string $file): string
260+
{
261+
if (isset($this->fileContents[$file])) {
262+
return $this->fileContents[$file];
263+
}
264+
265+
$fileContents = file_get_contents($file);
266+
$this->fileContents[$file] = $fileContents;
267+
268+
return $fileContents;
269+
}
230270
}

‎src/PackageJsonSynchronizer.php

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,12 @@ private function updateControllersJsonFile(array $phpPackages)
296296
return;
297297
}
298298

299-
$previousControllersJson = (new JsonFile($controllersJsonPath))->read();
299+
try {
300+
$previousControllersJson = (new JsonFile($controllersJsonPath))->read();
301+
} catch (ParsingException $e) {
302+
// if controllers.json is invalid (possible during a recipe upgrade), we can't update the file
303+
return;
304+
}
300305
$newControllersJson = [
301306
'controllers' => [],
302307
'entrypoints' => $previousControllersJson['entrypoints'],

‎tests/Configurator/AddLinesConfiguratorTest.php

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -409,8 +409,8 @@ public function getUnconfigureTests()
409409
*/
410410
public function testUpdate(array $originalFiles, array $originalConfig, array $newConfig, array $expectedFiles)
411411
{
412-
foreach ($originalFiles as $filename => $contents) {
413-
$this->saveFile($filename, $contents);
412+
foreach ($originalFiles as $filename => $originalContents) {
413+
$this->saveFile($filename, $originalContents);
414414
}
415415

416416
$composer = $this->createComposerMockWithPackagesInstalled([
@@ -423,28 +423,30 @@ public function testUpdate(array $originalFiles, array $originalConfig, array $n
423423
$recipeUpdate = new RecipeUpdate($recipe, $recipe, $lock, FLEX_TEST_DIR);
424424
$configurator->update($recipeUpdate, $originalConfig, $newConfig);
425425

426-
foreach ($expectedFiles as $filename => $contents) {
427-
$this->assertSame($contents, $this->readFile($filename));
426+
$this->assertCount(\count($expectedFiles), $recipeUpdate->getNewFiles());
427+
foreach ($expectedFiles as $filename => $expectedContents) {
428+
$this->assertSame($this->readFile($filename), $recipeUpdate->getOriginalFiles()[$filename]);
429+
$this->assertSame($expectedContents, $recipeUpdate->getNewFiles()[$filename]);
428430
}
429431
}
430432

431433
public function getUpdateTests()
432434
{
433-
$appJs = <<<EOF
435+
$appJsOriginal = <<<EOF
434436
import * as Turbo from '@hotwired/turbo';
435437
import './bootstrap';
436438
437439
console.log(Turbo);
438440
EOF;
439441

440-
$bootstrapJs = <<<EOF
442+
$bootstrapJsOriginal = <<<EOF
441443
console.log('bootstrap.js');
442444
443445
console.log('on the bottom');
444446
EOF;
445447

446448
yield 'recipe_changes_patch_contents' => [
447-
['assets/app.js' => $appJs],
449+
['assets/app.js' => $appJsOriginal],
448450
[
449451
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './bootstrap';"],
450452
],
@@ -461,18 +463,18 @@ public function getUpdateTests()
461463
];
462464

463465
yield 'recipe_file_and_value_same_before_and_after' => [
464-
['assets/app.js' => $appJs],
466+
['assets/app.js' => $appJsOriginal],
465467
[
466468
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import * as Turbo from '@hotwired/turbo';"],
467469
],
468470
[
469471
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import * as Turbo from '@hotwired/turbo';"],
470472
],
471-
['assets/app.js' => $appJs],
473+
['assets/app.js' => $appJsOriginal],
472474
];
473475

474476
yield 'different_files_unconfigures_old_and_configures_new' => [
475-
['assets/app.js' => $appJs, 'assets/bootstrap.js' => $bootstrapJs],
477+
['assets/app.js' => $appJsOriginal, 'assets/bootstrap.js' => $bootstrapJsOriginal],
476478
[
477479
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import * as Turbo from '@hotwired/turbo';"],
478480
],
@@ -496,18 +498,18 @@ public function getUpdateTests()
496498
];
497499

498500
yield 'recipe_changes_but_ignored_because_package_not_installed' => [
499-
['assets/app.js' => $appJs],
501+
['assets/app.js' => $appJsOriginal],
500502
[
501503
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './bootstrap';", 'requires' => 'symfony/not-installed'],
502504
],
503505
[
504506
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './stimulus_bootstrap';", 'requires' => 'symfony/not-installed'],
505507
],
506-
['assets/app.js' => $appJs],
508+
[], // no changes will come back in the RecipePatch
507509
];
508510

509511
yield 'recipe_changes_are_applied_if_required_package_installed' => [
510-
['assets/app.js' => $appJs],
512+
['assets/app.js' => $appJsOriginal],
511513
[
512514
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './bootstrap';", 'requires' => 'symfony/installed-package'],
513515
],

0 commit comments

Comments
 (0)