Refactor commit-ref validation

The require and require-dev arrays have been merged into one and no
longer user private methods/properties to collect warnings.

src/Composer/Util/ConfigValidator.php

@@ -30,8 +30,6 @@ class ConfigValidator
 {
     private $io;
 
-    private $warnings = array();
-
     public function __construct(IOInterface $io)
     {
         $this->io = $io;
@@ -49,6 +47,7 @@ class ConfigValidator
     {
         $errors = array();
         $publishErrors = array();
+        $warnings = array();
 
         // validate json schema
         $laxValid = false;
@@ -86,18 +85,18 @@ class ConfigValidator
 
             $licenseValidator = new SpdxLicenses();
             if ('proprietary' !== $manifest['license'] && array() !== $manifest['license'] && !$licenseValidator->validate($manifest['license'])) {
-                $this->warnings[] = sprintf(
+                $warnings[] = sprintf(
                     'License %s is not a valid SPDX license identifier, see http://www.spdx.org/licenses/ if you use an open license.'
                     ."\nIf the software is closed-source, you may use \"proprietary\" as license.",
                     json_encode($manifest['license'])
                 );
             }
         } else {
-            $this->warnings[] = 'No license specified, it is recommended to do so. For closed-source software you may use "proprietary" as license.';
+            $warnings[] = 'No license specified, it is recommended to do so. For closed-source software you may use "proprietary" as license.';
         }
 
         if (isset($manifest['version'])) {
-            $this->warnings[] = 'The version field is present, it is recommended to leave it out if the package is published on Packagist.';
+            $warnings[] = 'The version field is present, it is recommended to leave it out if the package is published on Packagist.';
         }
 
         if (!empty($manifest['name']) && preg_match('{[A-Z]}', $manifest['name'])) {
@@ -112,7 +111,7 @@ class ConfigValidator
         }
 
         if (!empty($manifest['type']) && $manifest['type'] == 'composer-installer') {
-            $this->warnings[] = "The package type 'composer-installer' is deprecated. Please distribute your custom installers as plugins from now on. See http://getcomposer.org/doc/articles/plugins.md for plugin documentation.";
+            $warnings[] = "The package type 'composer-installer' is deprecated. Please distribute your custom installers as plugins from now on. See http://getcomposer.org/doc/articles/plugins.md for plugin documentation.";
         }
 
         // check for require-dev overrides
@@ -120,20 +119,27 @@ class ConfigValidator
             $requireOverrides = array_intersect_key($manifest['require'], $manifest['require-dev']);
             if (!empty($requireOverrides)) {
                 $plural = (count($requireOverrides) > 1) ? 'are' : 'is';
-                $this->warnings[] = implode(', ', array_keys($requireOverrides)). " {$plural} required both in require and require-dev, this can lead to unexpected behavior";
+                $warnings[] = implode(', ', array_keys($requireOverrides)). " {$plural} required both in require and require-dev, this can lead to unexpected behavior";
             }
         }
 
         // check for commit references
-        $this->checkForCommitReferences($manifest['require']);
-        $this->checkForCommitReferences($manifest['require-dev']);
+        $packages = array_merge($manifest['require'], $manifest['require-dev']);
+        foreach ($packages as $package => $version) {
+            if (preg_match('/#/', $version) === 1) {
+                $warnings[] = sprintf(
+                    'The package "%s" is pointing to a commit-ref, this is bad practice and can cause unforeseen issues.',
+                    $package
+                );
+            }
+        }
 
         // check for empty psr-0/psr-4 namespace prefixes
         if (isset($manifest['autoload']['psr-0'][''])) {
-            $this->warnings[] = "Defining autoload.psr-0 with an empty namespace prefix is a bad idea for performance";
+            $warnings[] = "Defining autoload.psr-0 with an empty namespace prefix is a bad idea for performance";
         }
         if (isset($manifest['autoload']['psr-4'][''])) {
-            $this->warnings[] = "Defining autoload.psr-4 with an empty namespace prefix is a bad idea for performance";
+            $warnings[] = "Defining autoload.psr-4 with an empty namespace prefix is a bad idea for performance";
         }
 
         try {
@@ -149,27 +155,8 @@ class ConfigValidator
             $errors = array_merge($errors, $e->getErrors());
         }
 
-        $this->warnings = array_merge($this->warnings, $loader->getWarnings());
+        $warnings = array_merge($warnings, $loader->getWarnings());
 
-        return array($errors, $publishErrors, $this->warnings);
-    }
-
-    /**
-     * Check for explicit commit references.
-     *
-     * @param array $packages An array of packages and their versions
-     *
-     * @return void
-     */
-    private function checkForCommitReferences(array $packages)
-    {
-        foreach ($packages as $package => $version) {
-            if (preg_match('/#/', $version) === 1) {
-                $this->warnings[] = sprintf(
-                    'The package "%s" is pointing to a commit-ref, this is bad practice and can cause unforeseen issues.',
-                    $package
-                );
-            }
-        }
+        return array($errors, $publishErrors, $warnings);
     }
 }

tests/Composer/Test/Util/ConfigValidatorTest.php

@@ -27,22 +27,11 @@ class ConfigValidatorTest extends TestCase
     public function testConfigValidatorCommitRefWarning()
     {
         $configValidator = new ConfigValidator(new NullIO());
-        $reflection      = new \ReflectionClass(get_class($configValidator));
-        $method          = $reflection->getMethod('checkForCommitReferences');
-        $warnings        = $reflection->getProperty('warnings');
+        list(, , $warnings) = $configValidator->validate(__DIR__ . '/Fixtures/composer_commit-ref.json');
 
-        $method->setAccessible(true);
-        $warnings->setAccessible(true);
-
-        $this->assertEquals(0, count($warnings->getValue($configValidator)));
-
-        $method->invokeArgs($configValidator, array(
-            array(
-                'some-package'    => 'dev-master#62c4da6',
-                'another-package' => '^1.0.0'
-            )
+        $this->assertEquals(true, in_array(
+            'The package "some/package" is pointing to a commit-ref, this is bad practice and can cause unforeseen issues.',
+            $warnings
         ));
-
-        $this->assertEquals(1, count($warnings->getValue($configValidator)));
     }
 }