From 4b9b499ce5f7bee87a23fb49591a9078c92e9829 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Sat, 6 Jun 2020 17:16:54 +0200 Subject: [PATCH] Fix issue loading aliases and fix markPackageNameForLoading when called twice in a row for same package it would overwrite the constraint the second time --- .../DependencyResolver/PoolBuilder.php | 70 ++++++++++++------- .../alias-priority-conflicting.test | 61 ++++++++++++++++ .../poolbuilder/alias-with-reference.test | 57 +++++++++++++++ ...minimum-stability-and-filter-packages.test | 2 +- .../DependencyResolver/PoolBuilderTest.php | 18 +++-- 5 files changed, 175 insertions(+), 33 deletions(-) create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/alias-priority-conflicting.test create mode 100644 tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/alias-with-reference.test diff --git a/src/Composer/DependencyResolver/PoolBuilder.php b/src/Composer/DependencyResolver/PoolBuilder.php index 9f8f4280e..c6ea7d73d 100644 --- a/src/Composer/DependencyResolver/PoolBuilder.php +++ b/src/Composer/DependencyResolver/PoolBuilder.php @@ -85,6 +85,8 @@ class PoolBuilder */ private $updateAllowWarned = array(); + private $indexCounter = 0; + /** * @param int[] $acceptableStabilities array of stability => BasePackage::STABILITY_* value * @psalm-param array $acceptableStabilities @@ -154,7 +156,7 @@ class PoolBuilder continue; } - $this->markPackageNameForLoading($packageName, $constraint); + $this->markPackageNameForLoading($request, $packageName, $constraint); } // clean up packagesToLoad for anything we manually marked loaded above @@ -216,19 +218,28 @@ class PoolBuilder $this->loadedPackages = array(); $this->packages = array(); $this->unacceptableFixedPackages = array(); + $this->indexCounter = 0; return $pool; } - private function markPackageNameForLoading($name, ConstraintInterface $constraint) + private function markPackageNameForLoading(Request $request, $name, ConstraintInterface $constraint) { // Maybe it was already marked before but not loaded yet. In that case // we have to extend the constraint (we don't check if they match because // MultiConstraint::create() will optimize anyway) - if (isset($this->packagesToLoad[$name]) && !Intervals::isSubsetOf($constraint, $this->packagesToLoad[$name])) { + if (isset($this->packagesToLoad[$name])) { + // Already marked for loading and this does not expand the constraint to be loaded, nothing to do + if (Intervals::isSubsetOf($constraint, $this->packagesToLoad[$name])) { + return; + } + + // extend the constraint to be loaded $constraint = MultiConstraint::create(array($this->packagesToLoad[$name], $constraint), false); } + // Not yet loaded or already marked for a reload, override the existing constraint + // (either it's a new one to load, or it has already been extended above) if (!isset($this->loadedPackages[$name])) { $this->packagesToLoad[$name] = $constraint; return; @@ -245,18 +256,18 @@ class PoolBuilder // yet so we get the required package versions $this->packagesToLoad[$name] = MultiConstraint::create(array($this->loadedPackages[$name], $constraint), false); unset($this->loadedPackages[$name]); - - // remove all already-loaded packages matching those to be loaded to avoid duplicates - foreach ($this->packages as $index => $pkg) { - if ($pkg->getName() === $name) { - unset($this->packages[$index]); - } - } } private function loadPackagesMarkedForLoading(Request $request, $repositories) { foreach ($this->packagesToLoad as $name => $constraint) { + // remove all already-loaded packages matching those to be loaded to avoid duplicates + foreach ($this->packages as $index => $pkg) { + if ($pkg->getName() === $name) { + $this->removeLoadedPackage($request, $pkg, $index); + } + } + $this->loadedPackages[$name] = $constraint; } @@ -287,9 +298,8 @@ class PoolBuilder private function loadPackage(Request $request, PackageInterface $package, $propagateUpdate = true) { - end($this->packages); - $index = key($this->packages) + 1; - $this->packages[] = $package; + $index = $this->indexCounter++; + $this->packages[$index] = $package; if ($package instanceof AliasPackage) { $this->aliasMap[spl_object_hash($package->getAliasOf())][$index] = $package; @@ -319,8 +329,9 @@ class PoolBuilder $aliasPackage = new AliasPackage($basePackage, $alias['alias_normalized'], $alias['alias']); $aliasPackage->setRootPackageAlias(true); - $this->packages[] = $aliasPackage; - $this->aliasMap[spl_object_hash($aliasPackage->getAliasOf())][$index+1] = $aliasPackage; + $newIndex = $this->indexCounter++; + $this->packages[$newIndex] = $aliasPackage; + $this->aliasMap[spl_object_hash($aliasPackage->getAliasOf())][$newIndex] = $aliasPackage; } foreach ($package->getRequires() as $link) { @@ -333,19 +344,19 @@ class PoolBuilder if ($request->getUpdateAllowTransitiveDependencies() && isset($this->skippedLoad[$require])) { if ($request->getUpdateAllowTransitiveRootDependencies() || !$this->isRootRequire($request, $this->skippedLoad[$require])) { $this->unfixPackage($request, $require); - $this->markPackageNameForLoading($require, $linkConstraint); + $this->markPackageNameForLoading($request, $require, $linkConstraint); } elseif (!$request->getUpdateAllowTransitiveRootDependencies() && $this->isRootRequire($request, $require) && !isset($this->updateAllowWarned[$require])) { $this->updateAllowWarned[$require] = true; $this->io->writeError('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 to include root dependencies.'); } } else { - $this->markPackageNameForLoading($require, $linkConstraint); + $this->markPackageNameForLoading($request, $require, $linkConstraint); } } else { // We also need to load the requirements of a fixed package // unless it was skipped if (!isset($this->skippedLoad[$require])) { - $this->markPackageNameForLoading($require, $linkConstraint); + $this->markPackageNameForLoading($request, $require, $linkConstraint); } } } @@ -358,7 +369,7 @@ class PoolBuilder if (isset($this->loadedPackages[$replace]) && isset($this->skippedLoad[$replace])) { if ($request->getUpdateAllowTransitiveRootDependencies() || !$this->isRootRequire($request, $this->skippedLoad[$replace])) { $this->unfixPackage($request, $replace); - $this->markPackageNameForLoading($replace, $link->getConstraint()); + $this->markPackageNameForLoading($request, $replace, $link->getConstraint()); } elseif (!$request->getUpdateAllowTransitiveRootDependencies() && $this->isRootRequire($request, $replace) && !isset($this->updateAllowWarned[$replace])) { $this->updateAllowWarned[$replace] = true; $this->io->writeError('Dependency "'.$replace.'" is also a root requirement. Package has not been listed as an update argument, so keeping locked at old version. Use --with-all-dependencies to include root dependencies.'); @@ -430,14 +441,7 @@ class PoolBuilder if (!($lockedPackage instanceof AliasPackage) && $lockedPackage->getName() === $name) { if (false !== $index = array_search($lockedPackage, $this->packages, true)) { $request->unfixPackage($lockedPackage); - unset($this->packages[$index]); - if (isset($this->aliasMap[spl_object_hash($lockedPackage)])) { - foreach ($this->aliasMap[spl_object_hash($lockedPackage)] as $aliasIndex => $aliasPackage) { - $request->unfixPackage($aliasPackage); - unset($this->packages[$aliasIndex]); - } - unset($this->aliasMap[spl_object_hash($lockedPackage)]); - } + $this->removeLoadedPackage($request, $lockedPackage, $index); } } } @@ -455,6 +459,18 @@ class PoolBuilder unset($this->loadedPackages[$name]); } + private function removeLoadedPackage(Request $request, PackageInterface $package, $index) + { + unset($this->packages[$index]); + if (isset($this->aliasMap[spl_object_hash($package)])) { + foreach ($this->aliasMap[spl_object_hash($package)] as $aliasIndex => $aliasPackage) { + $request->unfixPackage($aliasPackage); + unset($this->packages[$aliasIndex]); + } + unset($this->aliasMap[spl_object_hash($package)]); + } + } + private function getRootAliasesPerPackage(array $aliases) { $normalizedAliases = array(); diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/alias-priority-conflicting.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/alias-priority-conflicting.test new file mode 100644 index 000000000..68136d6c2 --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/alias-priority-conflicting.test @@ -0,0 +1,61 @@ +--TEST-- +Check root aliases are loaded + +--ROOT-- +{ + "minimum-stability": "dev", + "aliases": [ + { + "package": "req/pkg", + "version": "dev-feature-foo", + "alias": "dev-master" + } + ] +} + + +--REQUEST-- +{ + "require": { + "package/a": "dev-master", + "req/pkg": "dev-feature-foo" + } +} + +--FIXED-- +[ +] + +--PACKAGES-- +[ + { + "name": "req/pkg", "version": "dev-feature-foo", + "source": { "reference": "feat.f", "type": "git", "url": "" } + }, + { + "name": "req/pkg", "version": "dev-master", + "extra": { "branch-alias": { "dev-master": "1.0.x-dev" } }, + "source": { "reference": "forked", "type": "git", "url": "" } + }, + { + "name": "req/pkg", "version": "dev-master", + "extra": { "branch-alias": { "dev-master": "1.0.x-dev" } }, + "source": { "reference": "master", "type": "git", "url": "" } + }, + { + "name": "package/a", "version": "dev-master", + "require": { "req/pkg": "dev-master" } + } +] + +--EXPECT-- +[ + "package/a-dev-master", + "package/a-9999999-dev (alias of dev-master)", + "req/pkg-dev-feature-foo#feat.f", + "req/pkg-dev-master#feat.f (alias of dev-feature-foo)", + "req/pkg-dev-master#forked", + "req/pkg-dev-master#master", + "req/pkg-1.0.9999999.9999999-dev#forked (alias of dev-master)", + "req/pkg-1.0.9999999.9999999-dev#master (alias of dev-master)" +] diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/alias-with-reference.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/alias-with-reference.test new file mode 100644 index 000000000..4401f869e --- /dev/null +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/alias-with-reference.test @@ -0,0 +1,57 @@ +--TEST-- +Check root aliases get selected correctly + +--ROOT-- +{ + "stability-flags": { + "a/aliased": "dev" + }, + "aliases": [ + { + "package": "a/aliased", + "version": "dev-master", + "alias": "1.0.0" + } + ], + "references": { + "a/aliased": "abcd" + } +} + + +--REQUEST-- +{ + "require": { + "a/aliased": "dev-master", + "b/requirer": "*" + } +} + +--FIXED-- +[ +] + +--PACKAGES-- +[ + { + "name": "a/aliased", "version": "dev-master", + "source": { "reference": "orig", "type": "git", "url": "" } + }, + { + "name": "a/aliased", "version": "1.0" + }, + { + "name": "b/requirer", "version": "1.0.0", + "require": { "a/aliased": "1.0.0" }, + "source": { "reference": "1.0.0", "type": "git", "url": "" } + } +] + +--EXPECT-- +[ + "b/requirer-1.0.0.0#1.0.0", + "a/aliased-dev-master#abcd", + "a/aliased-1.0.0.0#abcd (alias of dev-master)", + "a/aliased-1.0.0.0#abcd", + "a/aliased-9999999-dev#abcd (alias of dev-master)" +] diff --git a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/stability-flags-take-over-minimum-stability-and-filter-packages.test b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/stability-flags-take-over-minimum-stability-and-filter-packages.test index b273753d2..3f6d8346c 100644 --- a/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/stability-flags-take-over-minimum-stability-and-filter-packages.test +++ b/tests/Composer/Test/DependencyResolver/Fixtures/poolbuilder/stability-flags-take-over-minimum-stability-and-filter-packages.test @@ -42,5 +42,5 @@ Stability flags apply 4, 5, 6, - "default/pkg-1.2.0.0 alias of 6" + "default/pkg-1.2.0.0 (alias of 6)" ] diff --git a/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php b/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php index b756219a2..75e2b76b0 100644 --- a/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php +++ b/tests/Composer/Test/DependencyResolver/PoolBuilderTest.php @@ -41,6 +41,7 @@ class PoolBuilderTest extends TestCase $rootAliases = !empty($root['aliases']) ? $root['aliases'] : array(); $minimumStability = !empty($root['minimum-stability']) ? $root['minimum-stability'] : 'stable'; $stabilityFlags = !empty($root['stability-flags']) ? $root['stability-flags'] : array(); + $rootReferences = !empty($root['references']) ? $root['references'] : array(); $stabilityFlags = array_map(function ($stability) { return BasePackage::$stabilities[$stability]; }, $stabilityFlags); @@ -71,7 +72,7 @@ class PoolBuilderTest extends TestCase return $pkg; }; - $repositorySet = new RepositorySet($minimumStability, $stabilityFlags, $rootAliases); + $repositorySet = new RepositorySet($minimumStability, $stabilityFlags, $rootAliases, $rootReferences); $repositorySet->addRepository($repo = new ArrayRepository()); $repositorySet->addRepository($lockedRepo = new LockArrayRepository()); foreach ($packages as $package) { @@ -114,15 +115,22 @@ class PoolBuilderTest extends TestCase } $suffix = ''; + if ($package->getSourceReference()) { + $suffix = '#'.$package->getSourceReference(); + } if ($package->getRepository() instanceof LockArrayRepository) { - $suffix = ' (locked)'; + $suffix .= ' (locked)'; } - if ($package instanceof AliasPackage && $id = array_search($package->getAliasOf(), $packageIds, true)) { - return (string) $package->getName().'-'.$package->getVersion() .' alias of '.$id . $suffix; + if ($package instanceof AliasPackage) { + if ($id = array_search($package->getAliasOf(), $packageIds, true)) { + return (string) $package->getName().'-'.$package->getVersion() . $suffix . ' (alias of '.$id . ')'; + } + + return (string) $package->getName().'-'.$package->getVersion() . $suffix . ' (alias of '.$package->getAliasOf()->getVersion().')'; } - return (string) $package . $suffix; + return (string) $package->getName().'-'.$package->getVersion() . $suffix; }, $result); $this->assertSame($expect, $result);