Skip to content

Commit

Permalink
bug #988 [AddLines] Fix update process to send changes to RecipeUpdat…
Browse files Browse the repository at this point in the history 
…e 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
  • Loading branch information
@fabpot
fabpot committed Aug 4, 2023
2 parents 1a482f5 + afd6260 commit a2554c7
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 63 deletions.
138 changes: 89 additions & 49 deletions src/Configurator/AddLinesConfigurator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,61 @@ class AddLinesConfigurator extends AbstractConfigurator
self::POSITION_AFTER_TARGET,
];

/**
* Holds file contents for files that have been loaded.
* This allows us to "change" the contents of a file multiple
* times before we actually write it out.
*
* @var string[]
*/
private $fileContents = [];

public function configure(Recipe $recipe, $config, Lock $lock, array $options = []): void
{
$this->fileContents = [];
$this->executeConfigure($recipe, $config);

foreach ($this->fileContents as $file => $contents) {
$this->write(sprintf('[add-lines] Patching file "%s"', $this->relativize($file)));
file_put_contents($file, $contents);
}
}

public function unconfigure(Recipe $recipe, $config, Lock $lock): void
{
$this->fileContents = [];
$this->executeUnconfigure($recipe, $config);

foreach ($this->fileContents as $file => $change) {
$this->write(sprintf('[add-lines] Reverting file "%s"', $this->relativize($file)));
file_put_contents($file, $change);
}
}

public function update(RecipeUpdate $recipeUpdate, array $originalConfig, array $newConfig): void
{
// manually check for "requires", as unconfigure ignores it
$originalConfig = array_filter($originalConfig, function ($item) {
return !isset($item['requires']) || $this->isPackageInstalled($item['requires']);
});

// reset the file content cache
$this->fileContents = [];
$this->executeUnconfigure($recipeUpdate->getOriginalRecipe(), $originalConfig);
$this->executeConfigure($recipeUpdate->getNewRecipe(), $newConfig);
$newFiles = [];
$originalFiles = [];
foreach ($this->fileContents as $file => $contents) {
// set the original file to the current contents
$originalFiles[$this->relativize($file)] = file_get_contents($file);
// and the new file where the old recipe was unconfigured, and the new configured
$newFiles[$this->relativize($file)] = $contents;
}
$recipeUpdate->addOriginalFiles($originalFiles);
$recipeUpdate->addNewFiles($newFiles);
}

public function executeConfigure(Recipe $recipe, $config): void
{
foreach ($config as $patch) {
if (!isset($patch['file'])) {
Expand Down Expand Up @@ -57,8 +111,6 @@ public function configure(Recipe $recipe, $config, Lock $lock, array $options =
continue;
}

$this->write(sprintf('Patching file "%s"', $patch['file']));

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

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

$this->patchFile($file, $content, $position, $target, $warnIfMissing);
$newContents = $this->getPatchedContents($file, $content, $position, $target, $warnIfMissing);
$this->fileContents[$file] = $newContents;
}
}

public function unconfigure(Recipe $recipe, $config, Lock $lock): void
public function executeUnconfigure(Recipe $recipe, $config): void
{
foreach ($config as $patch) {
if (!isset($patch['file'])) {
Expand All @@ -106,51 +159,17 @@ public function unconfigure(Recipe $recipe, $config, Lock $lock): void
}
$value = $patch['content'];

$this->unPatchFile($file, $value);
$newContents = $this->getUnPatchedContents($file, $value);
$this->fileContents[$file] = $newContents;
}
}

public function update(RecipeUpdate $recipeUpdate, array $originalConfig, array $newConfig): void
private function getPatchedContents(string $file, string $value, string $position, ?string $target, bool $warnIfMissing): string
{
$originalConfig = array_filter($originalConfig, function ($item) {
return !isset($item['requires']) || $this->isPackageInstalled($item['requires']);
});
$newConfig = array_filter($newConfig, function ($item) {
return !isset($item['requires']) || $this->isPackageInstalled($item['requires']);
});

$filterDuplicates = function (array $sourceConfig, array $comparisonConfig) {
$filtered = [];
foreach ($sourceConfig as $sourceItem) {
$found = false;
foreach ($comparisonConfig as $comparisonItem) {
if ($sourceItem['file'] === $comparisonItem['file'] && $sourceItem['content'] === $comparisonItem['content']) {
$found = true;
break;
}
}
if (!$found) {
$filtered[] = $sourceItem;
}
}

return $filtered;
};

// remove any config where the file+value is the same before & after
$filteredOriginalConfig = $filterDuplicates($originalConfig, $newConfig);
$filteredNewConfig = $filterDuplicates($newConfig, $originalConfig);

$this->unconfigure($recipeUpdate->getOriginalRecipe(), $filteredOriginalConfig, $recipeUpdate->getLock());
$this->configure($recipeUpdate->getNewRecipe(), $filteredNewConfig, $recipeUpdate->getLock());
}

private function patchFile(string $file, string $value, string $position, ?string $target, bool $warnIfMissing)
{
$fileContents = file_get_contents($file);
$fileContents = $this->readFile($file);

if (false !== strpos($fileContents, $value)) {
return; // already includes value, skip
return $fileContents; // already includes value, skip
}

switch ($position) {
Expand Down Expand Up @@ -188,15 +207,15 @@ private function patchFile(string $file, string $value, string $position, ?strin
break;
}

file_put_contents($file, $fileContents);
return $fileContents;
}

private function unPatchFile(string $file, $value)
private function getUnPatchedContents(string $file, $value): string
{
$fileContents = file_get_contents($file);
$fileContents = $this->readFile($file);

if (false === strpos($fileContents, $value)) {
return; // value already gone!
return $fileContents; // value already gone!
}

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

$position = strpos($fileContents, $value);
$fileContents = substr_replace($fileContents, '', $position, \strlen($value));

file_put_contents($file, $fileContents);
return substr_replace($fileContents, '', $position, \strlen($value));
}

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

return true;
}

private function relativize(string $path): string
{
$rootDir = $this->options->get('root-dir');
if (0 === strpos($path, $rootDir)) {
$path = substr($path, \strlen($rootDir) + 1);
}

return ltrim($path, '/\\');
}

private function readFile(string $file): string
{
if (isset($this->fileContents[$file])) {
return $this->fileContents[$file];
}

$fileContents = file_get_contents($file);
$this->fileContents[$file] = $fileContents;

return $fileContents;
}
}
7 changes: 6 additions & 1 deletion src/PackageJsonSynchronizer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,12 @@ private function updateControllersJsonFile(array $phpPackages)
return;
}

$previousControllersJson = (new JsonFile($controllersJsonPath))->read();
try {
$previousControllersJson = (new JsonFile($controllersJsonPath))->read();
} catch (ParsingException $e) {
// if controllers.json is invalid (possible during a recipe upgrade), we can't update the file
return;
}
$newControllersJson = [
'controllers' => [],
'entrypoints' => $previousControllersJson['entrypoints'],
Expand Down
28 changes: 15 additions & 13 deletions tests/Configurator/AddLinesConfiguratorTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -409,8 +409,8 @@ public function getUnconfigureTests()
*/
public function testUpdate(array $originalFiles, array $originalConfig, array $newConfig, array $expectedFiles)
{
foreach ($originalFiles as $filename => $contents) {
$this->saveFile($filename, $contents);
foreach ($originalFiles as $filename => $originalContents) {
$this->saveFile($filename, $originalContents);
}

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

foreach ($expectedFiles as $filename => $contents) {
$this->assertSame($contents, $this->readFile($filename));
$this->assertCount(\count($expectedFiles), $recipeUpdate->getNewFiles());
foreach ($expectedFiles as $filename => $expectedContents) {
$this->assertSame($this->readFile($filename), $recipeUpdate->getOriginalFiles()[$filename]);
$this->assertSame($expectedContents, $recipeUpdate->getNewFiles()[$filename]);
}
}

public function getUpdateTests()
{
$appJs = <<<EOF
$appJsOriginal = <<<EOF
import * as Turbo from '@hotwired/turbo';
import './bootstrap';
console.log(Turbo);
EOF;

$bootstrapJs = <<<EOF
$bootstrapJsOriginal = <<<EOF
console.log('bootstrap.js');
console.log('on the bottom');
EOF;

yield 'recipe_changes_patch_contents' => [
['assets/app.js' => $appJs],
['assets/app.js' => $appJsOriginal],
[
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './bootstrap';"],
],
Expand All @@ -461,18 +463,18 @@ public function getUpdateTests()
];

yield 'recipe_file_and_value_same_before_and_after' => [
['assets/app.js' => $appJs],
['assets/app.js' => $appJsOriginal],
[
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import * as Turbo from '@hotwired/turbo';"],
],
[
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import * as Turbo from '@hotwired/turbo';"],
],
['assets/app.js' => $appJs],
['assets/app.js' => $appJsOriginal],
];

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

yield 'recipe_changes_but_ignored_because_package_not_installed' => [
['assets/app.js' => $appJs],
['assets/app.js' => $appJsOriginal],
[
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './bootstrap';", 'requires' => 'symfony/not-installed'],
],
[
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './stimulus_bootstrap';", 'requires' => 'symfony/not-installed'],
],
['assets/app.js' => $appJs],
[], // no changes will come back in the RecipePatch
];

yield 'recipe_changes_are_applied_if_required_package_installed' => [
['assets/app.js' => $appJs],
['assets/app.js' => $appJsOriginal],
[
['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './bootstrap';", 'requires' => 'symfony/installed-package'],
],
Expand Down

0 comments on commit a2554c7

Please sign in to comment.