From 73e73c90fbff81ab1c6e022cdea28c2e1114db4c Mon Sep 17 00:00:00 2001 From: Jeroen Seegers Date: Fri, 23 Oct 2015 19:20:56 +0200 Subject: [PATCH 1/4] 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))); + } +} From 476c6f279b99402d01ced153d1a814bc7025562d Mon Sep 17 00:00:00 2001 From: Jeroen Seegers Date: Tue, 27 Oct 2015 13:50:41 +0100 Subject: [PATCH 2/4] Add fixture for composer.json with commit-ref --- .../Util/Fixtures/composer_commit-ref.json | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 tests/Composer/Test/Util/Fixtures/composer_commit-ref.json diff --git a/tests/Composer/Test/Util/Fixtures/composer_commit-ref.json b/tests/Composer/Test/Util/Fixtures/composer_commit-ref.json new file mode 100644 index 000000000..9d4a4d309 --- /dev/null +++ b/tests/Composer/Test/Util/Fixtures/composer_commit-ref.json @@ -0,0 +1,55 @@ +{ + "name": "composer/commit-ref-validation", + "description": "Dummy file to test the commit-ref validation", + "keywords": ["package", "dependency", "autoload"], + "homepage": "https://getcomposer.org/", + "type": "library", + "license": "MIT", + "authors": [ + { + "name": "Nils Adermann", + "email": "naderman@naderman.de", + "homepage": "http://www.naderman.de" + }, + { + "name": "Jordi Boggiano", + "email": "j.boggiano@seld.be", + "homepage": "http://seld.be" + } + ], + "support": { + "irc": "irc://irc.freenode.org/composer", + "issues": "https://github.com/composer/composer/issues" + }, + "require": { + "php": ">=5.3.2", + "some/package": "dev-master#fgb42d" + }, + "require-dev": { + "phpunit/phpunit": "~4.5" + }, + "config": { + "platform": { + "php": "5.3.3" + } + }, + "suggest": { + "ext-zip": "Enabling the zip extension allows you to unzip archives, and allows gzip compression of all internet traffic", + "ext-openssl": "Enabling the openssl extension allows you to access https URLs for repositories and packages" + }, + "autoload": { + "psr-0": { "Composer": "src/" } + }, + "autoload-dev": { + "psr-0": { "Composer\\Test": "tests/" } + }, + "bin": ["bin/composer"], + "extra": { + "branch-alias": { + "dev-master": "1.0-dev" + } + }, + "scripts": { + "test": "phpunit" + } +} From f3dc31839fca620b64f44b2625ce0091f7818302 Mon Sep 17 00:00:00 2001 From: Jeroen Seegers Date: Tue, 27 Oct 2015 13:51:51 +0100 Subject: [PATCH 3/4] 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))); } } From 279b5f01563f795abd927e52463b09db0fd74785 Mon Sep 17 00:00:00 2001 From: Jeroen Seegers Date: Tue, 27 Oct 2015 14:06:48 +0100 Subject: [PATCH 4/4] Drop irrelevant properties from composer_commit-ref.json --- src/Composer/Util/ConfigValidator.php | 5 +- .../Util/Fixtures/composer_commit-ref.json | 50 ------------------- 2 files changed, 4 insertions(+), 51 deletions(-) diff --git a/src/Composer/Util/ConfigValidator.php b/src/Composer/Util/ConfigValidator.php index 6bf0e73c5..f4a876cfe 100644 --- a/src/Composer/Util/ConfigValidator.php +++ b/src/Composer/Util/ConfigValidator.php @@ -117,6 +117,7 @@ class ConfigValidator // 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"; @@ -124,7 +125,9 @@ class ConfigValidator } // check for commit references - $packages = array_merge($manifest['require'], $manifest['require-dev']); + $require = isset($manifest['require']) ? $manifest['require'] : array(); + $requireDev = isset($manifest['require-dev']) ? $manifest['require-dev'] : array(); + $packages = array_merge($require, $requireDev); foreach ($packages as $package => $version) { if (preg_match('/#/', $version) === 1) { $warnings[] = sprintf( diff --git a/tests/Composer/Test/Util/Fixtures/composer_commit-ref.json b/tests/Composer/Test/Util/Fixtures/composer_commit-ref.json index 9d4a4d309..40bbbe41d 100644 --- a/tests/Composer/Test/Util/Fixtures/composer_commit-ref.json +++ b/tests/Composer/Test/Util/Fixtures/composer_commit-ref.json @@ -1,55 +1,5 @@ { - "name": "composer/commit-ref-validation", - "description": "Dummy file to test the commit-ref validation", - "keywords": ["package", "dependency", "autoload"], - "homepage": "https://getcomposer.org/", - "type": "library", - "license": "MIT", - "authors": [ - { - "name": "Nils Adermann", - "email": "naderman@naderman.de", - "homepage": "http://www.naderman.de" - }, - { - "name": "Jordi Boggiano", - "email": "j.boggiano@seld.be", - "homepage": "http://seld.be" - } - ], - "support": { - "irc": "irc://irc.freenode.org/composer", - "issues": "https://github.com/composer/composer/issues" - }, "require": { - "php": ">=5.3.2", "some/package": "dev-master#fgb42d" - }, - "require-dev": { - "phpunit/phpunit": "~4.5" - }, - "config": { - "platform": { - "php": "5.3.3" - } - }, - "suggest": { - "ext-zip": "Enabling the zip extension allows you to unzip archives, and allows gzip compression of all internet traffic", - "ext-openssl": "Enabling the openssl extension allows you to access https URLs for repositories and packages" - }, - "autoload": { - "psr-0": { "Composer": "src/" } - }, - "autoload-dev": { - "psr-0": { "Composer\\Test": "tests/" } - }, - "bin": ["bin/composer"], - "extra": { - "branch-alias": { - "dev-master": "1.0-dev" - } - }, - "scripts": { - "test": "phpunit" } }