From afd6260ef0ac45f88bfc99b971b1c5603d869c49 Mon Sep 17 00:00:00 2001 From: Ryan Weaver Date: Wed, 2 Aug 2023 10:51:37 -0400 Subject: [PATCH] Fixing bug where update changes were made immediately vs reported to RecipeUpdate --- src/Configurator/AddLinesConfigurator.php | 110 ++++++++++-------- .../Configurator/AddLinesConfiguratorTest.php | 28 ++--- 2 files changed, 77 insertions(+), 61 deletions(-) diff --git a/src/Configurator/AddLinesConfigurator.php b/src/Configurator/AddLinesConfigurator.php index 10c22b610..bd9aeb12b 100644 --- a/src/Configurator/AddLinesConfigurator.php +++ b/src/Configurator/AddLinesConfigurator.php @@ -23,64 +23,62 @@ 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 { - $changes = $this->getConfigureFileChanges($recipe, $config); + $this->fileContents = []; + $this->executeConfigure($recipe, $config); - foreach ($changes as $file => $change) { - $this->write(sprintf('[add-lines] Patching file "%s"', $file)); - file_put_contents($file, $change); + 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 { - $changes = $this->getUnconfigureFileChanges($recipe, $config); + $this->fileContents = []; + $this->executeUnconfigure($recipe, $config); - foreach ($changes as $file => $change) { - $this->write(sprintf('[add-lines] Reverting file "%s"', $file)); + 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']); }); - $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()); + // 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 getConfigureFileChanges(Recipe $recipe, $config): array + public function executeConfigure(Recipe $recipe, $config): void { - $changes = []; foreach ($config as $patch) { if (!isset($patch['file'])) { $this->write(sprintf('The "file" key is required for the "add-lines" configurator for recipe "%s". Skipping', $recipe->getName())); @@ -133,15 +131,12 @@ public function getConfigureFileChanges(Recipe $recipe, $config): array $target = isset($patch['target']) ? $patch['target'] : null; $newContents = $this->getPatchedContents($file, $content, $position, $target, $warnIfMissing); - $changes[$file] = $newContents; + $this->fileContents[$file] = $newContents; } - - return $changes; } - public function getUnconfigureFileChanges(Recipe $recipe, $config): array + public function executeUnconfigure(Recipe $recipe, $config): void { - $changes = []; foreach ($config as $patch) { if (!isset($patch['file'])) { $this->write(sprintf('The "file" key is required for the "add-lines" configurator for recipe "%s". Skipping', $recipe->getName())); @@ -165,15 +160,13 @@ public function getUnconfigureFileChanges(Recipe $recipe, $config): array $value = $patch['content']; $newContents = $this->getUnPatchedContents($file, $value); - $changes[$file] = $newContents; + $this->fileContents[$file] = $newContents; } - - return $changes; } private function getPatchedContents(string $file, string $value, string $position, ?string $target, bool $warnIfMissing): string { - $fileContents = file_get_contents($file); + $fileContents = $this->readFile($file); if (false !== strpos($fileContents, $value)) { return $fileContents; // already includes value, skip @@ -219,7 +212,7 @@ private function getPatchedContents(string $file, string $value, string $positio private function getUnPatchedContents(string $file, $value): string { - $fileContents = file_get_contents($file); + $fileContents = $this->readFile($file); if (false === strpos($fileContents, $value)) { return $fileContents; // value already gone! @@ -232,9 +225,8 @@ private function getUnPatchedContents(string $file, $value): string } $position = strpos($fileContents, $value); - $fileContents = substr_replace($fileContents, '', $position, \strlen($value)); - return $fileContents; + return substr_replace($fileContents, '', $position, \strlen($value)); } private function isPackageInstalled($packages): bool @@ -253,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; + } } diff --git a/tests/Configurator/AddLinesConfiguratorTest.php b/tests/Configurator/AddLinesConfiguratorTest.php index 2e9179787..07d2ce07b 100644 --- a/tests/Configurator/AddLinesConfiguratorTest.php +++ b/tests/Configurator/AddLinesConfiguratorTest.php @@ -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([ @@ -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 = << [ - ['assets/app.js' => $appJs], + ['assets/app.js' => $appJsOriginal], [ ['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './bootstrap';"], ], @@ -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';"], ], @@ -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'], ],