From 010bad5428b40ca69b0d8db025eacc48f6391609 Mon Sep 17 00:00:00 2001 From: Jason Woods Date: Wed, 29 Dec 2021 12:21:36 +0000 Subject: [PATCH 1/4] fix: If a replacer is updated to a version that no longer replaces, the replaced package is not loaded --- .../DependencyResolver/PoolBuilder.php | 8 +++ ...-replaced-package-if-replacer-dropped.test | 50 +++++++++++++++++++ .../DependencyResolver/PoolBuilderTest.php | 4 ++ 3 files changed, 62 insertions(+) create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/load-replaced-package-if-replacer-dropped.test diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index 9de4bed71..8f5f5cd15 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -639,6 +639,8 @@ class PoolBuilder // 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,6 +651,12 @@ class PoolBuilder 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()); + $this->markPackageNameForLoading($request, $replace->getTarget(), $requires[$replace->getTarget()]->getConstraint()); + } + } } } } diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/load-replaced-package-if-replacer-dropped.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/load-replaced-package-if-replacer-dropped.test new file mode 100644 index 000000000..6c148c1d8 --- /dev/null +++ b/tests/Composer/Test/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/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php b/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php index db7b0390e..433008ddc 100644 --- a/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php +++ b/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php @@ -137,10 +137,14 @@ class PoolBuilderTest extends TestCase $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); From 995b806dfe41734456d018cbc61e8a0bbe609434 Mon Sep 17 00:00:00 2001 From: Yanick Witschi Date: Tue, 2 May 2023 18:08:46 +0200 Subject: [PATCH 2/4] Optimize PoolBuilder to not load replaced targets if not required --- .../DependencyResolver/PoolBuilder.php | 37 +++++++++++++++++-- ...multi-repo-replace-partial-update-all.test | 2 - 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index 8f5f5cd15..01115fd65 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -94,6 +94,11 @@ class PoolBuilder * @phpstan-var array>> */ private $loadedPerRepo = []; + /** + * @var array[] + * @phpstan-var array + */ + private $optionalPackages = []; /** * @var BasePackage[] */ @@ -235,8 +240,9 @@ class PoolBuilder } } - 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 @@ class PoolBuilder 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])) { @@ -651,10 +658,12 @@ class PoolBuilder 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()); - $this->markPackageNameForLoading($request, $replace->getTarget(), $requires[$replace->getTarget()]->getConstraint()); + // Mark as optional - if no other package requires it, we don't need to load it + $this->markPackageNameForOptionalLoading($replace->getTarget()); } } } @@ -664,6 +673,28 @@ class PoolBuilder } } + 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/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/multi-repo-replace-partial-update-all.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/multi-repo-replace-partial-update-all.test index 72e88141a..5ffd3ce14 100644 --- a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/multi-repo-replace-partial-update-all.test +++ b/tests/Composer/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" ] From f9944867c3830ea29ec2a6cdd27f0d45fcd83d99 Mon Sep 17 00:00:00 2001 From: Yanick Witschi Date: Tue, 2 May 2023 21:39:45 +0200 Subject: [PATCH 3/4] Added integration test --- ...-replaced-package-if-replacer-dropped.test | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 tests/Composer/Test/Fixtures/installer/load-replaced-package-if-replacer-dropped.test 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 new file mode 100644 index 000000000..851e03a97 --- /dev/null +++ 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) + From 9ced20fd0df986ed930def71bff76bd7f9fa8157 Mon Sep 17 00:00:00 2001 From: Yanick Witschi Date: Tue, 2 May 2023 23:11:20 +0200 Subject: [PATCH 4/4] Take the short cut --- src/Composer/DependencyResolver/PoolBuilder.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index 01115fd65..fc3b95968 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -662,8 +662,10 @@ class PoolBuilder foreach ($lockedPackage->getReplaces() as $replace) { if (isset($requires[$replace->getTarget()], $this->skippedLoad[$replace->getTarget()])) { $this->unlockPackage($request, $repositories, $replace->getTarget()); - // Mark as optional - if no other package requires it, we don't need to load it - $this->markPackageNameForOptionalLoading($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()); } } }