Merge pull request #11449 from Toflar/fix/poolbuilder-replace-should-…

…be-optional Optimize PoolBuilder to not load replaced targets if not required
composer · May 2, 2023 · 2928b87 · 2928b87
2 parents 9555ae1 + 9ced20f
commit 2928b87
Show file tree

Hide file tree

Showing 5 changed files with 142 additions and 4 deletions.
diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php
@@ -94,6 +94,11 @@ class PoolBuilder
      * @phpstan-var array<int, array<string, array<string, PackageInterface>>>
      */
     private $loadedPerRepo = [];
+    /**
+     * @var array[]
+     * @phpstan-var array<string, bool>
+     */
+    private $optionalPackages = [];
     /**
      * @var BasePackage[]
      */
@@ -235,8 +240,9 @@ public function buildPool(array $repositories, Request $request): Pool
             }
         }
 
-        while (!empty($this->packagesToLoad)) {
+        while ([] !== $this->packagesToLoad || [] !== $this->optionalPackages) {
             $this->loadPackagesMarkedForLoading($request, $repositories);
+            $this->loadOptionalPackages($request);
         }
 
         if (\count($this->temporaryConstraints) > 0) {
@@ -483,7 +489,8 @@ private function loadPackage(Request $request, array $repositories, BasePackage
 
                     if ($request->getUpdateAllowTransitiveRootDependencies() || !$skippedRootRequires) {
                         $this->unlockPackage($request, $repositories, $replace);
-                        $this->markPackageNameForLoading($request, $replace, $link->getConstraint());
+                        // Mark as optional - if no other package requires it, we don't need to load it
+                        $this->markPackageNameForOptionalLoading($replace);
                     } else {
                         foreach ($skippedRootRequires as $rootRequire) {
                             if (!isset($this->updateAllowWarned[$rootRequire])) {
@@ -639,6 +646,8 @@ private function unlockPackage(Request $request, array $repositories, string $na
                     // make sure that any requirements for this package by other locked or fixed packages are now
                     // also loaded, as they were previously ignored because the locked (now unlocked) package already
                     // satisfied their requirements
+                    // and if this package is replacing another that is required by a locked or fixed package, ensure
+                    // that we load that replaced package in case an update to this package removes the replacement
                     foreach ($request->getFixedOrLockedPackages() as $fixedOrLockedPackage) {
                         if ($fixedOrLockedPackage === $lockedPackage) {
                             continue;
@@ -649,13 +658,45 @@ private function unlockPackage(Request $request, array $repositories, string $na
                             if (isset($requires[$lockedPackage->getName()])) {
                                 $this->markPackageNameForLoading($request, $lockedPackage->getName(), $requires[$lockedPackage->getName()]->getConstraint());
                             }
+
+                            foreach ($lockedPackage->getReplaces() as $replace) {
+                                if (isset($requires[$replace->getTarget()], $this->skippedLoad[$replace->getTarget()])) {
+                                    $this->unlockPackage($request, $repositories, $replace->getTarget());
+                                    // Do not call markPackageNameForOptionalLoading() here, we know that $lockedPackage is already
+                                    // part of $this->packages, and we check for $requires[$replace->getTarget()] so we're guaranteed
+                                    // to require this package.
+                                    $this->markPackageNameForLoading($request, $replace->getTarget(), $replace->getConstraint());
+                                }
+                            }
                         }
                     }
                 }
             }
         }
     }
 
+    private function markPackageNameForOptionalLoading(string $name): void
+    {
+        $this->optionalPackages[$name] = true;
+    }
+
+    private function loadOptionalPackages(Request $request): void
+    {
+        if ([] === $this->optionalPackages) {
+            return;
+        }
+
+        foreach ($this->packages as $package) {
+            foreach ($package->getRequires() as $link) {
+                if (isset($this->optionalPackages[$link->getTarget()])) {
+                    $this->markPackageNameForLoading($request, $link->getTarget(), $link->getConstraint());
+                }
+            }
+        }
+
+        $this->optionalPackages = [];
+    }
+
     /**
      * @param RepositoryInterface[] $repositories
      */

diff --git a/...st/DependencyResolver/Fixtures/poolbuilder/load-replaced-package-if-replacer-dropped.test b/...st/DependencyResolver/Fixtures/poolbuilder/load-replaced-package-if-replacer-dropped.test
@@ -0,0 +1,50 @@
+--TEST--
+Ensure that a package gets loaded which was previously skipped due to replacement
+
+--REQUEST--
+{
+    "require": {
+        "root/dep": "*",
+        "root/no-update": "*"
+    },
+    "locked": [
+        {"name": "root/dep", "version": "1.1.0", "require": {"replacer/pkg": "1.*"}},
+        {"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}},
+        {"name": "root/no-update", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}
+    ],
+    "allowList": [
+        "root/dep"
+    ],
+    "allowTransitiveDepsNoRootRequire": true
+}
+
+--FIXED--
+[
+]
+
+--PACKAGE-REPOS--
+[
+    [
+        {"name": "root/dep", "version": "1.2.0", "require": {"replacer/pkg": ">=1.1.0"}},
+        {"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}},
+        {"name": "replacer/pkg", "version": "1.1.0"},
+        {"name": "replaced/pkg", "version": "1.0.0"},
+        {"name": "root/no-update", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}
+    ]
+]
+
+--EXPECT--
+[
+    "root/no-update-1.0.0.0 (locked)",
+    "root/dep-1.2.0.0",
+    "replaced/pkg-1.0.0.0",
+    "replacer/pkg-1.1.0.0"
+]
+
+--EXPECT-OPTIMIZED--
+[
+    "root/no-update-1.0.0.0 (locked)",
+    "root/dep-1.2.0.0",
+    "replaced/pkg-1.0.0.0",
+    "replacer/pkg-1.1.0.0"
+]
diff --git a/...r/Test/DependencyResolver/Fixtures/poolbuilder/multi-repo-replace-partial-update-all.test b/...r/Test/DependencyResolver/Fixtures/poolbuilder/multi-repo-replace-partial-update-all.test
@@ -101,7 +101,6 @@ Check that replacers from additional repositories are loaded when doing a partia
     "indirect/replacer-1.0.0.0",
     "replacer/package-1.2.0.0",
     "replacer/package-1.0.0.0",
-    "base/package-1.0.0.0",
     "shared/dep-1.0.0.0",
     "shared/dep-1.2.0.0"
 ]
@@ -112,6 +111,5 @@ Check that replacers from additional repositories are loaded when doing a partia
     "indirect/replacer-1.0.0.0",
     "replacer/package-1.2.0.0",
     "replacer/package-1.0.0.0",
-    "base/package-1.0.0.0",
     "shared/dep-1.2.0.0"
 ]
diff --git a/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php b/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php
@@ -137,10 +137,14 @@ public function testPoolBuilder(string $file, string $message, array $expect, ar
 
         $result = $this->getPackageResultSet($pool, $packageIds);
 
+        sort($expect);
+        sort($result);
         $this->assertSame($expect, $result, 'Unoptimized pool does not match expected package set');
 
         $optimizer = new PoolOptimizer(new DefaultPolicy());
         $result = $this->getPackageResultSet($optimizer->optimize($request, $pool), $packageIds);
+        sort($expectOptimized);
+        sort($result);
         $this->assertSame($expectOptimized, $result, 'Optimized pool does not match expected package set');
 
         chdir($oldCwd);

diff --git a/tests/Composer/Test/Fixtures/installer/load-replaced-package-if-replacer-dropped.test b/tests/Composer/Test/Fixtures/installer/load-replaced-package-if-replacer-dropped.test
@@ -0,0 +1,45 @@
+--TEST--
+Ensure that a package gets loaded which was previously skipped due to replacement
+--COMPOSER--
+{
+    "repositories": [
+        {
+            "type": "package",
+            "package": [
+                {"name": "root/dep", "version": "1.2.0", "require": {"replacer/pkg": ">=1.1.0"}},
+                {"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}},
+                {"name": "replacer/pkg", "version": "1.1.0"},
+                {"name": "replaced/pkg", "version": "1.0.0"},
+                {"name": "root/no-update", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}
+            ]
+        }
+    ],
+    "require": {
+        "root/dep": "*",
+        "root/no-update": "*"
+    }
+}
+--LOCK--
+{
+    "packages": [
+        {"name": "root/dep", "version": "1.1.0", "require": {"replacer/pkg": "1.*"}},
+        {"name": "replacer/pkg", "version": "1.0.0", "replace": {"replaced/pkg": "1.0.0"}},
+        {"name": "root/no-update", "version": "1.0.0", "require": {"replaced/pkg": "1.0.0"}}
+    ],
+    "packages-dev": [],
+    "aliases": [],
+    "minimum-stability": "dev",
+    "stability-flags": [],
+    "prefer-stable": false,
+    "prefer-lowest": false,
+    "platform": [],
+    "platform-dev": []
+}
+--RUN--
+update root/dep --with-all-dependencies
+--EXPECT--
+Installing replacer/pkg (1.1.0)
+Installing root/dep (1.2.0)
+Installing replaced/pkg (1.0.0)
+Installing root/no-update (1.0.0)
+