From db4055b778d6682f951883da3dab0744f76b90c2 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Wed, 17 Apr 2013 15:39:42 +0200 Subject: [PATCH 1/3] Put a higher prio on replacers of the same vendor as the required package --- .../DependencyResolver/DefaultPolicy.php | 35 +++++++++++++++---- src/Composer/DependencyResolver/Rule.php | 12 +++++++ src/Composer/DependencyResolver/Solver.php | 2 +- .../DependencyResolver/DefaultPolicyTest.php | 28 ++++++++++++++- .../installer/replace-vendor-priorities.test | 21 +++++++++++ 5 files changed, 90 insertions(+), 8 deletions(-) create mode 100644 tests/Composer/Test/Fixtures/installer/replace-vendor-priorities.test diff --git a/src/Composer/DependencyResolver/DefaultPolicy.php b/src/Composer/DependencyResolver/DefaultPolicy.php index d59a07d55..6be7dcc23 100644 --- a/src/Composer/DependencyResolver/DefaultPolicy.php +++ b/src/Composer/DependencyResolver/DefaultPolicy.php @@ -60,14 +60,14 @@ class DefaultPolicy implements PolicyInterface return $pool->getPriority($package->getRepository()); } - public function selectPreferedPackages(Pool $pool, array $installedMap, array $literals) + public function selectPreferedPackages(Pool $pool, array $installedMap, array $literals, Rule $rule = null) { $packages = $this->groupLiteralsByNamePreferInstalled($pool, $installedMap, $literals); foreach ($packages as &$literals) { $policy = $this; - usort($literals, function ($a, $b) use ($policy, $pool, $installedMap) { - return $policy->compareByPriorityPreferInstalled($pool, $installedMap, $pool->literalToPackage($a), $pool->literalToPackage($b), true); + usort($literals, function ($a, $b) use ($policy, $pool, $installedMap, $rule) { + return $policy->compareByPriorityPreferInstalled($pool, $installedMap, $pool->literalToPackage($a), $pool->literalToPackage($b), $rule, true); }); } @@ -82,8 +82,8 @@ class DefaultPolicy implements PolicyInterface $selected = call_user_func_array('array_merge', $packages); // now sort the result across all packages to respect replaces across packages - usort($selected, function ($a, $b) use ($policy, $pool, $installedMap) { - return $policy->compareByPriorityPreferInstalled($pool, $installedMap, $pool->literalToPackage($a), $pool->literalToPackage($b)); + usort($selected, function ($a, $b) use ($policy, $pool, $installedMap, $rule) { + return $policy->compareByPriorityPreferInstalled($pool, $installedMap, $pool->literalToPackage($a), $pool->literalToPackage($b), $rule); }); return $selected; @@ -109,7 +109,10 @@ class DefaultPolicy implements PolicyInterface return $packages; } - public function compareByPriorityPreferInstalled(Pool $pool, array $installedMap, PackageInterface $a, PackageInterface $b, $ignoreReplace = false) + /** + * @protected + */ + public function compareByPriorityPreferInstalled(Pool $pool, array $installedMap, PackageInterface $a, PackageInterface $b, Rule $rule = null, $ignoreReplace = false) { if ($a->getRepository() === $b->getRepository()) { // prefer aliases to the original package @@ -132,6 +135,26 @@ class DefaultPolicy implements PolicyInterface if ($this->replaces($b, $a)) { return -1; // use a } + + // for replacers not replacing each other, put a higher prio on replacing + // packages with the same vendor as the required package + if ($rule) { + if ($rule->getReason() === Rule::RULE_JOB_INSTALL) { + $required = $rule->getReasonData(); + } elseif ($rule->getReason() === Rule::RULE_PACKAGE_REQUIRES) { + $required = $rule->getReasonData()->getTarget(); + } + + if ($required && (false !== ($pos = strpos($required, '/')))) { + $requiredVendor = substr($required, 0, $pos); + $aIsSameVendor = substr($a->getName(), 0, $pos) === $requiredVendor; + $bIsSameVendor = substr($b->getName(), 0, $pos) === $requiredVendor; + + if ($bIsSameVendor !== $aIsSameVendor) { + return $aIsSameVendor ? -1 : 1; + } + } + } } // priority equal, sort by package id to make reproducible diff --git a/src/Composer/DependencyResolver/Rule.php b/src/Composer/DependencyResolver/Rule.php index d249f6d37..13c253848 100644 --- a/src/Composer/DependencyResolver/Rule.php +++ b/src/Composer/DependencyResolver/Rule.php @@ -35,6 +35,8 @@ class Rule protected $literals; protected $type; protected $id; + protected $reason; + protected $reasonData; protected $job; @@ -80,6 +82,16 @@ class Rule return $this->job; } + public function getReason() + { + return $this->reason; + } + + public function getReasonData() + { + return $this->reasonData; + } + /** * Checks if this rule is equal to another one * diff --git a/src/Composer/DependencyResolver/Solver.php b/src/Composer/DependencyResolver/Solver.php index 3c46c0c41..05bb0b400 100644 --- a/src/Composer/DependencyResolver/Solver.php +++ b/src/Composer/DependencyResolver/Solver.php @@ -321,7 +321,7 @@ class Solver private function selectAndInstall($level, array $decisionQueue, $disableRules, Rule $rule) { // choose best package to install from decisionQueue - $literals = $this->policy->selectPreferedPackages($this->pool, $this->installedMap, $decisionQueue); + $literals = $this->policy->selectPreferedPackages($this->pool, $this->installedMap, $decisionQueue, $rule); $selectedLiteral = array_shift($literals); diff --git a/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php b/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php index 8b82cc54d..3a17bf7ba 100644 --- a/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php +++ b/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php @@ -16,6 +16,7 @@ use Composer\Repository\ArrayRepository; use Composer\Repository\RepositoryInterface; use Composer\DependencyResolver\DefaultPolicy; use Composer\DependencyResolver\Pool; +use Composer\DependencyResolver\Rule; use Composer\Package\Link; use Composer\Package\AliasPackage; use Composer\Package\LinkConstraint\VersionConstraint; @@ -177,7 +178,6 @@ class DefaultPolicyTest extends TestCase public function testPreferNonReplacingFromSameRepo() { - $this->repo->addPackage($packageA = $this->getPackage('A', '1.0')); $this->repo->addPackage($packageB = $this->getPackage('B', '2.0')); @@ -193,6 +193,32 @@ class DefaultPolicyTest extends TestCase $this->assertEquals($expected, $selected); } + public function testPreferReplacingPackageFromSameVendor() + { + $this->repo->addPackage($packageB = $this->getPackage('vendor-b/replacer', '1.0')); + $this->repo->addPackage($packageA = $this->getPackage('vendor-a/replacer', '1.0')); + + $packageA->setReplaces(array(new Link('vendor-a/replacer', 'vendor-a/package', new VersionConstraint('==', '1.0'), 'replaces'))); + $packageB->setReplaces(array(new Link('vendor-b/replacer', 'vendor-a/package', new VersionConstraint('==', '1.0'), 'replaces'))); + + $this->pool->addRepository($this->repo); + + $literals = array($packageA->getId(), $packageB->getId()); + $expected = $literals; + + // test with install rule + $rule = new Rule($this->pool, $literals, Rule::RULE_JOB_INSTALL, 'vendor-a/package'); + $selected = $this->policy->selectPreferedPackages($this->pool, array(), $literals, $rule); + + $this->assertEquals($expected, $selected); + + // test with requires rule + $rule = new Rule($this->pool, $literals, Rule::RULE_PACKAGE_REQUIRES, new Link('foo', 'vendor-a/package')); + $selected = $this->policy->selectPreferedPackages($this->pool, array(), $literals, $rule); + + $this->assertEquals($expected, $selected); + } + protected function mapFromRepo(RepositoryInterface $repo) { $map = array(); diff --git a/tests/Composer/Test/Fixtures/installer/replace-vendor-priorities.test b/tests/Composer/Test/Fixtures/installer/replace-vendor-priorities.test new file mode 100644 index 000000000..86c491feb --- /dev/null +++ b/tests/Composer/Test/Fixtures/installer/replace-vendor-priorities.test @@ -0,0 +1,21 @@ +--TEST-- +Replacer of the same vendor takes precedence if same prio repo +--COMPOSER-- +{ + "repositories": [ + { + "type": "package", + "package": [ + { "name": "b/replacer", "version": "1.1.0", "replace": { "a/package": "1.1.0" } }, + { "name": "a/replacer", "version": "1.1.0", "replace": { "a/package": "1.1.0" } } + ] + } + ], + "require": { + "a/package": "1.*" + } +} +--RUN-- +install +--EXPECT-- +Installing a/replacer (1.1.0) From 0700cd918682947710b3ae4adebf0c02fafc24c8 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Wed, 17 Apr 2013 17:37:22 +0200 Subject: [PATCH 2/3] Adjust according to feedback --- .../DependencyResolver/DefaultPolicy.php | 31 +++++++------------ src/Composer/DependencyResolver/Rule.php | 13 ++++---- src/Composer/DependencyResolver/Solver.php | 2 +- .../DependencyResolver/DefaultPolicyTest.php | 20 +++++++----- 4 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/Composer/DependencyResolver/DefaultPolicy.php b/src/Composer/DependencyResolver/DefaultPolicy.php index 6be7dcc23..4d2c25855 100644 --- a/src/Composer/DependencyResolver/DefaultPolicy.php +++ b/src/Composer/DependencyResolver/DefaultPolicy.php @@ -60,14 +60,14 @@ class DefaultPolicy implements PolicyInterface return $pool->getPriority($package->getRepository()); } - public function selectPreferedPackages(Pool $pool, array $installedMap, array $literals, Rule $rule = null) + public function selectPreferedPackages(Pool $pool, array $installedMap, array $literals, $requiredPackage = null) { $packages = $this->groupLiteralsByNamePreferInstalled($pool, $installedMap, $literals); foreach ($packages as &$literals) { $policy = $this; - usort($literals, function ($a, $b) use ($policy, $pool, $installedMap, $rule) { - return $policy->compareByPriorityPreferInstalled($pool, $installedMap, $pool->literalToPackage($a), $pool->literalToPackage($b), $rule, true); + usort($literals, function ($a, $b) use ($policy, $pool, $installedMap, $requiredPackage) { + return $policy->compareByPriorityPreferInstalled($pool, $installedMap, $pool->literalToPackage($a), $pool->literalToPackage($b), $requiredPackage, true); }); } @@ -82,8 +82,8 @@ class DefaultPolicy implements PolicyInterface $selected = call_user_func_array('array_merge', $packages); // now sort the result across all packages to respect replaces across packages - usort($selected, function ($a, $b) use ($policy, $pool, $installedMap, $rule) { - return $policy->compareByPriorityPreferInstalled($pool, $installedMap, $pool->literalToPackage($a), $pool->literalToPackage($b), $rule); + usort($selected, function ($a, $b) use ($policy, $pool, $installedMap, $requiredPackage) { + return $policy->compareByPriorityPreferInstalled($pool, $installedMap, $pool->literalToPackage($a), $pool->literalToPackage($b), $requiredPackage); }); return $selected; @@ -112,7 +112,7 @@ class DefaultPolicy implements PolicyInterface /** * @protected */ - public function compareByPriorityPreferInstalled(Pool $pool, array $installedMap, PackageInterface $a, PackageInterface $b, Rule $rule = null, $ignoreReplace = false) + public function compareByPriorityPreferInstalled(Pool $pool, array $installedMap, PackageInterface $a, PackageInterface $b, $requiredPackage = null, $ignoreReplace = false) { if ($a->getRepository() === $b->getRepository()) { // prefer aliases to the original package @@ -138,21 +138,14 @@ class DefaultPolicy implements PolicyInterface // for replacers not replacing each other, put a higher prio on replacing // packages with the same vendor as the required package - if ($rule) { - if ($rule->getReason() === Rule::RULE_JOB_INSTALL) { - $required = $rule->getReasonData(); - } elseif ($rule->getReason() === Rule::RULE_PACKAGE_REQUIRES) { - $required = $rule->getReasonData()->getTarget(); - } + if ($requiredPackage && false !== ($pos = strpos($requiredPackage, '/'))) { + $requiredVendor = substr($requiredPackage, 0, $pos); - if ($required && (false !== ($pos = strpos($required, '/')))) { - $requiredVendor = substr($required, 0, $pos); - $aIsSameVendor = substr($a->getName(), 0, $pos) === $requiredVendor; - $bIsSameVendor = substr($b->getName(), 0, $pos) === $requiredVendor; + $aIsSameVendor = substr($a->getName(), 0, $pos) === $requiredVendor; + $bIsSameVendor = substr($b->getName(), 0, $pos) === $requiredVendor; - if ($bIsSameVendor !== $aIsSameVendor) { - return $aIsSameVendor ? -1 : 1; - } + if ($bIsSameVendor !== $aIsSameVendor) { + return $aIsSameVendor ? -1 : 1; } } } diff --git a/src/Composer/DependencyResolver/Rule.php b/src/Composer/DependencyResolver/Rule.php index 13c253848..a69d3bc10 100644 --- a/src/Composer/DependencyResolver/Rule.php +++ b/src/Composer/DependencyResolver/Rule.php @@ -82,14 +82,15 @@ class Rule return $this->job; } - public function getReason() + public function getRequiredPackage() { - return $this->reason; - } + if ($this->reason === self::RULE_JOB_INSTALL) { + return $this->reasonData; + } - public function getReasonData() - { - return $this->reasonData; + if ($this->reason === self::RULE_PACKAGE_REQUIRES) { + return $this->reasonData->getTarget(); + } } /** diff --git a/src/Composer/DependencyResolver/Solver.php b/src/Composer/DependencyResolver/Solver.php index 05bb0b400..f65f6e0e2 100644 --- a/src/Composer/DependencyResolver/Solver.php +++ b/src/Composer/DependencyResolver/Solver.php @@ -321,7 +321,7 @@ class Solver private function selectAndInstall($level, array $decisionQueue, $disableRules, Rule $rule) { // choose best package to install from decisionQueue - $literals = $this->policy->selectPreferedPackages($this->pool, $this->installedMap, $decisionQueue, $rule); + $literals = $this->policy->selectPreferedPackages($this->pool, $this->installedMap, $decisionQueue, $rule->getRequiredPackage()); $selectedLiteral = array_shift($literals); diff --git a/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php b/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php index 3a17bf7ba..1e8503f2c 100644 --- a/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php +++ b/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php @@ -195,6 +195,7 @@ class DefaultPolicyTest extends TestCase public function testPreferReplacingPackageFromSameVendor() { + // test with default order $this->repo->addPackage($packageB = $this->getPackage('vendor-b/replacer', '1.0')); $this->repo->addPackage($packageA = $this->getPackage('vendor-a/replacer', '1.0')); @@ -206,16 +207,21 @@ class DefaultPolicyTest extends TestCase $literals = array($packageA->getId(), $packageB->getId()); $expected = $literals; - // test with install rule - $rule = new Rule($this->pool, $literals, Rule::RULE_JOB_INSTALL, 'vendor-a/package'); - $selected = $this->policy->selectPreferedPackages($this->pool, array(), $literals, $rule); - + $selected = $this->policy->selectPreferedPackages($this->pool, array(), $literals, 'vendor-a/package'); $this->assertEquals($expected, $selected); - // test with requires rule - $rule = new Rule($this->pool, $literals, Rule::RULE_PACKAGE_REQUIRES, new Link('foo', 'vendor-a/package')); - $selected = $this->policy->selectPreferedPackages($this->pool, array(), $literals, $rule); + // test with reversed order in repo + $repo = new ArrayRepository; + $repo->addPackage($packageA = clone $packageA); + $repo->addPackage($packageB = clone $packageB); + $pool = new Pool('dev'); + $pool->addRepository($this->repo); + + $literals = array($packageA->getId(), $packageB->getId()); + $expected = $literals; + + $selected = $this->policy->selectPreferedPackages($this->pool, array(), $literals, 'vendor-a/package'); $this->assertEquals($expected, $selected); } From b41fd35c2b1cab3648c62f2ec2ebe6cb177fb58d Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Wed, 17 Apr 2013 18:38:05 +0200 Subject: [PATCH 3/3] Remove unused use statement --- tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php b/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php index 1e8503f2c..af0c0f99e 100644 --- a/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php +++ b/tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php @@ -16,7 +16,6 @@ use Composer\Repository\ArrayRepository; use Composer\Repository\RepositoryInterface; use Composer\DependencyResolver\DefaultPolicy; use Composer\DependencyResolver\Pool; -use Composer\DependencyResolver\Rule; use Composer\Package\Link; use Composer\Package\AliasPackage; use Composer\Package\LinkConstraint\VersionConstraint;