Merge pull request #1067 from stof/suggestions

Refactored the search of suggested packages to support replacements

src/Composer/Installer.php

@@ -188,7 +188,12 @@ class Installer
 
         // output suggestions
         foreach ($this->suggestedPackages as $suggestion) {
-            if (!$installedRepo->findPackages($suggestion['target'])) {
+            $target = $suggestion['target'];
+            if ($installedRepo->filterPackages(function (PackageInterface $package) use ($target) {
+                if (in_array($target, $package->getNames())) {
+                    return false;
+                }
+            })) {
                 $this->io->write($suggestion['source'].' suggests installing '.$suggestion['target'].' ('.$suggestion['reason'].')');
             }
         }

tests/Composer/Test/Fixtures/installer/suggest-installed.test

@@ -0,0 +1,29 @@
+--TEST--
+Suggestions are not displayed for installed packages
+--COMPOSER--
+{
+    "repositories": [
+        {
+            "type": "package",
+            "package": [
+                { "name": "a/a", "version": "1.0.0", "suggest": { "b/b": "an obscure reason" } },
+                { "name": "b/b", "version": "1.0.0" }
+            ]
+        }
+    ],
+    "require": {
+        "a/a": "1.0.0",
+        "b/b": "1.0.0"
+    }
+}
+--RUN--
+install
+--EXPECT-OUTPUT--
+<info>Loading composer repositories with package information</info>
+<info>Installing dependencies</info>
+<info>Writing lock file</info>
+<info>Generating autoload files</info>
+
+--EXPECT--
+Installing a/a (1.0.0)
+Installing b/b (1.0.0)

tests/Composer/Test/Fixtures/installer/suggest-replaced.test

@@ -0,0 +1,29 @@
+--TEST--
+Suggestions are not displayed for packages if they are replaced
+--COMPOSER--
+{
+    "repositories": [
+        {
+            "type": "package",
+            "package": [
+                { "name": "a/a", "version": "1.0.0", "suggest": { "b/b": "an obscure reason" } },
+                { "name": "c/c", "version": "1.0.0", "replace": { "b/b": "1.0.0" } }
+            ]
+        }
+    ],
+    "require": {
+        "a/a": "1.0.0",
+        "b/b": "1.0.0"
+    }
+}
+--RUN--
+install
+--EXPECT-OUTPUT--
+<info>Loading composer repositories with package information</info>
+<info>Installing dependencies</info>
+<info>Writing lock file</info>
+<info>Generating autoload files</info>
+
+--EXPECT--
+Installing a/a (1.0.0)
+Installing c/c (1.0.0)

tests/Composer/Test/Fixtures/installer/suggest-uninstalled.test

@@ -0,0 +1,27 @@
+--TEST--
+Suggestions are displayed
+--COMPOSER--
+{
+    "repositories": [
+        {
+            "type": "package",
+            "package": [
+                { "name": "a/a", "version": "1.0.0", "suggest": { "b/b": "an obscure reason" } }
+            ]
+        }
+    ],
+    "require": {
+        "a/a": "1.0.0"
+    }
+}
+--RUN--
+install
+--EXPECT-OUTPUT--
+<info>Loading composer repositories with package information</info>
+<info>Installing dependencies</info>
+a/a suggests installing b/b (an obscure reason)
+<info>Writing lock file</info>
+<info>Generating autoload files</info>
+
+--EXPECT--
+Installing a/a (1.0.0)

tests/Composer/Test/InstallerTest.php

@@ -123,7 +123,7 @@ class InstallerTest extends TestCase
     /**
      * @dataProvider getIntegrationTests
      */
-    public function testIntegration($file, $message, $condition, $composerConfig, $lock, $installed, $installedDev, $run, $expectLock, $expect)
+    public function testIntegration($file, $message, $condition, $composerConfig, $lock, $installed, $installedDev, $run, $expectLock, $expectOutput, $expect)
     {
         if ($condition) {
             eval('$res = '.$condition.';');
@@ -226,6 +226,10 @@ class InstallerTest extends TestCase
 
         $installationManager = $composer->getInstallationManager();
         $this->assertSame($expect, implode("\n", $installationManager->getTrace()));
+
+        if ($expectOutput) {
+            $this->assertEquals($expectOutput, $output);
+        }
     }
 
     public function getIntegrationTests()
@@ -250,6 +254,7 @@ class InstallerTest extends TestCase
                 (?:--INSTALLED-DEV--\s*(?P<installedDev>'.$content.'))?\s*
                 --RUN--\s*(?P<run>.*?)\s*
                 (?:--EXPECT-LOCK--\s*(?P<expectLock>'.$content.'))?\s*
+                (?:--EXPECT-OUTPUT--\s*(?P<expectOutput>'.$content.'))?\s*
                 --EXPECT--\s*(?P<expect>.*?)\s*
             $}xs';
 
@@ -279,6 +284,7 @@ class InstallerTest extends TestCase
                     if (!empty($match['expectLock'])) {
                         $expectLock = JsonFile::parseJson($match['expectLock']);
                     }
+                    $expectOutput = $match['expectOutput'];
                     $expect = $match['expect'];
                 } catch (\Exception $e) {
                     die(sprintf('Test "%s" is not valid: '.$e->getMessage(), str_replace($fixturesDir.'/', '', $file)));
@@ -287,7 +293,7 @@ class InstallerTest extends TestCase
                 die(sprintf('Test "%s" is not valid, did not match the expected format.', str_replace($fixturesDir.'/', '', $file)));
             }
 
-            $tests[] = array(str_replace($fixturesDir.'/', '', $file), $message, $condition, $composer, $lock, $installed, $installedDev, $run, $expectLock, $expect);
+            $tests[] = array(str_replace($fixturesDir.'/', '', $file), $message, $condition, $composer, $lock, $installed, $installedDev, $run, $expectLock, $expectOutput, $expect);
         }
 
         return $tests;