Correctly sort operations within transactions using DFS

Fixes #655

src/Composer/DependencyResolver/Solver.php

@@ -171,6 +171,13 @@ class Solver
 
         $this->runSat(true);
 
+        // decide to remove everything that's installed and undecided
+        foreach ($this->installedMap as $packageId => $void) {
+            if ($this->decisions->undecided($packageId)) {
+                $this->decisions->decide(-$packageId, 1, null);
+            }
+        }
+
         if ($this->problems) {
             throw new SolverProblemsException($this->problems);
         }

src/Composer/DependencyResolver/Transaction.php

@@ -37,14 +37,148 @@ class Transaction
 
     public function getOperations()
     {
-        $installMeansUpdateMap = array();
+        $installMeansUpdateMap = $this->findUpdates();
 
-        foreach ($this->installedMap as $packageId => $void) {
+        $updateMap = array();
-            if ($this->decisions->undecided($packageId)) {
+        $installMap = array();
-                $this->decisions->decide(-$packageId, 1, null);
+        $uninstallMap = array();
+
+        foreach ($this->decisions as $i => $decision) {
+            $literal = $decision[Decisions::DECISION_LITERAL];
+            $reason = $decision[Decisions::DECISION_REASON];
+
+            $package = $this->pool->literalToPackage($literal);
+
+            // wanted & installed || !wanted & !installed
+            if (($literal > 0) == (isset($this->installedMap[$package->getId()]))) {
+                continue;
+            }
+
+            if ($literal > 0) {
+                if (isset($installMeansUpdateMap[abs($literal)]) && !$package instanceof AliasPackage) {
+
+                    $source = $installMeansUpdateMap[abs($literal)];
+
+                    $updateMap[$package->getId()] = array(
+                        'package' => $package,
+                        'source' => $source,
+                        'reason' => $reason,
+                    );
+
+                    // avoid updates to one package from multiple origins
+                    unset($installMeansUpdateMap[abs($literal)]);
+                    $ignoreRemove[$source->getId()] = true;
+                } else {
+                    $installMap[$package->getId()] = array(
+                        'package' => $package,
+                        'reason' => $reason,
+                    );
+                }
             }
         }
 
+        foreach ($this->decisions as $i => $decision) {
+            $literal = $decision[Decisions::DECISION_LITERAL];
+            $package = $this->pool->literalToPackage($literal);
+
+            if ($literal <= 0 &&
+                isset($this->installedMap[$package->getId()]) &&
+                !isset($ignoreRemove[$package->getId()])) {
+                $uninstallMap[$package->getId()] = array(
+                    'package' => $package,
+                    'reason' => $reason,
+                );
+
+            }
+        }
+
+        $this->transactionFromMaps($installMap, $updateMap, $uninstallMap);
+
+        return $this->transaction;
+    }
+
+    protected function transactionFromMaps($installMap, $updateMap, $uninstallMap)
+    {
+        $queue = array_map(function ($operation) {
+                return $operation['package'];
+            },
+            $this->findRootPackages($installMap, $updateMap)
+        );
+
+        $visited = array();
+
+        while (!empty($queue)) {
+            $package = array_pop($queue);
+            $packageId = $package->getId();
+
+            if (!isset($visited[$packageId])) {
+                array_push($queue, $package);
+
+                if ($package instanceof AliasPackage) {
+                    array_push($queue, $package->getAliasOf());
+                } else {
+                    foreach ($package->getRequires() as $link) {
+                        $possibleRequires = $this->pool->whatProvides($link->getTarget(), $link->getConstraint());
+
+                        foreach ($possibleRequires as $require) {
+                            array_push($queue, $require);
+                        }
+                    }
+                }
+
+                $visited[$package->getId()] = true;
+            } else {
+                if (isset($installMap[$packageId])) {
+                    $this->install(
+                        $installMap[$packageId]['package'],
+                        $installMap[$packageId]['reason']
+                    );
+                    unset($installMap[$packageId]);
+                }
+                if (isset($updateMap[$packageId])) {
+                    $this->update(
+                        $updateMap[$packageId]['source'],
+                        $updateMap[$packageId]['package'],
+                        $updateMap[$packageId]['reason']
+                    );
+                    unset($updateMap[$packageId]);
+                }
+            }
+        }
+
+        foreach ($uninstallMap as $uninstall) {
+            $this->uninstall($uninstall['package'], $uninstall['reason']);
+        }
+    }
+
+    protected function findRootPackages($installMap, $updateMap)
+    {
+        $packages = $installMap + $updateMap;
+        $roots = $packages;
+
+        foreach ($packages as $packageId => $operation) {
+            $package = $operation['package'];
+
+            if (!isset($roots[$packageId])) {
+                continue;
+            }
+
+            foreach ($package->getRequires() as $link) {
+                $possibleRequires = $this->pool->whatProvides($link->getTarget(), $link->getConstraint());
+
+                foreach ($possibleRequires as $require) {
+                    unset($roots[$require->getId()]);
+                }
+            }
+        }
+
+        return $roots;
+    }
+
+    protected function findUpdates()
+    {
+        $installMeansUpdateMap = array();
+
         foreach ($this->decisions as $i => $decision) {
             $literal = $decision[Decisions::DECISION_LITERAL];
             $package = $this->pool->literalToPackage($literal);
@@ -67,51 +201,7 @@ class Transaction
             }
         }
 
-        foreach ($this->decisions as $i => $decision) {
+        return $installMeansUpdateMap;
-            $literal = $decision[Decisions::DECISION_LITERAL];
-            $package = $this->pool->literalToPackage($literal);
-
-            // wanted & installed || !wanted & !installed
-            if (($literal > 0) == (isset($this->installedMap[$package->getId()]))) {
-                continue;
-            }
-
-            if ($literal > 0) {
-                if ($package instanceof AliasPackage) {
-                    $this->markAliasInstalled($package, $decision[Decisions::DECISION_REASON]);
-                    continue;
-                }
-
-                if (isset($installMeansUpdateMap[abs($literal)])) {
-
-                    $source = $installMeansUpdateMap[abs($literal)];
-
-                    $this->update($source, $package, $decision[Decisions::DECISION_REASON]);
-
-                    // avoid updates to one package from multiple origins
-                    unset($installMeansUpdateMap[abs($literal)]);
-                    $ignoreRemove[$source->getId()] = true;
-                } else {
-                    $this->install($package, $decision[Decisions::DECISION_REASON]);
-                }
-            }
-        }
-
-        foreach ($this->decisions as $i => $decision) {
-            $literal = $decision[Decisions::DECISION_LITERAL];
-            $package = $this->pool->literalToPackage($literal);
-
-            // wanted & installed || !wanted & !installed
-            if (($literal > 0) == (isset($this->installedMap[$package->getId()]))) {
-                continue;
-            }
-
-            if ($literal <= 0 && !isset($ignoreRemove[$package->getId()])) {
-                $this->uninstall($package, $decision[Decisions::DECISION_REASON]);
-            }
-        }
-
-        return $this->transaction;
     }
 
     protected function install($package, $reason)

tests/Composer/Test/DependencyResolver/SolverTest.php

@@ -505,8 +505,8 @@ class SolverTest extends TestCase
         $this->request->install('X');
 
         $this->checkSolverResult(array(
-            array('job' => 'install', 'package' => $packageA),
             array('job' => 'install', 'package' => $newPackageB),
+            array('job' => 'install', 'package' => $packageA),
             array('job' => 'install', 'package' => $packageX),
         ));
     }
@@ -548,9 +548,9 @@ class SolverTest extends TestCase
         $this->request->install('A');
 
         $this->checkSolverResult(array(
-            array('job' => 'install', 'package' => $packageC),
             array('job' => 'install', 'package' => $packageB),
             array('job' => 'install', 'package' => $packageA),
+            array('job' => 'install', 'package' => $packageC),
         ));
     }
 
@@ -718,8 +718,8 @@ class SolverTest extends TestCase
         $this->request->install('A', $this->getVersionConstraint('==', '1.1.0.0'));
 
         $this->checkSolverResult(array(
-            array('job' => 'install', 'package' => $packageB),
             array('job' => 'install', 'package' => $packageA2),
+            array('job' => 'install', 'package' => $packageB),
             array('job' => 'install', 'package' => $packageA2Alias),
         ));
     }
@@ -741,9 +741,9 @@ class SolverTest extends TestCase
         $this->request->install('B');
 
         $this->checkSolverResult(array(
+            array('job' => 'install', 'package' => $packageA),
             array('job' => 'install', 'package' => $packageAAlias),
             array('job' => 'install', 'package' => $packageB),
-            array('job' => 'install', 'package' => $packageA),
         ));
     }
 

tests/Composer/Test/Fixtures/installer/aliased-priority-conflicting.test

@@ -45,7 +45,7 @@ Aliases take precedence over default package even if default is selected
 --RUN--
 install
 --EXPECT--
-Marking a/req (dev-master feat.f) as installed, alias of a/req (dev-feature-foo feat.f)
 Installing a/req (dev-feature-foo feat.f)
-Installing a/b (dev-master)
+Marking a/req (dev-master feat.f) as installed, alias of a/req (dev-feature-foo feat.f)
 Installing a/a (dev-master)
+Installing a/b (dev-master)

tests/Composer/Test/Fixtures/installer/aliased-priority.test

@@ -47,9 +47,9 @@ Aliases take precedence over default package
 --RUN--
 install
 --EXPECT--
-Marking a/b (1.0.x-dev forked) as installed, alias of a/b (dev-master forked)
-Installing a/b (dev-master forked)
-Marking a/c (dev-master feat.f) as installed, alias of a/c (dev-feature-foo feat.f)
-Installing a/a (dev-master master)
 Installing a/c (dev-feature-foo feat.f)
+Marking a/c (dev-master feat.f) as installed, alias of a/c (dev-feature-foo feat.f)
+Installing a/b (dev-master forked)
+Installing a/a (dev-master master)
 Marking a/a (1.0.x-dev master) as installed, alias of a/a (dev-master master)
+Marking a/b (1.0.x-dev forked) as installed, alias of a/b (dev-master forked)