Repository: symfony/flex (php)

Issue:
Flex does not install recipes if Flex itself is being upgraded
Hi!

Pretty sure we stumbled on a bug where if (A) `symfony/flex` is being upgraded AND (B) a new package is being installed that has a recipe, the recipe will be ignored.

We're seeing this when upgrading apps using certain UX packages, as a `composer update` grabs the new `symfony/flex` version AND installs `symfony/stimulus-bundle`, which is a new dependency for some UX packages.

Reproducer:

```
git clone git@github.com:weaverryan/flex-recipe-upgrade-bug.git
cd flex-recipe-upgrade-bug
composer update
```

You'll notice that it installs `symfony/stimulus-bundle`, but skips installing its recipe. Running `composer update` a 2nd time causes it to be installed.

## Cause

This appears to be due to Composer being smart. When a plugin is upgraded, it's PluginManager::update() method unloads the old plugin, and loads the new one. The install of StimulusBundle is recorded into a `Flex` instance... but then that `Flex` instance is discarded and a new instance is created that is *not* aware of what packages were just installed.

## Solution

Possibly, in `Flex::deactivate()`, we could... store `$this->operations` onto a static `Flex` property - e.g. `public static $storedOperations`. Then in `Flex::activate()`, we could look for `Symfony\Flex\Flex::$storedOperations` and use that (after the update, Composer uses a class called `Symfony\Flex\Flex_composer_tmp0`... so I'm assuming that `Symfony\Flex\Flex` would still be available, and we could look directly for it.

Cheers!

Pull request:
[Bug] Fix Flex not installing recipes while it is being upgraded
Fixes #977

Here's the lifecycle:

A) Packages are installed/updated and `Flex::recordOperations()` is called.
B) Composer sees that `symfony/flex` is updated and unloads the plugin and reloads it under a new `Symfony\Flex\Flex_composer_tmp0` class. This results in the `Flex::$operations` property being lost.
C) And so, no recipes are installed.

This PR "stashes" the operations directly on `Symfony/Flex/Flex` where we then read them later. Tested locally. Amazing, it seems to work perfectly.

Also, backports #976 to 1.x - my fault for making the PR on the wrong branch.

Cheers!

Patch:
diff --git a/src/Configurator/AddLinesConfigurator.php b/src/Configurator/AddLinesConfigurator.php
index bd9aeb1..c779474 100644
--- a/src/Configurator/AddLinesConfigurator.php
+++ b/src/Configurator/AddLinesConfigurator.php
@@ -23,61 +23,7 @@ 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'])) {
@@ -111,6 +57,8 @@ class AddLinesConfigurator extends AbstractConfigurator
                 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()));
 
@@ -130,12 +78,11 @@ class AddLinesConfigurator extends AbstractConfigurator
             }
             $target = isset($patch['target']) ? $patch['target'] : null;
 
-            $newContents = $this->getPatchedContents($file, $content, $position, $target, $warnIfMissing);
-            $this->fileContents[$file] = $newContents;
+            $this->patchFile($file, $content, $position, $target, $warnIfMissing);
         }
     }
 
-    public function executeUnconfigure(Recipe $recipe, $config): void
+    public function unconfigure(Recipe $recipe, $config, Lock $lock): void
     {
         foreach ($config as $patch) {
             if (!isset($patch['file'])) {
@@ -159,17 +106,51 @@ class AddLinesConfigurator extends AbstractConfigurator
             }
             $value = $patch['content'];
 
-            $newContents = $this->getUnPatchedContents($file, $value);
-            $this->fileContents[$file] = $newContents;
+            $this->unPatchFile($file, $value);
         }
     }
 
-    private function getPatchedContents(string $file, string $value, string $position, ?string $target, bool $warnIfMissing): string
+    public function update(RecipeUpdate $recipeUpdate, array $originalConfig, array $newConfig): void
     {
-        $fileContents = $this->readFile($file);
+        $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);
 
         if (false !== strpos($fileContents, $value)) {
-            return $fileContents; // already includes value, skip
+            return; // already includes value, skip
         }
 
         switch ($position) {
@@ -207,15 +188,15 @@ class AddLinesConfigurator extends AbstractConfigurator
                 break;
         }
 
-        return $fileContents;
+        file_put_contents($file, $fileContents);
     }
 
-    private function getUnPatchedContents(string $file, $value): string
+    private function unPatchFile(string $file, $value)
     {
-        $fileContents = $this->readFile($file);
+        $fileContents = file_get_contents($file);
 
         if (false === strpos($fileContents, $value)) {
-            return $fileContents; // value already gone!
+            return; // value already gone!
         }
 
         if (false !== strpos($fileContents, "\n".$value)) {
@@ -225,8 +206,9 @@ class AddLinesConfigurator extends AbstractConfigurator
         }
 
         $position = strpos($fileContents, $value);
+        $fileContents = substr_replace($fileContents, '', $position, \strlen($value));
 
-        return substr_replace($fileContents, '', $position, \strlen($value));
+        file_put_contents($file, $fileContents);
     }
 
     private function isPackageInstalled($packages): bool
@@ -245,26 +227,4 @@ class AddLinesConfigurator extends AbstractConfigurator
 
         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/src/Configurator/DockerComposeConfigurator.php b/src/Configurator/DockerComposeConfigurator.php
index debe924..4bc2f2c 100644
--- a/src/Configurator/DockerComposeConfigurator.php
+++ b/src/Configurator/DockerComposeConfigurator.php
@@ -332,11 +332,7 @@ class DockerComposeConfigurator extends AbstractConfigurator
 
         $updatedContents = [];
         foreach ($files as $file) {
-            $localPath = $file;
-            if (0 === strpos($file, $rootDir)) {
-                $localPath = substr($file, \strlen($rootDir) + 1);
-            }
-            $localPath = ltrim($localPath, '/\\');
+            $localPath = ltrim(str_replace($rootDir, '', $file), '/\\');
             $updatedContents[$localPath] = file_exists($file) ? file_get_contents($file) : null;
         }
 
diff --git a/src/Flex.php b/src/Flex.php
index de4ee85..e437e8e 100644
--- a/src/Flex.php
+++ b/src/Flex.php
@@ -60,6 +60,8 @@ use Symfony\Thanks\Thanks;
  */
 class Flex implements PluginInterface, EventSubscriberInterface
 {
+    public static $storedOperations = [];
+
     /**
      * @var Composer
      */
@@ -104,9 +106,6 @@ class Flex implements PluginInterface, EventSubscriberInterface
     ];
     private $filter;
 
-    /**
-     * @return void
-     */
     public function activate(Composer $composer, IOInterface $io)
     {
         if (!\extension_loaded('openssl')) {
@@ -129,6 +128,13 @@ class Flex implements PluginInterface, EventSubscriberInterface
         $this->config = $composer->getConfig();
         $this->options = $this->initOptions();
 
+        // if Flex is being upgraded, the original operations from the original Flex
+        // instance are stored in the static property, so we can reuse them now.
+        if (property_exists(self::class, 'storedOperations') && self::$storedOperations) {
+            $this->operations = self::$storedOperations;
+            self::$storedOperations = [];
+        }
+
         $symfonyRequire = preg_replace('/\.x$/', '.x-dev', getenv('SYMFONY_REQUIRE') ?: ($composer->getPackage()->getExtra()['symfony']['require'] ?? ''));
 
         if ($composer2 = version_compare('2.0.0', PluginInterface::PLUGIN_API_VERSION, '<=')) {
@@ -294,11 +300,10 @@ class Flex implements PluginInterface, EventSubscriberInterface
         }
     }
 
-    /**
-     * @return void
-     */
     public function deactivate(Composer $composer, IOInterface $io)
     {
+        // store operations in case Flex is being upgraded
+        self::$storedOperations = $this->operations;
         self::$activated = false;
     }
 
@@ -550,10 +555,12 @@ class Flex implements PluginInterface, EventSubscriberInterface
             }
         }
 
-        foreach ($postInstallRecipes as $recipe) {
-            $this->configurator->postInstall($recipe, $this->lock, [
-                'force' => $event instanceof UpdateEvent && $event->force(),
-            ]);
+        if (method_exists($this->configurator, 'postInstall')) {
+            foreach ($postInstallRecipes as $recipe) {
+                $this->configurator->postInstall($recipe, $this->lock, [
+                    'force' => $event instanceof UpdateEvent && $event->force(),
+                ]);
+            }
         }
 
         if (null !== $manifest) {
@@ -600,9 +607,6 @@ class Flex implements PluginInterface, EventSubscriberInterface
         }
     }
 
-    /**
-     * @return void
-     */
     public function uninstall(Composer $composer, IOInterface $io)
     {
         $this->lock->delete();
diff --git a/src/InformationOperation.php b/src/InformationOperation.php
index 8cad6a8..288dcc6 100644
--- a/src/InformationOperation.php
+++ b/src/InformationOperation.php
@@ -57,8 +57,6 @@ class InformationOperation implements OperationInterface
 
     /**
      * {@inheritdoc}
-     *
-     * @return string
      */
     public function getOperationType()
     {
@@ -67,8 +65,6 @@ class InformationOperation implements OperationInterface
 
     /**
      * {@inheritdoc}
-     *
-     * @return string
      */
     public function show($lock)
     {
diff --git a/src/PackageJsonSynchronizer.php b/src/PackageJsonSynchronizer.php
index 3bf1fc2..35f1816 100644
--- a/src/PackageJsonSynchronizer.php
+++ b/src/PackageJsonSynchronizer.php
@@ -296,12 +296,7 @@ class PackageJsonSynchronizer
             return;
         }
 
-        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;
-        }
+        $previousControllersJson = (new JsonFile($controllersJsonPath))->read();
         $newControllersJson = [
             'controllers' => [],
             'entrypoints' => $previousControllersJson['entrypoints'],
diff --git a/src/Response.php b/src/Response.php
index f334c03..6384562 100644
--- a/src/Response.php
+++ b/src/Response.php
@@ -65,9 +65,6 @@ class Response implements \JsonSerializable
         return $response;
     }
 
-    /**
-     * @return mixed
-     */
     #[\ReturnTypeWillChange]
     public function jsonSerialize()
     {


Test patch:
diff --git a/tests/Configurator/AddLinesConfiguratorTest.php b/tests/Configurator/AddLinesConfiguratorTest.php
index 07d2ce0..2e91797 100644
--- a/tests/Configurator/AddLinesConfiguratorTest.php
+++ b/tests/Configurator/AddLinesConfiguratorTest.php
@@ -409,8 +409,8 @@ EOF
      */
     public function testUpdate(array $originalFiles, array $originalConfig, array $newConfig, array $expectedFiles)
     {
-        foreach ($originalFiles as $filename => $originalContents) {
-            $this->saveFile($filename, $originalContents);
+        foreach ($originalFiles as $filename => $contents) {
+            $this->saveFile($filename, $contents);
         }
 
         $composer = $this->createComposerMockWithPackagesInstalled([
@@ -423,30 +423,28 @@ EOF
         $recipeUpdate = new RecipeUpdate($recipe, $recipe, $lock, FLEX_TEST_DIR);
         $configurator->update($recipeUpdate, $originalConfig, $newConfig);
 
-        $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]);
+        foreach ($expectedFiles as $filename => $contents) {
+            $this->assertSame($contents, $this->readFile($filename));
         }
     }
 
     public function getUpdateTests()
     {
-        $appJsOriginal = <<<EOF
+        $appJs = <<<EOF
 import * as Turbo from '@hotwired/turbo';
 import './bootstrap';
 
 console.log(Turbo);
 EOF;
 
-        $bootstrapJsOriginal = <<<EOF
+        $bootstrapJs = <<<EOF
 console.log('bootstrap.js');
 
 console.log('on the bottom');
 EOF;
 
         yield 'recipe_changes_patch_contents' => [
-            ['assets/app.js' => $appJsOriginal],
+            ['assets/app.js' => $appJs],
             [
                 ['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './bootstrap';"],
             ],
@@ -463,18 +461,18 @@ EOF
         ];
 
         yield 'recipe_file_and_value_same_before_and_after' => [
-            ['assets/app.js' => $appJsOriginal],
+            ['assets/app.js' => $appJs],
             [
                 ['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' => $appJsOriginal],
+            ['assets/app.js' => $appJs],
         ];
 
         yield 'different_files_unconfigures_old_and_configures_new' => [
-            ['assets/app.js' => $appJsOriginal, 'assets/bootstrap.js' => $bootstrapJsOriginal],
+            ['assets/app.js' => $appJs, 'assets/bootstrap.js' => $bootstrapJs],
             [
                 ['file' => 'assets/app.js', 'position' => 'top', 'content' => "import * as Turbo from '@hotwired/turbo';"],
             ],
@@ -498,18 +496,18 @@ EOF
         ];
 
         yield 'recipe_changes_but_ignored_because_package_not_installed' => [
-            ['assets/app.js' => $appJsOriginal],
+            ['assets/app.js' => $appJs],
             [
                 ['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'],
             ],
-            [], // no changes will come back in the RecipePatch
+            ['assets/app.js' => $appJs],
         ];
 
         yield 'recipe_changes_are_applied_if_required_package_installed' => [
-            ['assets/app.js' => $appJsOriginal],
+            ['assets/app.js' => $appJs],
             [
                 ['file' => 'assets/app.js', 'position' => 'top', 'content' => "import './bootstrap';", 'requires' => 'symfony/installed-package'],
             ],