From f3dc31839fca620b64f44b2625ce0091f7818302 Mon Sep 17 00:00:00 2001 From: Jeroen Seegers Date: Tue, 27 Oct 2015 13:51:51 +0100 Subject: [PATCH] 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 | 51 +++++++------------ .../Test/Util/ConfigValidatorTest.php | 19 ++----- 2 files changed, 23 insertions(+), 47 deletions(-) diff --git a/src/Composer/Util/ConfigValidator.php b/src/Composer/Util/ConfigValidator.php index 06c1372ef..6bf0e73c5 100644 --- a/src/Composer/Util/ConfigValidator.php +++ b/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); } } diff --git a/tests/Composer/Test/Util/ConfigValidatorTest.php b/tests/Composer/Test/Util/ConfigValidatorTest.php index 2ed4c4440..e4edc1ca4 100644 --- a/tests/Composer/Test/Util/ConfigValidatorTest.php +++ b/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))); } }