Merge pull request composer#9360 from naderman/pool-builder-unlock-co…

…nsider-all-constraints PoolBuilder: Ensure versions matching locked constraints get loaded
nanbando · Dec 3, 2020 · 7f3a56f · 7f3a56f
2 parents adf7beb + db0656e
commit 7f3a56f
Show file tree

Hide file tree

Showing 5 changed files with 213 additions and 23 deletions.
diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php
@@ -372,26 +372,22 @@ private function loadPackage(Request $request, PackageInterface $package, $propa
             $require = $link->getTarget();
             $linkConstraint = $link->getConstraint();
 
-            if ($propagateUpdate) {
-                // if this is a partial update with transitive dependencies we need to unlock the package we now know is a
+            // if the required package is loaded as a locked package only and hasn't had its deps analyzed
+            if (isset($this->skippedLoad[$require])) {
+                // if we're doing a full update or this is a partial update with transitive deps and we're currently
+                // looking at a package which needs to be updated we need to unlock the package we now know is a
                 // dependency of another package which we are trying to update, and then attempt to load it again
-                if ($request->getUpdateAllowTransitiveDependencies() && isset($this->skippedLoad[$require])) {
+                if ($propagateUpdate && $request->getUpdateAllowTransitiveDependencies()) {
                     if ($request->getUpdateAllowTransitiveRootDependencies() || !$this->isRootRequire($request, $this->skippedLoad[$require])) {
                         $this->unlockPackage($request, $require);
                         $this->markPackageNameForLoading($request, $require, $linkConstraint);
-                    } elseif (!$request->getUpdateAllowTransitiveRootDependencies() && $this->isRootRequire($request, $require) && !isset($this->updateAllowWarned[$require])) {
-                        $this->updateAllowWarned[$require] = true;
-                        $this->io->writeError('<warning>Dependency "'.$require.'" is also a root requirement. Package has not been listed as an update argument, so keeping locked at old version. Use --with-all-dependencies (-W) to include root dependencies.</warning>');
+                    } elseif (!isset($this->updateAllowWarned[$this->skippedLoad[$require]])) {
+                        $this->updateAllowWarned[$this->skippedLoad[$require]] = true;
+                        $this->io->writeError('<warning>Dependency "'.$this->skippedLoad[$require].'" is also a root requirement. Package has not been listed as an update argument, so keeping locked at old version. Use --with-all-dependencies (-W) to include root dependencies.</warning>');
                     }
-                } else {
-                    $this->markPackageNameForLoading($request, $require, $linkConstraint);
                 }
             } else {
-                // We also need to load the requirements of a locked package
-                // unless it was skipped
-                if (!isset($this->skippedLoad[$require])) {
-                    $this->markPackageNameForLoading($request, $require, $linkConstraint);
-                }
+                $this->markPackageNameForLoading($request, $require, $linkConstraint);
             }
         }
 
@@ -471,15 +467,6 @@ private function warnAboutNonMatchingUpdateAllowList(Request $request)
      */
     private function unlockPackage(Request $request, $name)
     {
-        // remove locked package by this name which was already initialized
-        foreach ($request->getLockedPackages() as $lockedPackage) {
-            if (!($lockedPackage instanceof AliasPackage) && $lockedPackage->getName() === $name) {
-                if (false !== $index = array_search($lockedPackage, $this->packages, true)) {
-                    $request->unlockPackage($lockedPackage);
-                    $this->removeLoadedPackage($request, $lockedPackage, $index);
-                }
-            }
-        }
 
         if (
             // if we unfixed a replaced package name, we also need to unfix the replacer itself
@@ -491,6 +478,29 @@ private function unlockPackage(Request $request, $name)
         }
 
         unset($this->skippedLoad[$name], $this->loadedPackages[$name], $this->maxExtendedReqs[$name]);
+
+        // remove locked package by this name which was already initialized
+        foreach ($request->getLockedPackages() as $lockedPackage) {
+            if (!($lockedPackage instanceof AliasPackage) && $lockedPackage->getName() === $name) {
+                if (false !== $index = array_search($lockedPackage, $this->packages, true)) {
+                    $request->unlockPackage($lockedPackage);
+                    $this->removeLoadedPackage($request, $lockedPackage, $index);
+
+                    // 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
+                    foreach ($request->getFixedOrLockedPackages() as $fixedOrLockedPackage) {
+                        if ($fixedOrLockedPackage !== $lockedPackage && isset($this->skippedLoad[$fixedOrLockedPackage->getName()])) {
+                            foreach ($fixedOrLockedPackage->getRequires() as $requireLink) {
+                                if ($requireLink->getTarget() === $lockedPackage->getName()) {
+                                    $this->markPackageNameForLoading($request, $lockedPackage->getName(), $requireLink->getConstraint());
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
     }
 
     private function removeLoadedPackage(Request $request, PackageInterface $package, $index)

diff --git a/src/Composer/DependencyResolver/Problem.php b/src/Composer/DependencyResolver/Problem.php
@@ -271,7 +271,7 @@ public static function getMissingPackageReason(RepositorySet $repositorySet, Req
                 return array("- Root composer.json requires $packageName".self::constraintToText($constraint) . ', ', 'found '.self::getPackageList($packages, $isVerbose).' in lock file but not in remote repositories, make sure you avoid updating this package to keep the one from lock file.');
             }
 
-            return array("- Root composer.json requires $packageName".self::constraintToText($constraint) . ', ', 'found '.self::getPackageList($packages, $isVerbose).' but '.(self::hasMultipleNames($packages) ? 'these conflict' : 'it conflicts').' with another require.');
+            return array("- Root composer.json requires $packageName".self::constraintToText($constraint) . ', ', 'found '.self::getPackageList($packages, $isVerbose).' but these were not loaded, likely because '.(self::hasMultipleNames($packages) ? 'they conflict' : 'it conflicts').' with another require.');
         }
 
         // check if the package is found when bypassing stability checks

diff --git a/...ser/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test b/...ser/Test/DependencyResolver/Fixtures/poolbuilder/partial-update-unfixing-locked-deps.test
@@ -0,0 +1,50 @@
+--TEST--
+Unlocking a package also unlocks its dependencies when transitive deps are true. But version constraints from other
+locked packages still need to be taking into account for loading all necessary versions of these transitive deps.
+
+--REQUEST--
+{
+    "require": {
+        "root/req1": "*",
+        "root/req2": "*"
+    },
+    "locked": [
+        {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}},
+        {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}},
+        {"name": "dep/pkg1", "version": "1.0.0", "author": "old"},
+        {"name": "dep/pkg2", "version": "1.0.0"}
+    ],
+    "allowList": [
+        "dep/pkg2"
+    ],
+    "allowTransitiveDeps": true
+}
+
+--FIXED--
+[
+]
+
+--PACKAGE-REPOS--
+[
+    [
+        {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}},
+        {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}},
+        {"name": "dep/pkg1", "version": "1.0.0", "author": "new"},
+        {"name": "dep/pkg1", "version": "1.0.1"},
+        {"name": "dep/pkg1", "version": "2.0.0"},
+        {"name": "dep/pkg1", "version": "3.0.0"},
+        {"name": "dep/pkg2", "version": "1.0.0"},
+        {"name": "dep/pkg2", "version": "1.2.0", "require": {"dep/pkg1": "2.*"}}
+    ]
+]
+
+--EXPECT--
+[
+    "root/req1-1.0.0.0 (locked)",
+    "root/req2-1.0.0.0 (locked)",
+    "dep/pkg2-1.0.0.0",
+    "dep/pkg2-1.2.0.0",
+    "dep/pkg1-1.0.0.0",
+    "dep/pkg1-1.0.1.0",
+    "dep/pkg1-2.0.0.0"
+]
diff --git a/...est/Fixtures/installer/partial-update-keeps-older-dep-if-still-required-with-provide.test b/...est/Fixtures/installer/partial-update-keeps-older-dep-if-still-required-with-provide.test
@@ -0,0 +1,65 @@
+--TEST--
+Ensure that a partial update of a dependency does not conflict if the only way to proceed is using an old locked version.
+
+--COMPOSER--
+{
+    "repositories": [
+        {
+            "type": "package",
+            "package": [
+                {"name": "root/req1", "version": "1.0.0", "require": {"virtual/pkg1": "1.*"}},
+                {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*", "dep/pkg1": "*"}},
+                {"name": "dep/pkg1", "version": "1.0.0", "provide": {"virtual/pkg1": "1.0.0"}},
+                {"name": "dep/pkg1", "version": "2.0.0"},
+                {"name": "dep/pkg2", "version": "1.0.0"},
+                {"name": "dep/pkg2", "version": "1.0.1"},
+                {"name": "dep/pkg2", "version": "1.2.0", "require": {"virtual/pkg1": "2.*"}}
+            ]
+        }
+    ],
+    "require": {
+        "root/req1": "*",
+        "root/req2": "*"
+    }
+}
+--LOCK--
+{
+    "packages": [
+        {"name": "dep/pkg1", "version": "1.0.0", "type": "library", "provide": {"virtual/pkg1": "1.0.0"}},
+        {"name": "dep/pkg2", "version": "1.0.0", "type": "library"},
+        {"name": "root/req1", "version": "1.0.0", "require": {"virtual/pkg1": "1.*"}, "type": "library"},
+        {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*", "dep/pkg1": "*"}, "type": "library"}
+    ],
+    "packages-dev": [],
+    "aliases": [],
+    "minimum-stability": "stable",
+    "stability-flags": {},
+    "prefer-stable": false,
+    "prefer-lowest": false,
+    "platform": [],
+    "platform-dev": []
+}
+--RUN--
+update dep/pkg2 --with-dependencies
+--EXPECT-LOCK--
+{
+    "packages": [
+        {"name": "dep/pkg1", "version": "1.0.0", "type": "library", "provide": {"virtual/pkg1": "1.0.0"}},
+        {"name": "dep/pkg2", "version": "1.0.1", "type": "library"},
+        {"name": "root/req1", "version": "1.0.0", "require": {"virtual/pkg1": "1.*"}, "type": "library"},
+        {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*", "dep/pkg1": "*"}, "type": "library"}
+    ],
+    "packages-dev": [],
+    "aliases": [],
+    "minimum-stability": "stable",
+    "stability-flags": [],
+    "prefer-stable": false,
+    "prefer-lowest": false,
+    "platform": [],
+    "platform-dev": []
+}
+--EXPECT--
+Installing dep/pkg1 (1.0.0)
+Installing root/req1 (1.0.0)
+Installing dep/pkg2 (1.0.1)
+Installing root/req2 (1.0.0)
diff --git a/tests/Composer/Test/Fixtures/installer/partial-update-keeps-older-dep-if-still-required.test b/tests/Composer/Test/Fixtures/installer/partial-update-keeps-older-dep-if-still-required.test
@@ -0,0 +1,65 @@
+--TEST--
+Ensure that a partial update of a dependency does not conflict if the only way to proceed is using an old locked version.
+
+--COMPOSER--
+{
+    "repositories": [
+        {
+            "type": "package",
+            "package": [
+                {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}},
+                {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}},
+                {"name": "dep/pkg1", "version": "1.0.0"},
+                {"name": "dep/pkg1", "version": "2.0.0"},
+                {"name": "dep/pkg2", "version": "1.0.0"},
+                {"name": "dep/pkg2", "version": "1.0.1"},
+                {"name": "dep/pkg2", "version": "1.2.0", "require": {"dep/pkg1": "2.*"}}
+            ]
+        }
+    ],
+    "require": {
+        "root/req1": "*",
+        "root/req2": "*"
+    }
+}
+--LOCK--
+{
+    "packages": [
+        {"name": "dep/pkg1", "version": "1.0.0", "type": "library"},
+        {"name": "dep/pkg2", "version": "1.0.1", "type": "library"},
+        {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}, "type": "library"},
+        {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}, "type": "library"}
+    ],
+    "packages-dev": [],
+    "aliases": [],
+    "minimum-stability": "stable",
+    "stability-flags": {},
+    "prefer-stable": false,
+    "prefer-lowest": false,
+    "platform": [],
+    "platform-dev": []
+}
+--RUN--
+update dep/pkg2 --with-dependencies
+--EXPECT-LOCK--
+{
+    "packages": [
+        {"name": "dep/pkg1", "version": "1.0.0", "type": "library"},
+        {"name": "dep/pkg2", "version": "1.0.1", "type": "library"},
+        {"name": "root/req1", "version": "1.0.0", "require": {"dep/pkg1": "1.*"}, "type": "library"},
+        {"name": "root/req2", "version": "1.0.0", "require": {"dep/pkg2": "1.*"}, "type": "library"}
+    ],
+    "packages-dev": [],
+    "aliases": [],
+    "minimum-stability": "stable",
+    "stability-flags": [],
+    "prefer-stable": false,
+    "prefer-lowest": false,
+    "platform": [],
+    "platform-dev": []
+}
+--EXPECT--
+Installing dep/pkg1 (1.0.0)
+Installing root/req1 (1.0.0)
+Installing dep/pkg2 (1.0.1)
+Installing root/req2 (1.0.0)