Make problem report messages more readable

Added pretty strings to constraints

src/Composer/DependencyResolver/Pool.php

@@ -163,4 +163,17 @@ class Pool
     {
         return ($literal > 0 ? '+' : '-') . $this->literalToPackage($literal);
     }
+
+    public function literalToPrettyString($literal, $installedMap)
+    {
+        $package = $this->literalToPackage($literal);
+
+        if (isset($installedMap[$package->getId()])) {
+            $prefix = ($literal > 0 ? 'keep' : 'remove');
+        } else {
+            $prefix = ($literal > 0 ? 'install' : 'don\'t install');
+        }
+
+        return $prefix.' '.$package->getPrettyString();
+    }
 }

src/Composer/DependencyResolver/Problem.php

@@ -19,11 +19,19 @@ namespace Composer\DependencyResolver;
  */
 class Problem
 {
+    /**
+     * A map containing the id of each rule part of this problem as a key
+     * @var array
+     */
+    protected $reasonSeen;
+
     /**
      * A set of reasons for the problem, each is a rule or a job and a rule
      * @var array
      */
-    protected $reasons;
+    protected $reasons = array();
+
+    protected $section = 0;
 
     /**
      * Add a rule as a reason
@@ -50,12 +58,16 @@ class Problem
 
     /**
      * A human readable textual representation of the problem's reasons
+     *
+     * @param array $installedMap A map of all installed packages
      */
-    public function __toString()
+    public function getPrettyString(array $installedMap = array())
     {
-        if (count($this->reasons) === 1) {
-            reset($this->reasons);
-            $reason = current($this->reasons);
+        $reasons = call_user_func_array('array_merge', array_reverse($this->reasons));
+
+        if (count($reasons) === 1) {
+            reset($reasons);
+            $reason = current($reasons);
 
             $rule = $reason['rule'];
             $job = $reason['job'];
@@ -73,9 +85,9 @@ class Problem
             }
         }
 
-        $messages = array("Problem caused by:");
+        $messages = array();
 
-        foreach ($this->reasons as $reason) {
+        foreach ($reasons as $reason) {
 
             $rule = $reason['rule'];
             $job = $reason['job'];
@@ -84,12 +96,12 @@ class Problem
                 $messages[] = $this->jobToText($job);
             } elseif ($rule) {
                 if ($rule instanceof Rule) {
-                    $messages[] = $rule->toHumanReadableString();
+                    $messages[] = $rule->getPrettyString($installedMap);
                 }
             }
         }
 
-        return implode("\n\t\t\t- ", $messages);
+        return "\n    - ".implode("\n    - ", $messages);
     }
 
     /**
@@ -100,11 +112,17 @@ class Problem
      */
     protected function addReason($id, $reason)
     {
-        if (!isset($this->reasons[$id])) {
-            $this->reasons[$id] = $reason;
+        if (!isset($this->reasonSeen[$id])) {
+            $this->reasonSeen[$id] = true;
+            $this->reasons[$this->section][] = $reason;
         }
     }
 
+    public function nextSection()
+    {
+        $this->section++;
+    }
+
     /**
      * Turns a job into a human readable description
      *
@@ -119,7 +137,7 @@ class Problem
                     return 'No package found to satisfy install request for '.$job['packageName'].$this->constraintToText($job['constraint']);
                 }
 
-                return 'Installation request for '.$job['packageName'].$this->constraintToText($job['constraint']).': Satisfiable by ['.$this->getPackageList($job['packages']).'].';
+                return 'Installation request for '.$job['packageName'].$this->constraintToText($job['constraint']).' -> satisfiable by '.$this->getPackageList($job['packages']).'.';
             case 'update':
                 return 'Update request for '.$job['packageName'].$this->constraintToText($job['constraint']).'.';
             case 'remove':
@@ -146,6 +164,6 @@ class Problem
      */
     protected function constraintToText($constraint)
     {
-        return ($constraint) ? ' '.$constraint : '';
+        return ($constraint) ? ' '.$constraint->getPrettyString() : '';
     }
 }

src/Composer/DependencyResolver/Rule.php

@@ -147,14 +147,14 @@ class Rule
         return 1 === count($this->literals);
     }
 
-    public function toHumanReadableString()
+    public function getPrettyString(array $installedMap = array())
     {
         $ruleText = '';
         foreach ($this->literals as $i => $literal) {
             if ($i != 0) {
                 $ruleText .= '|';
             }
-            $ruleText .= $this->pool->literalToString($literal);
+            $ruleText .= $this->pool->literalToPrettyString($literal, $installedMap);
         }
 
         switch ($this->reason) {
@@ -171,7 +171,7 @@ class Rule
                 $package1 = $this->pool->literalToPackage($this->literals[0]);
                 $package2 = $this->pool->literalToPackage($this->literals[1]);
 
-                return 'Package '.$package1->getPrettyString().' conflicts with '.$package2->getPrettyString().'"';
+                return $package1->getPrettyString().' conflicts with '.$package2->getPrettyString().'.';
 
             case self::RULE_PACKAGE_REQUIRES:
                 $literals = $this->literals;
@@ -183,11 +183,15 @@ class Rule
                     $requires[] = $this->pool->literalToPackage($literal);
                 }
 
-                $text = 'Package "'.$sourcePackage.'" contains the rule '.$this->reasonData.'. ';
+                $text = $this->reasonData->getPrettyString($sourcePackage);
                 if ($requires) {
-                    $text .= 'Any of these packages satisfy the dependency: '.implode(', ', $requires).'.';
+                    $requireText = array();
+                    foreach ($requires as $require) {
+                        $requireText[] = $require->getPrettyString();
+                    }
+                    $text .= ' -> satisfiable by '.implode(', ', $requireText).'.';
                 } else {
-                    $text .= 'No package satisfies this dependency.';
+                    $text .= ' -> no matching package found.';
                 }
 
                 return $text;
@@ -197,11 +201,18 @@ class Rule
             case self::RULE_INSTALLED_PACKAGE_OBSOLETES:
                 return $ruleText;
             case self::RULE_PACKAGE_SAME_NAME:
-                return $ruleText;
+                $text = "Can only install one of: ";
+
+                $packages = array();
+                foreach ($this->literals as $i => $literal) {
+                    $packages[] = $this->pool->literalToPackage($literal)->getPrettyString();
+                }
+
+                return $text.implode(', ', $packages).'.';
             case self::RULE_PACKAGE_IMPLICIT_OBSOLETES:
                 return $ruleText;
             case self::RULE_LEARNED:
-                return 'learned: '.$ruleText;
+                return 'Conclusion: '.$ruleText;
             case self::RULE_PACKAGE_ALIAS:
                 return $ruleText;
         }

src/Composer/DependencyResolver/RuleSetGenerator.php

@@ -157,7 +157,7 @@ class RuleSetGenerator
             foreach ($package->getRequires() as $link) {
                 $possibleRequires = $this->pool->whatProvides($link->getTarget(), $link->getConstraint());
 
-                $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createRequireRule($package, $possibleRequires, Rule::RULE_PACKAGE_REQUIRES, (string) $link));
+                $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createRequireRule($package, $possibleRequires, Rule::RULE_PACKAGE_REQUIRES, $link));
 
                 foreach ($possibleRequires as $require) {
                     $workQueue->enqueue($require);
@@ -168,7 +168,7 @@ class RuleSetGenerator
                 $possibleConflicts = $this->pool->whatProvides($link->getTarget(), $link->getConstraint());
 
                 foreach ($possibleConflicts as $conflict) {
-                    $this->addRule(RuleSet::TYPE_PACKAGE, $this->createConflictRule($package, $conflict, Rule::RULE_PACKAGE_CONFLICT, (string) $link));
+                    $this->addRule(RuleSet::TYPE_PACKAGE, $this->createConflictRule($package, $conflict, Rule::RULE_PACKAGE_CONFLICT, $link));
                 }
             }
 
@@ -185,7 +185,7 @@ class RuleSetGenerator
 
                     if (!$this->obsoleteImpossibleForAlias($package, $provider)) {
                         $reason = ($isInstalled) ? Rule::RULE_INSTALLED_PACKAGE_OBSOLETES : Rule::RULE_PACKAGE_OBSOLETES;
-                        $this->addRule(RuleSet::TYPE_PACKAGE, $this->createConflictRule($package, $provider, $reason, (string) $link));
+                        $this->addRule(RuleSet::TYPE_PACKAGE, $this->createConflictRule($package, $provider, $reason, $link));
                     }
                 }
             }
@@ -198,10 +198,10 @@ class RuleSetGenerator
                 }
 
                 if (($package instanceof AliasPackage) && $package->getAliasOf() === $provider) {
-                    $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createRequireRule($package, array($provider), Rule::RULE_PACKAGE_ALIAS, (string) $package));
+                    $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createRequireRule($package, array($provider), Rule::RULE_PACKAGE_ALIAS, $package));
                 } elseif (!$this->obsoleteImpossibleForAlias($package, $provider)) {
                     $reason = ($package->getName() == $provider->getName()) ? Rule::RULE_PACKAGE_SAME_NAME : Rule::RULE_PACKAGE_IMPLICIT_OBSOLETES;
-                    $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createConflictRule($package, $provider, $reason, (string) $package));
+                    $this->addRule(RuleSet::TYPE_PACKAGE, $rule = $this->createConflictRule($package, $provider, $reason, $package));
                 }
             }
         }

src/Composer/DependencyResolver/Solver.php

@@ -182,7 +182,7 @@ class Solver
         }
 
         if ($this->problems) {
-            throw new SolverProblemsException($this->problems);
+            throw new SolverProblemsException($this->problems, $this->installedMap);
         }
 
         $transaction = new Transaction($this->policy, $this->pool, $this->installedMap, $this->decisions);
@@ -457,6 +457,7 @@ class Solver
             return;
         }
 
+        $problem->nextSection();
         $problem->addRule($conflictRule);
     }
 

src/Composer/DependencyResolver/SolverProblemsException.php

@@ -18,23 +18,24 @@ namespace Composer\DependencyResolver;
 class SolverProblemsException extends \RuntimeException
 {
     protected $problems;
+    protected $installedMap;
 
-    public function __construct(array $problems)
+    public function __construct(array $problems, array $installedMap)
     {
         $this->problems = $problems;
+        $this->installedMap = $installedMap;
 
         parent::__construct($this->createMessage());
     }
 
     protected function createMessage()
     {
-        $messages = array();
-
-        foreach ($this->problems as $problem) {
-            $messages[] = (string) $problem;
+        $text = "\n";
+        foreach ($this->problems as $i => $problem) {
+            $text .= "  Problem ".($i+1).$problem->getPrettyString($this->installedMap)."\n";
         }
 
-        return "\n\tProblems:\n\t\t- ".implode("\n\t\t- ", $messages);
+        return $text;
     }
 
     public function getProblems()

src/Composer/Package/BasePackage.php

@@ -203,7 +203,7 @@ abstract class BasePackage implements PackageInterface
 
     public function getPrettyString()
     {
-        return $this->getPrettyName().'-'.$this->getPrettyVersion();
+        return $this->getPrettyName().' '.$this->getPrettyVersion();
     }
 
     public function __clone()

src/Composer/Package/Link.php

@@ -13,6 +13,7 @@
 namespace Composer\Package;
 
 use Composer\Package\LinkConstraint\LinkConstraintInterface;
+use Composer\Package\PackageInterface;
 
 /**
  * Represents a link between two packages, represented by their names
@@ -71,4 +72,9 @@ class Link
     {
         return $this->source.' '.$this->description.' '.$this->target.' ('.$this->constraint.')';
     }
+
+    public function getPrettyString(PackageInterface $sourcePackage)
+    {
+        return $sourcePackage->getPrettyString().' '.$this->description.' '.$this->target.' '.$this->constraint->getPrettyString().'';
+    }
 }

src/Composer/Package/LinkConstraint/LinkConstraintInterface.php

@@ -20,5 +20,7 @@ namespace Composer\Package\LinkConstraint;
 interface LinkConstraintInterface
 {
     public function matches(LinkConstraintInterface $provider);
+    public function setPrettyString($prettyString);
+    public function getPrettyString();
     public function __toString();
 }

src/Composer/Package/LinkConstraint/MultiConstraint.php

@@ -20,6 +20,7 @@ namespace Composer\Package\LinkConstraint;
 class MultiConstraint implements LinkConstraintInterface
 {
     protected $constraints;
+    protected $prettyString;
 
     /**
      * Sets operator and version to compare a package with
@@ -42,6 +43,19 @@ class MultiConstraint implements LinkConstraintInterface
         return true;
     }
 
+    public function setPrettyString($prettyString)
+    {
+        $this->prettyString = $prettyString;
+    }
+
+    public function getPrettyString()
+    {
+        if ($this->prettyString) {
+            return $this->prettyString;
+        }
+        return $this->__toString();
+    }
+
     public function __toString()
     {
         $constraints = array();

src/Composer/Package/LinkConstraint/SpecificConstraint.php

@@ -19,6 +19,8 @@ namespace Composer\Package\LinkConstraint;
  */
 abstract class SpecificConstraint implements LinkConstraintInterface
 {
+    protected $prettyString;
+
     public function matches(LinkConstraintInterface $provider)
     {
         if ($provider instanceof MultiConstraint) {
@@ -31,7 +33,21 @@ abstract class SpecificConstraint implements LinkConstraintInterface
         return true;
     }
 
+    public function setPrettyString($prettyString)
+    {
+        $this->prettyString = $prettyString;
+    }
+
+    public function getPrettyString()
+    {
+        if ($this->prettyString) {
+            return $this->prettyString;
+        }
+        return $this->__toString();
+    }
+
     // implementations must implement a method of this format:
     // not declared abstract here because type hinting violates parameter coherence (TODO right word?)
     // public function matchSpecific(<SpecificConstraintType> $provider);
+
 }

src/Composer/Package/Version/VersionParser.php

@@ -164,6 +164,8 @@ class VersionParser
      */
     public function parseConstraints($constraints)
     {
+        $prettyConstraint = $constraints;
+
         if (preg_match('{^([^,\s]*?)@('.implode('|', array_keys(BasePackage::$stabilities)).')$}i', $constraints, $match)) {
             $constraints = empty($match[1]) ? '*' : $match[1];
         }
@@ -184,10 +186,13 @@ class VersionParser
         }
 
         if (1 === count($constraintObjects)) {
-            return $constraintObjects[0];
+            $constraint = $constraintObjects[0];
+        } else {
+            $constraint = new MultiConstraint($constraintObjects);
         }
 
-        return new MultiConstraint($constraintObjects);
+        $constraint->setPrettyString($prettyConstraint);
+        return $constraint;
     }
 
     private function parseConstraint($constraint)

tests/Composer/Test/DependencyResolver/SolverTest.php

@@ -66,7 +66,7 @@ class SolverTest extends TestCase
         $this->repo->addPackage($this->getPackage('A', '1.0'));
         $this->reposComplete();
 
-        $this->request->install('B', $this->getVersionConstraint('=', '1'));
+        $this->request->install('B', $this->getVersionConstraint('==', '1'));
 
         try {
             $transaction = $this->solver->solve($this->request);
@@ -74,7 +74,7 @@ class SolverTest extends TestCase
         } catch (SolverProblemsException $e) {
             $problems = $e->getProblems();
             $this->assertEquals(1, count($problems));
-            $this->assertEquals('The requested package b == 1.0.0.0 could not be found.', (string) $problems[0]);
+            $this->assertEquals('The requested package b == 1 could not be found.', $problems[0]->getPrettyString());
         }
     }
 
@@ -641,7 +641,13 @@ class SolverTest extends TestCase
         } catch (SolverProblemsException $e) {
             $problems = $e->getProblems();
             $this->assertEquals(1, count($problems));
-            // TODO assert problem properties
+
+            $msg = "\n";
+            $msg .= "  Problem 1\n";
+            $msg .= "    - Installation request for a -> satisfiable by A 1.0.\n";
+            $msg .= "    - B 1.0 conflicts with A 1.0.\n";
+            $msg .= "    - Installation request for b -> satisfiable by B 1.0.\n";
+            $this->assertEquals($msg, $e->getMessage());
         }
     }
 
@@ -665,6 +671,56 @@ class SolverTest extends TestCase
             $problems = $e->getProblems();
             $this->assertEquals(1, count($problems));
             // TODO assert problem properties
+
+            $msg = "\n";
+            $msg .= "  Problem 1\n";
+            $msg .= "    - Installation request for a -> satisfiable by A 1.0.\n";
+            $msg .= "    - A 1.0 requires b >= 2.0 -> no matching package found.\n";
+            $this->assertEquals($msg, $e->getMessage());
+        }
+    }
+
+    public function testRequireMismatchException()
+    {
+        $this->repo->addPackage($packageA = $this->getPackage('A', '1.0'));
+        $this->repo->addPackage($packageB = $this->getPackage('B', '1.0'));
+        $this->repo->addPackage($packageB2 = $this->getPackage('B', '0.9'));
+        $this->repo->addPackage($packageC = $this->getPackage('C', '1.0'));
+        $this->repo->addPackage($packageD = $this->getPackage('D', '1.0'));
+
+        $packageA->setRequires(array(
+            new Link('A', 'B', $this->getVersionConstraint('>=', '1.0'), 'requires'),
+        ));
+        $packageB->setRequires(array(
+            new Link('B', 'C', $this->getVersionConstraint('>=', '1.0'), 'requires'),
+        ));
+        $packageC->setRequires(array(
+            new Link('C', 'D', $this->getVersionConstraint('>=', '1.0'), 'requires'),
+        ));
+        $packageD->setRequires(array(
+            new Link('D', 'B', $this->getVersionConstraint('<', '1.0'), 'requires'),
+        ));
+
+        $this->reposComplete();
+
+        $this->request->install('A');
+
+        try {
+            $transaction = $this->solver->solve($this->request);
+            $this->fail('Unsolvable conflict did not result in exception.');
+        } catch (SolverProblemsException $e) {
+            $problems = $e->getProblems();
+            $this->assertEquals(1, count($problems));
+
+            $msg = "\n";
+            $msg .= "  Problem 1\n";
+            $msg .= "    - C 1.0 requires d >= 1.0 -> satisfiable by D 1.0.\n";
+            $msg .= "    - D 1.0 requires b < 1.0 -> satisfiable by B 0.9.\n";
+            $msg .= "    - B 1.0 requires c >= 1.0 -> satisfiable by C 1.0.\n";
+            $msg .= "    - Can only install one of: B 0.9, B 1.0.\n";
+            $msg .= "    - A 1.0 requires b >= 1.0 -> satisfiable by B 1.0.\n";
+            $msg .= "    - Installation request for a -> satisfiable by A 1.0.\n";
+            $this->assertEquals($msg, $e->getMessage());
         }
     }
 

tests/Composer/Test/TestCase.php

@@ -33,10 +33,14 @@ abstract class TestCase extends \PHPUnit_Framework_TestCase
 
     protected function getVersionConstraint($operator, $version)
     {
-        return new VersionConstraint(
+        $constraint = new VersionConstraint(
             $operator,
             self::getVersionParser()->normalize($version)
         );
+
+        $constraint->setPrettyString($operator.' '.$version);
+
+        return $constraint;
     }
 
     protected function getPackage($name, $version)