From 73e73c90fbff81ab1c6e022cdea28c2e1114db4c Mon Sep 17 00:00:00 2001 From: Jeroen Seegers Date: Fri, 23 Oct 2015 19:20:56 +0200 Subject: [PATCH] Generate a warning when a commit reference is used Closes #4485 --- src/Composer/Util/ConfigValidator.php | 45 ++++++++++++----- .../Test/Util/ConfigValidatorTest.php | 48 +++++++++++++++++++ 2 files changed, 82 insertions(+), 11 deletions(-) create mode 100644 tests/Composer/Test/Util/ConfigValidatorTest.php diff --git a/src/Composer/Util/ConfigValidator.php b/src/Composer/Util/ConfigValidator.php index a7ddaca9b..06c1372ef 100644 --- a/src/Composer/Util/ConfigValidator.php +++ b/src/Composer/Util/ConfigValidator.php @@ -30,6 +30,8 @@ class ConfigValidator { private $io; + private $warnings = array(); + public function __construct(IOInterface $io) { $this->io = $io; @@ -47,7 +49,6 @@ class ConfigValidator { $errors = array(); $publishErrors = array(); - $warnings = array(); // validate json schema $laxValid = false; @@ -85,18 +86,18 @@ class ConfigValidator $licenseValidator = new SpdxLicenses(); if ('proprietary' !== $manifest['license'] && array() !== $manifest['license'] && !$licenseValidator->validate($manifest['license'])) { - $warnings[] = sprintf( + $this->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 { - $warnings[] = 'No license specified, it is recommended to do so. For closed-source software you may use "proprietary" as license.'; + $this->warnings[] = 'No license specified, it is recommended to do so. For closed-source software you may use "proprietary" as license.'; } if (isset($manifest['version'])) { - $warnings[] = 'The version field is present, it is recommended to leave it out if the package is published on Packagist.'; + $this->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'])) { @@ -111,25 +112,28 @@ class ConfigValidator } if (!empty($manifest['type']) && $manifest['type'] == 'composer-installer') { - $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."; + $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."; } // check for require-dev overrides if (isset($manifest['require']) && isset($manifest['require-dev'])) { $requireOverrides = array_intersect_key($manifest['require'], $manifest['require-dev']); - if (!empty($requireOverrides)) { $plural = (count($requireOverrides) > 1) ? 'are' : 'is'; - $warnings[] = implode(', ', array_keys($requireOverrides)). " {$plural} required both in require and require-dev, this can lead to unexpected behavior"; + $this->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']); + // check for empty psr-0/psr-4 namespace prefixes if (isset($manifest['autoload']['psr-0'][''])) { - $warnings[] = "Defining autoload.psr-0 with an empty namespace prefix is a bad idea for performance"; + $this->warnings[] = "Defining autoload.psr-0 with an empty namespace prefix is a bad idea for performance"; } if (isset($manifest['autoload']['psr-4'][''])) { - $warnings[] = "Defining autoload.psr-4 with an empty namespace prefix is a bad idea for performance"; + $this->warnings[] = "Defining autoload.psr-4 with an empty namespace prefix is a bad idea for performance"; } try { @@ -145,8 +149,27 @@ class ConfigValidator $errors = array_merge($errors, $e->getErrors()); } - $warnings = array_merge($warnings, $loader->getWarnings()); + $this->warnings = array_merge($this->warnings, $loader->getWarnings()); - return array($errors, $publishErrors, $warnings); + 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 + ); + } + } } } diff --git a/tests/Composer/Test/Util/ConfigValidatorTest.php b/tests/Composer/Test/Util/ConfigValidatorTest.php new file mode 100644 index 000000000..2ed4c4440 --- /dev/null +++ b/tests/Composer/Test/Util/ConfigValidatorTest.php @@ -0,0 +1,48 @@ + + * Jordi Boggiano + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Composer\Test\Util; + +use Composer\IO\NullIO; +use Composer\Util\ConfigValidator; +use Composer\TestCase; + +/** + * ConfigValidator test case + */ +class ConfigValidatorTest extends TestCase +{ + /** + * Test ConfigValidator warns on commit reference + */ + public function testConfigValidatorCommitRefWarning() + { + $configValidator = new ConfigValidator(new NullIO()); + $reflection = new \ReflectionClass(get_class($configValidator)); + $method = $reflection->getMethod('checkForCommitReferences'); + $warnings = $reflection->getProperty('warnings'); + + $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(1, count($warnings->getValue($configValidator))); + } +}