From ced24da7b012ec666d8d02ec71ffbcec6e4fb021 Mon Sep 17 00:00:00 2001 From: Yanick Witschi Date: Sat, 12 Mar 2022 14:16:38 +0100 Subject: [PATCH] Fix PoolOptimizer should consider disjunctive MultiConstraints (#10579) --- .../DependencyResolver/PoolOptimizer.php | 60 +++++++++++++++++-- .../pooloptimizer/basic-prefer-highest.test | 10 +++- .../pooloptimizer/complex-prefer-lowest.test | 55 +++++++++++++++++ 3 files changed, 118 insertions(+), 7 deletions(-) create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/complex-prefer-lowest.test diff --git a/src/Composer/DependencyResolver/PoolOptimizer.php b/src/Composer/DependencyResolver/PoolOptimizer.php index ef17938b5c28..939e54a8d85d 100644 --- a/src/Composer/DependencyResolver/PoolOptimizer.php +++ b/src/Composer/DependencyResolver/PoolOptimizer.php @@ -110,21 +110,18 @@ private function prepare(Request $request, Pool $pool) // Extract requested package requirements foreach ($request->getRequires() as $require => $constraint) { - $constraint = Intervals::compactConstraint($constraint); - $this->requireConstraintsPerPackage[$require][(string) $constraint] = $constraint; + $this->extractRequireConstraintsPerPackage($require, $constraint); } // First pass over all packages to extract information and mark package constraints irremovable foreach ($pool->getPackages() as $package) { // Extract package requirements foreach ($package->getRequires() as $link) { - $constraint = Intervals::compactConstraint($link->getConstraint()); - $this->requireConstraintsPerPackage[$link->getTarget()][(string) $constraint] = $constraint; + $this->extractRequireConstraintsPerPackage($link->getTarget(), $link->getConstraint()); } // Extract package conflicts foreach ($package->getConflicts() as $link) { - $constraint = Intervals::compactConstraint($link->getConstraint()); - $this->conflictConstraintsPerPackage[$link->getTarget()][(string) $constraint] = $constraint; + $this->extractConflictConstraintsPerPackage($link->getTarget(), $link->getConstraint()); } // Keep track of alias packages for every package so if either the alias or aliased is kept @@ -453,4 +450,55 @@ private function optimizeImpossiblePackagesAway(Request $request, Pool $pool) } } } + + /** + * Disjunctive require constraints need to be considered in their own group. E.g. "^2.14 || ^3.3" needs to generate + * two require constraint groups in order for us to keep the best matching package for "^2.14" AND "^3.3" as otherwise, we'd + * only keep either one which can cause trouble (e.g. when using --prefer-lowest). + * + * @param string $package + * @param ConstraintInterface $constraint + * @return void + */ + private function extractRequireConstraintsPerPackage($package, ConstraintInterface $constraint) + { + foreach ($this->expandDisjunctiveMultiConstraints($constraint) as $expanded) { + $this->requireConstraintsPerPackage[$package][(string) $expanded] = $expanded; + } + } + + /** + * Disjunctive conflict constraints need to be considered in their own group. E.g. "^2.14 || ^3.3" needs to generate + * two conflict constraint groups in order for us to keep the best matching package for "^2.14" AND "^3.3" as otherwise, we'd + * only keep either one which can cause trouble (e.g. when using --prefer-lowest). + * + * @param string $package + * @param ConstraintInterface $constraint + * @return void + */ + private function extractConflictConstraintsPerPackage($package, ConstraintInterface $constraint) + { + foreach ($this->expandDisjunctiveMultiConstraints($constraint) as $expanded) { + $this->conflictConstraintsPerPackage[$package][(string) $expanded] = $expanded; + } + } + + /** + * + * @param ConstraintInterface $constraint + * @return ConstraintInterface[] + */ + private function expandDisjunctiveMultiConstraints(ConstraintInterface $constraint) + { + $constraint = Intervals::compactConstraint($constraint); + + if ($constraint instanceof MultiConstraint && $constraint->isDisjunctive()) { + // No need to call ourselves recursively here because Intervals::compactConstraint() ensures that there + // are no nested disjunctive MultiConstraint instances possible + return $constraint->getConstraints(); + } + + // Regular constraints and conjunctive MultiConstraints + return array($constraint); + } } diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/basic-prefer-highest.test b/tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/basic-prefer-highest.test index 91131b790204..904d6c3750fe 100644 --- a/tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/basic-prefer-highest.test +++ b/tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/basic-prefer-highest.test @@ -15,7 +15,7 @@ Test filters irrelevant package "package/b" in version 1.0.0 "name": "package/a", "version": "1.0.0", "require": { - "package/b": "^1.0" + "package/b": ">=1.0 <1.1 || ^1.2" } }, { @@ -25,6 +25,10 @@ Test filters irrelevant package "package/b" in version 1.0.0 { "name": "package/b", "version": "1.0.1" + }, + { + "name": "package/b", + "version": "1.2.0" } ] @@ -41,6 +45,10 @@ Test filters irrelevant package "package/b" in version 1.0.0 { "name": "package/b", "version": "1.0.1" + }, + { + "name": "package/b", + "version": "1.2.0" } ] diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/complex-prefer-lowest.test b/tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/complex-prefer-lowest.test new file mode 100644 index 000000000000..74240e145268 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/pooloptimizer/complex-prefer-lowest.test @@ -0,0 +1,55 @@ +--TEST-- +Test keeps package "package/b" in version 2.2.0 because for prefer-lowest either one might be relevant + +--REQUEST-- +{ + "require": { + "package/a": "^1.0" + }, + "preferLowest": true +} + + +--POOL-BEFORE-- +[ + { + "name": "package/a", + "version": "1.0.0", + "require": { + "package/b": "^1.0 || ^2.2" + } + }, + { + "name": "package/b", + "version": "1.0.0" + }, + { + "name": "package/b", + "version": "1.0.1" + }, + { + "name": "package/b", + "version": "2.2.0" + } +] + + +--POOL-AFTER-- +[ + { + "name": "package/a", + "version": "1.0.0", + "require": { + "package/b": "^1.0" + } + }, + { + "name": "package/b", + "version": "1.0.0" + }, + { + "name": "package/b", + "version": "2.2.0" + } +] +