Merge pull request #8528 from Seldaek/repo-prio-fix

Avoid loading packages found in a higher prio repo entirely from lower prio repos

src/Composer/DependencyResolver/DefaultPolicy.php

@@ -69,7 +69,6 @@ class DefaultPolicy implements PolicyInterface
         }
 
         foreach ($packages as &$sortedLiterals) {
-            $sortedLiterals = $this->pruneToHighestPriority($pool, $sortedLiterals);
             $sortedLiterals = $this->pruneToBestVersion($pool, $sortedLiterals);
             $sortedLiterals = $this->pruneRemoteAliases($pool, $sortedLiterals);
         }
@@ -104,51 +103,47 @@ class DefaultPolicy implements PolicyInterface
      */
     public function compareByPriority(Pool $pool, PackageInterface $a, PackageInterface $b, $requiredPackage = null, $ignoreReplace = false)
     {
-        if ($a->getRepository() === $b->getRepository()) {
-            // prefer aliases to the original package
-            if ($a->getName() === $b->getName()) {
-                $aAliased = $a instanceof AliasPackage;
-                $bAliased = $b instanceof AliasPackage;
-                if ($aAliased && !$bAliased) {
-                    return -1; // use a
-                }
-                if (!$aAliased && $bAliased) {
-                    return 1; // use b
-                }
+        // prefer aliases to the original package
+        if ($a->getName() === $b->getName()) {
+            $aAliased = $a instanceof AliasPackage;
+            $bAliased = $b instanceof AliasPackage;
+            if ($aAliased && !$bAliased) {
+                return -1; // use a
             }
+            if (!$aAliased && $bAliased) {
+                return 1; // use b
+            }
+        }
 
-            if (!$ignoreReplace) {
-                // return original, not replaced
-                if ($this->replaces($a, $b)) {
-                    return 1; // use b
-                }
-                if ($this->replaces($b, $a)) {
-                    return -1; // use a
-                }
+        if (!$ignoreReplace) {
+            // return original, not replaced
+            if ($this->replaces($a, $b)) {
+                return 1; // use b
+            }
+            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 ($requiredPackage && false !== ($pos = strpos($requiredPackage, '/'))) {
-                    $requiredVendor = substr($requiredPackage, 0, $pos);
+            // for replacers not replacing each other, put a higher prio on replacing
+            // packages with the same vendor as the required package
+            if ($requiredPackage && false !== ($pos = strpos($requiredPackage, '/'))) {
+                $requiredVendor = substr($requiredPackage, 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;
                 }
             }
+        }
 
-            // priority equal, sort by package id to make reproducible
-            if ($a->id === $b->id) {
-                return 0;
-            }
-
-            return ($a->id < $b->id) ? -1 : 1;
+        // priority equal, sort by package id to make reproducible
+        if ($a->id === $b->id) {
+            return 0;
         }
 
-        return ($pool->getPriority($a->id) > $pool->getPriority($b->id)) ? -1 : 1;
+        return ($a->id < $b->id) ? -1 : 1;
     }
 
     /**
@@ -198,32 +193,6 @@ class DefaultPolicy implements PolicyInterface
         return $bestLiterals;
     }
 
-    /**
-     * Assumes that highest priority packages come first
-     */
-    protected function pruneToHighestPriority(Pool $pool, array $literals)
-    {
-        $selected = array();
-
-        $priority = null;
-
-        foreach ($literals as $literal) {
-            $package = $pool->literalToPackage($literal);
-
-            if (null === $priority) {
-                $priority = $pool->getPriority($package->id);
-            }
-
-            if ($pool->getPriority($package->id) != $priority) {
-                break;
-            }
-
-            $selected[] = $literal;
-        }
-
-        return $selected;
-    }
-
     /**
      * Assumes that locally aliased (in root package requires) packages take priority over branch-alias ones
      *

src/Composer/DependencyResolver/Pool.php

@@ -36,7 +36,6 @@ class Pool implements \Countable
     protected $packages = array();
     protected $packageByName = array();
     protected $packageByExactName = array();
-    protected $priorities = array();
     protected $versionParser;
     protected $providerCache = array();
 
@@ -45,13 +44,12 @@ class Pool implements \Countable
         $this->versionParser = new VersionParser;
     }
 
-    public function setPackages(array $packages, array $priorities = array())
+    public function setPackages(array $packages)
     {
         $id = 1;
 
         foreach ($packages as $i => $package) {
             $this->packages[] = $package;
-            $this->priorities[] = isset($priorities[$i]) ? $priorities[$i] : 0;
 
             $package->id = $id++;
             $names = $package->getNames();
@@ -63,11 +61,6 @@ class Pool implements \Countable
         }
     }
 
-    public function getPriority($id)
-    {
-        return $this->priorities[$id - 1];
-    }
-
     /**
      * Retrieves the package object for a given package id.
      *

src/Composer/DependencyResolver/PoolBuilder.php

@@ -38,7 +38,6 @@ class PoolBuilder
     private $loadedNames = array();
 
     private $packages = array();
-    private $priorities = array();
 
     public function __construct($isPackageAcceptableCallable, array $rootRequires = array())
     {
@@ -97,9 +96,14 @@ class PoolBuilder
                 }
 
                 // TODO should we really pass the callable into here?
-                $packages = $repository->loadPackages($loadNames, $this->isPackageAcceptableCallable);
+                $result = $repository->loadPackages($loadNames, $this->isPackageAcceptableCallable);
+
+                foreach ($result['namesFound'] as $name) {
+                    // avoid loading the same package again from other repositories once it has been found
+                    unset($loadNames[$name]);
+                }
+                foreach ($result['packages'] as $package) {
 
-                foreach ($packages as $package) {
                     if (call_user_func($this->isPackageAcceptableCallable, $package->getNames(), $package->getStability())) {
                         $newLoadNames += $this->loadPackage($request, $package, $key);
                     }
@@ -129,7 +133,6 @@ class PoolBuilder
                 if (!$found) {
                     foreach ($aliasedPackages as $index => $packageOrAlias) {
                         unset($this->packages[$index]);
-                        unset($this->priorities[$index]);
                     }
                 }
             }
@@ -144,7 +147,7 @@ class PoolBuilder
             }
         }
 
-        $pool->setPackages($this->packages, $this->priorities);
+        $pool->setPackages($this->packages);
 
         unset($this->aliasMap);
         unset($this->loadedNames);
@@ -153,11 +156,10 @@ class PoolBuilder
         return $pool;
     }
 
-    private function loadPackage(Request $request, PackageInterface $package, $repoIndex)
+    private function loadPackage(Request $request, PackageInterface $package)
     {
         $index = count($this->packages);
         $this->packages[] = $package;
-        $this->priorities[] = -$repoIndex;
 
         if ($package instanceof AliasPackage) {
             $this->aliasMap[spl_object_hash($package->getAliasOf())][$index] = $package;
@@ -187,7 +189,6 @@ class PoolBuilder
 
             $package->getRepository()->addPackage($aliasPackage); // TODO do we need this?
             $this->packages[] = $aliasPackage;
-            $this->priorities[] = -$repoIndex;
             $this->aliasMap[spl_object_hash($aliasPackage->getAliasOf())][$index+1] = $aliasPackage;
         }
 

src/Composer/Repository/ArrayRepository.php

@@ -28,8 +28,8 @@ class ArrayRepository extends BaseRepository
 {
     /** @var PackageInterface[] */
     protected $packages;
-    
-    /** 
+
+    /**
       * @var PackageInterface[] indexed by package unique name and used to cache hasPackage calls
       */
     protected $packageMap;
@@ -41,6 +41,42 @@ class ArrayRepository extends BaseRepository
         }
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    public function loadPackages(array $packageMap, $isPackageAcceptableCallable)
+    {
+        $packages = $this->getPackages();
+
+        $result = array();
+        $namesFound = array();
+        foreach ($packages as $package) {
+            if (array_key_exists($package->getName(), $packageMap)) {
+                if (
+                    (!$packageMap[$package->getName()] || $packageMap[$package->getName()]->matches(new Constraint('==', $package->getVersion())))
+                    && call_user_func($isPackageAcceptableCallable, $package->getNames(), $package->getStability())
+                ) {
+                    $result[spl_object_hash($package)] = $package;
+                    if ($package instanceof AliasPackage && !isset($result[spl_object_hash($package->getAliasOf())])) {
+                        $result[spl_object_hash($package->getAliasOf())] = $package->getAliasOf();
+                    }
+                }
+
+                $namesFound[$package->getName()] = true;
+            }
+        }
+
+        foreach ($packages as $package) {
+            if ($package instanceof AliasPackage) {
+                if (isset($result[spl_object_hash($package->getAliasOf())])) {
+                    $result[spl_object_hash($package)] = $package;
+                }
+            }
+        }
+
+        return array('namesFound' => array_keys($namesFound), 'packages' => $result);
+    }
+
     /**
      * {@inheritDoc}
      */

src/Composer/Repository/BaseRepository.php

@@ -25,36 +25,6 @@ use Composer\Package\Link;
  */
 abstract class BaseRepository implements RepositoryInterface
 {
-    // TODO should this stay here? some repos need a better implementation
-    public function loadPackages(array $packageMap, $isPackageAcceptableCallable)
-    {
-        $packages = $this->getPackages();
-
-        $result = array();
-        foreach ($packages as $package) {
-            if (
-                array_key_exists($package->getName(), $packageMap)
-                && (!$packageMap[$package->getName()] || $packageMap[$package->getName()]->matches(new Constraint('==', $package->getVersion())))
-                && call_user_func($isPackageAcceptableCallable, $package->getNames(), $package->getStability())
-            ) {
-                $result[spl_object_hash($package)] = $package;
-                if ($package instanceof AliasPackage && !isset($result[spl_object_hash($package->getAliasOf())])) {
-                    $result[spl_object_hash($package->getAliasOf())] = $package->getAliasOf();
-                }
-            }
-        }
-
-        foreach ($packages as $package) {
-            if ($package instanceof AliasPackage) {
-                if (isset($result[spl_object_hash($package->getAliasOf())])) {
-                    $result[spl_object_hash($package)] = $package;
-                }
-            }
-        }
-
-        return $result;
-    }
-
     /**
      * Returns a list of links causing the requested needle packages to be installed, as an associative array with the
      * dependent's name as key, and an array containing in order the PackageInterface and Link describing the relationship

src/Composer/Repository/ComposerRepository.php

@@ -144,7 +144,7 @@ class ComposerRepository extends ArrayRepository implements ConfigurableReposito
 
             $packages = $this->loadAsyncPackages(array($name => $constraint));
 
-            return reset($packages);
+            return reset($packages['packages']);
         }
 
         if ($hasProviders) {
@@ -182,7 +182,9 @@ class ComposerRepository extends ArrayRepository implements ConfigurableReposito
                 return array();
             }
 
-            return $this->loadAsyncPackages(array($name => $constraint));
+            $result = $this->loadAsyncPackages(array($name => $constraint));
+
+            return $result['packages'];
         }
 
         if ($hasProviders) {
@@ -240,7 +242,9 @@ class ComposerRepository extends ArrayRepository implements ConfigurableReposito
                     $packageMap[$name] = new EmptyConstraint();
                 }
 
-                return array_values($this->loadAsyncPackages($packageMap));
+                $result = $this->loadAsyncPackages($packageMap);
+
+                return array_values($result['packages']);
             }
 
             if ($this->hasPartialPackages()) {
@@ -298,6 +302,7 @@ class ComposerRepository extends ArrayRepository implements ConfigurableReposito
         }
 
         $packages = array();
+        $namesFound = array();
 
         if ($hasProviders || $this->hasPartialPackages()) {
             foreach ($packageNameMap as $name => $constraint) {
@@ -314,6 +319,7 @@ class ComposerRepository extends ArrayRepository implements ConfigurableReposito
                     if ($candidate->getName() !== $name) {
                         throw new \LogicException('whatProvides should never return a package with a different name than the requested one');
                     }
+                    $namesFound[$name] = true;
                     if (!$constraint || $constraint->matches(new Constraint('==', $candidate->getVersion()))) {
                         $matches[spl_object_hash($candidate)] = $candidate;
                         if ($candidate instanceof AliasPackage && !isset($matches[spl_object_hash($candidate->getAliasOf())])) {
@@ -344,10 +350,12 @@ class ComposerRepository extends ArrayRepository implements ConfigurableReposito
                 }, ARRAY_FILTER_USE_KEY);
             }
 
-            $packages = array_merge($packages, $this->loadAsyncPackages($packageNameMap, $isPackageAcceptableCallable));
+            $result = $this->loadAsyncPackages($packageNameMap, $isPackageAcceptableCallable);
+            $packages = array_merge($packages, $result['packages']);
+            $namesFound = array_merge($namesFound, $result['namesFound']);
         }
 
-        return $packages;
+        return array('namesFound' => array_keys($namesFound), 'packages' => $packages);
     }
 
     /**
@@ -587,6 +595,7 @@ class ComposerRepository extends ArrayRepository implements ConfigurableReposito
         $this->loadRootServerFile();
 
         $packages = array();
+        $namesFound = array();
         $promises = array();
         $repo = $this;
 
@@ -620,7 +629,7 @@ class ComposerRepository extends ArrayRepository implements ConfigurableReposito
             }
 
             $promises[] = $this->asyncFetchFile($url, $cacheKey, $lastModified)
-                ->then(function ($response) use (&$packages, $contents, $realName, $constraint, $repo, $isPackageAcceptableCallable) {
+                ->then(function ($response) use (&$packages, &$namesFound, $contents, $realName, $constraint, $repo, $isPackageAcceptableCallable) {
                     if (true === $response) {
                         $response = $contents;
                     }
@@ -635,6 +644,7 @@ class ComposerRepository extends ArrayRepository implements ConfigurableReposito
                         $versions = MetadataMinifier::expand($versions);
                     }
 
+                    $namesFound[$realName] = true;
                     $versionsToLoad = array();
                     foreach ($versions as $version) {
                         if (!isset($version['version_normalized'])) {
@@ -661,7 +671,7 @@ class ComposerRepository extends ArrayRepository implements ConfigurableReposito
 
         $this->loop->wait($promises);
 
-        return $packages;
+        return array('namesFound' => $namesFound, 'packages' => $packages);
         // RepositorySet should call loadMetadata, getMetadata when all promises resolved, then metadataComplete when done so we can GC the loaded json and whatnot then as needed
     }
 

src/Composer/Repository/CompositeRepository.php

@@ -94,6 +94,26 @@ class CompositeRepository extends BaseRepository
         return $packages ? call_user_func_array('array_merge', $packages) : array();
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    public function loadPackages(array $packageMap, $isPackageAcceptableCallable)
+    {
+        $packages = array();
+        $namesFound = array();
+        foreach ($this->repositories as $repository) {
+            /* @var $repository RepositoryInterface */
+            $result = $repository->findPackages($name, $constraint);
+            $packages[] = $result['packages'];
+            $namesFound[] = $result['namesFound'];
+        }
+
+        return array(
+            'packages' => $packages ? call_user_func_array('array_merge', $packages) : array(),
+            'namesFound' => $namesFound ? array_unique(call_user_func_array('array_merge', $namesFound)) : array(),
+        );
+    }
+
     /**
      * {@inheritdoc}
      */

src/Composer/Repository/RepositoryInterface.php

@@ -69,7 +69,7 @@ interface RepositoryInterface extends \Countable
      *
      * @param ConstraintInterface[] $packageNameMap package names pointing to constraints
      * @param $isPackageAcceptableCallable
-     * @return PackageInterface[]
+     * @return array [namesFound => string[], packages => PackageInterface[]]
      */
     public function loadPackages(array $packageNameMap, $isPackageAcceptableCallable);
 

src/Composer/Repository/RepositorySet.php

@@ -65,6 +65,9 @@ class RepositorySet
     /**
      * Adds a repository to this repository set
      *
+     * The first repos added have a higher priority. As soon as a package is found in any
+     * repository the search for that package ends, and following repos will not be consulted.
+     *
      * @param RepositoryInterface $repo        A package repository
      */
     public function addRepository(RepositoryInterface $repo)
@@ -134,17 +137,6 @@ class RepositorySet
         return $candidates;
     }
 
-    public function getPriority(RepositoryInterface $repo)
-    {
-        $priority = array_search($repo, $this->repositories, true);
-
-        if (false === $priority) {
-            throw new \RuntimeException("Could not determine repository priority. The repository was not registered in the pool.");
-        }
-
-        return -$priority;
-    }
-
     /**
      * Create a pool for dependency resolution from the packages in this repository set.
      *

tests/Composer/Test/DependencyResolver/DefaultPolicyTest.php

@@ -13,6 +13,7 @@
 namespace Composer\Test\DependencyResolver;
 
 use Composer\Repository\ArrayRepository;
+use Composer\Repository\LockArrayRepository;
 use Composer\Repository\RepositoryInterface;
 use Composer\DependencyResolver\DefaultPolicy;
 use Composer\DependencyResolver\Pool;
@@ -28,7 +29,7 @@ class DefaultPolicyTest extends TestCase
     protected $repositorySet;
     /** @var ArrayRepository */
     protected $repo;
-    /** @var ArrayRepository */
+    /** @var LockArrayRepository */
     protected $repoLocked;
     /** @var DefaultPolicy */
     protected $policy;
@@ -37,7 +38,7 @@ class DefaultPolicyTest extends TestCase
     {
         $this->repositorySet = new RepositorySet(array(), array(), 'dev');
         $this->repo = new ArrayRepository;
-        $this->repoLocked = new ArrayRepository;
+        $this->repoLocked = new LockArrayRepository;
 
         $this->policy = new DefaultPolicy;
     }
@@ -122,44 +123,6 @@ class DefaultPolicyTest extends TestCase
         $this->assertSame($expected, $selected);
     }
 
-    public function testSelectNewestOverLocked()
-    {
-        $this->repo->addPackage($packageA = $this->getPackage('A', '2.0'));
-        $this->repoLocked->addPackage($packageAInstalled = $this->getPackage('A', '1.0'));
-        $this->repositorySet->addRepository($this->repo);
-        $this->repositorySet->addRepository($this->repoLocked);
-
-        $pool = $this->repositorySet->createPoolForPackage('A');
-
-        $literals = array($packageA->getId(), $packageAInstalled->getId());
-        $expected = array($packageA->getId());
-
-        $selected = $this->policy->selectPreferredPackages($pool, $literals);
-
-        $this->assertSame($expected, $selected);
-    }
-
-    public function testSelectFirstRepo()
-    {
-        $otherRepository = new ArrayRepository;
-
-        $this->repo->addPackage($packageA = $this->getPackage('A', '1.0'));
-        $otherRepository->addPackage($packageAImportant = $this->getPackage('A', '1.0'));
-
-        $this->repositorySet->addRepository($otherRepository);
-        $this->repositorySet->addRepository($this->repo);
-        $this->repositorySet->addRepository($this->repoLocked);
-
-        $pool = $this->repositorySet->createPoolForPackage('A');
-
-        $literals = array($packageA->getId(), $packageAImportant->getId());
-        $expected = array($packageAImportant->getId());
-
-        $selected = $this->policy->selectPreferredPackages($pool, $literals);
-
-        $this->assertSame($expected, $selected);
-    }
-
     public function testRepositoryOrderingAffectsPriority()
     {
         $repo1 = new ArrayRepository;

tests/Composer/Test/Fixtures/installer/repositories-priorities.test

@@ -0,0 +1,42 @@
+--TEST--
+Packages found in a higher priority repository take precedence even if they are not found in the requested version
+--COMPOSER--
+{
+    "repositories": [
+        {
+            "type": "package",
+            "package": [
+                { "name": "foo/a", "version": "1.0.0" }
+            ]
+        },
+        {
+            "type": "package",
+            "package": [
+                { "name": "foo/a", "version": "2.0.0" }
+            ]
+        }
+    ],
+    "require": {
+        "foo/a": "2.*"
+    }
+}
+--RUN--
+update
+--EXPECT-OUTPUT--
+Loading composer repositories with package information
+Updating dependencies
+Your requirements could not be resolved to an installable set of packages.
+
+  Problem 1
+    - The requested package foo/a could not be found in any version, there may be a typo in the package name.
+
+Potential causes:
+ - A typo in the package name
+ - The package is not available in a stable-enough version according to your minimum-stability setting
+   see <https://getcomposer.org/doc/04-schema.md#minimum-stability> for more details.
+ - It's a private package and you forgot to add a custom repository to find it
+
+Read <https://getcomposer.org/doc/articles/troubleshooting.md> for further common problems.
+--EXPECT--
+--EXPECT-EXIT-CODE--
+2

tests/Composer/Test/Fixtures/installer/repositories-priorities2.test

@@ -0,0 +1,26 @@
+--TEST--
+Packages found in a higher priority repository take precedence
+--COMPOSER--
+{
+    "repositories": [
+        {
+            "type": "package",
+            "package": [
+                { "name": "foo/a", "version": "1.0.0" }
+            ]
+        },
+        {
+            "type": "package",
+            "package": [
+                { "name": "foo/a", "version": "1.1.0" }
+            ]
+        }
+    ],
+    "require": {
+        "foo/a": "1.*"
+    }
+}
+--RUN--
+update
+--EXPECT--
+Installing foo/a (1.0.0)