From 967c771b26a888d2a31d4d611169201a4a7ff325 Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Mon, 5 Nov 2012 12:08:02 +0100 Subject: [PATCH] Add warnings to ValidatingArrayLoader that are simply stripped by default, add reporting of warnings when loading branches --- .../Loader/InvalidPackageException.php | 11 ++- .../Package/Loader/ValidatingArrayLoader.php | 67 +++++++++++-------- src/Composer/Repository/VcsRepository.php | 9 ++- src/Composer/Util/ConfigValidator.php | 3 +- .../Loader/ValidatingArrayLoaderTest.php | 55 +++++++++++++-- 5 files changed, 107 insertions(+), 38 deletions(-) diff --git a/src/Composer/Package/Loader/InvalidPackageException.php b/src/Composer/Package/Loader/InvalidPackageException.php index c409327c8..2f0c845c6 100644 --- a/src/Composer/Package/Loader/InvalidPackageException.php +++ b/src/Composer/Package/Loader/InvalidPackageException.php @@ -18,13 +18,15 @@ namespace Composer\Package\Loader; class InvalidPackageException extends \Exception { private $errors; + private $warnings; private $data; - public function __construct(array $errors, array $data) + public function __construct(array $errors, array $warnings, array $data) { $this->errors = $errors; + $this->warnings = $warnings; $this->data = $data; - parent::__construct("Invalid package information: \n".implode("\n", $errors)); + parent::__construct("Invalid package information: \n".implode("\n", array_merge($errors, $warnings))); } public function getData() @@ -36,4 +38,9 @@ class InvalidPackageException extends \Exception { return $this->errors; } + + public function getWarnings() + { + return $this->warnings; + } } diff --git a/src/Composer/Package/Loader/ValidatingArrayLoader.php b/src/Composer/Package/Loader/ValidatingArrayLoader.php index dcabcc663..d7f6bda34 100644 --- a/src/Composer/Package/Loader/ValidatingArrayLoader.php +++ b/src/Composer/Package/Loader/ValidatingArrayLoader.php @@ -23,20 +23,20 @@ class ValidatingArrayLoader implements LoaderInterface { private $loader; private $versionParser; - private $ignoreErrors; private $errors; + private $warnings; private $config; - public function __construct(LoaderInterface $loader, $ignoreErrors = true, VersionParser $parser = null) + public function __construct(LoaderInterface $loader, VersionParser $parser = null) { $this->loader = $loader; - $this->ignoreErrors = $ignoreErrors; $this->versionParser = $parser ?: new VersionParser(); } public function load(array $config, $class = 'Composer\Package\CompletePackage') { $this->errors = array(); + $this->warnings = array(); $this->config = $config; $this->validateRegex('name', '[A-Za-z0-9][A-Za-z0-9_.-]*/[A-Za-z0-9][A-Za-z0-9_.-]*', true); @@ -77,30 +77,27 @@ class ValidatingArrayLoader implements LoaderInterface } } - $this->validateArray('authors'); - if (!empty($this->config['authors'])) { + if ($this->validateArray('authors') && !empty($this->config['authors'])) { foreach ($this->config['authors'] as $key => $author) { if (!is_array($author)) { $this->errors[] = 'authors.'.$key.' : should be an array, '.gettype($author).' given'; unset($this->config['authors'][$key]); continue; } + foreach (array('homepage', 'email', 'name', 'role') as $authorData) { + if (isset($author[$authorData]) && !is_string($author[$authorData])) { + $this->errors[] = 'authors.'.$key.'.'.$authorData.' : invalid value, must be a string'; + unset($this->config['authors'][$key][$authorData]); + } + } if (isset($author['homepage']) && !$this->filterUrl($author['homepage'])) { - $this->errors[] = 'authors.'.$key.'.homepage : invalid value, must be a valid http/https URL'; + $this->warnings[] = 'authors.'.$key.'.homepage : invalid value, must be a valid http/https URL'; unset($this->config['authors'][$key]['homepage']); } if (isset($author['email']) && !filter_var($author['email'], FILTER_VALIDATE_EMAIL)) { - $this->errors[] = 'authors.'.$key.'.email : invalid value, must be a valid email address'; + $this->warnings[] = 'authors.'.$key.'.email : invalid value, must be a valid email address'; unset($this->config['authors'][$key]['email']); } - if (isset($author['name']) && !is_string($author['name'])) { - $this->errors[] = 'authors.'.$key.'.name : invalid value, must be a string'; - unset($this->config['authors'][$key]['name']); - } - if (isset($author['role']) && !is_string($author['role'])) { - $this->errors[] = 'authors.'.$key.'.role : invalid value, must be a string'; - unset($this->config['authors'][$key]['role']); - } if (empty($this->config['authors'][$key])) { unset($this->config['authors'][$key]); } @@ -110,23 +107,29 @@ class ValidatingArrayLoader implements LoaderInterface } } - $this->validateArray('support'); - if (!empty($this->config['support'])) { + if ($this->validateArray('support') && !empty($this->config['support'])) { + foreach (array('issues', 'forum', 'wiki', 'source', 'email', 'irc') as $key) { + if (isset($this->config['support'][$key]) && !is_string($this->config['support'][$key])) { + $this->errors[] = 'support.'.$key.' : invalid value, must be a string'; + unset($this->config['support'][$key]); + } + } + if (isset($this->config['support']['email']) && !filter_var($this->config['support']['email'], FILTER_VALIDATE_EMAIL)) { - $this->errors[] = 'support.email : invalid value, must be a valid email address'; + $this->warnings[] = 'support.email : invalid value, must be a valid email address'; unset($this->config['support']['email']); } if (isset($this->config['support']['irc']) && (!filter_var($this->config['support']['irc'], FILTER_VALIDATE_URL) || !preg_match('{^irc://}iu', $this->config['support']['irc'])) ) { - $this->errors[] = 'support.irc : invalid value, must be '; + $this->warnings[] = 'support.irc : invalid value, must be '; unset($this->config['support']['irc']); } foreach (array('issues', 'forum', 'wiki', 'source') as $key) { if (isset($this->config['support'][$key]) && !$this->filterUrl($this->config['support'][$key])) { - $this->errors[] = 'support.'.$key.' : invalid value, must be a valid http/https URL'; + $this->warnings[] = 'support.'.$key.' : invalid value, must be a valid http/https URL'; unset($this->config['support'][$key]); } } @@ -136,7 +139,7 @@ class ValidatingArrayLoader implements LoaderInterface } foreach (array_keys(BasePackage::$supportedLinkTypes) as $linkType) { - if (isset($this->config[$linkType])) { + if ($this->validateArray($linkType) && isset($this->config[$linkType])) { foreach ($this->config[$linkType] as $package => $constraint) { if (!is_string($constraint)) { $this->errors[] = $linkType.'.'.$package.' : invalid value, must be a string containing a version constraint'; @@ -153,8 +156,7 @@ class ValidatingArrayLoader implements LoaderInterface } } - $this->validateArray('suggest'); - if (!empty($this->config['suggest'])) { + if ($this->validateArray('suggest') && !empty($this->config['suggest'])) { foreach ($this->config['suggest'] as $package => $description) { if (!is_string($description)) { $this->errors[] = 'suggest.'.$package.' : invalid value, must be a string describing why the package is suggested'; @@ -205,17 +207,26 @@ class ValidatingArrayLoader implements LoaderInterface } } - if ($this->errors && !$this->ignoreErrors) { - throw new InvalidPackageException($this->errors, $config); + if ($this->errors) { + throw new InvalidPackageException($this->errors, $this->warnings, $config); } $package = $this->loader->load($this->config, $class); - $this->errors = array(); $this->config = null; return $package; } + public function getWarnings() + { + return $this->warnings; + } + + public function getErrors() + { + return $this->errors; + } + private function validateRegex($property, $regex, $mandatory = false) { if (!$this->validateString($property, $mandatory)) { @@ -307,11 +318,13 @@ class ValidatingArrayLoader implements LoaderInterface } if (!$this->filterUrl($this->config[$property])) { - $this->errors[] = $property.' : invalid value, must be a valid http/https URL'; + $this->warnings[] = $property.' : invalid value, must be a valid http/https URL'; unset($this->config[$property]); return false; } + + return true; } private function filterUrl($value) diff --git a/src/Composer/Repository/VcsRepository.php b/src/Composer/Repository/VcsRepository.php index a7cf0181b..9051ee3d7 100644 --- a/src/Composer/Repository/VcsRepository.php +++ b/src/Composer/Repository/VcsRepository.php @@ -16,6 +16,8 @@ use Composer\Downloader\TransportException; use Composer\Repository\Vcs\VcsDriverInterface; use Composer\Package\Version\VersionParser; use Composer\Package\Loader\ArrayLoader; +use Composer\Package\Loader\ValidatingArrayLoader; +use Composer\Package\Loader\InvalidPackageException; use Composer\Package\Loader\LoaderInterface; use Composer\IO\IOInterface; use Composer\Config; @@ -217,7 +219,12 @@ class VcsRepository extends ArrayRepository $this->io->write('Importing branch '.$branch.' ('.$data['version'].')'); } - $this->addPackage($this->loader->load($this->preProcess($driver, $data, $identifier))); + $packageData = $this->preProcess($driver, $data, $identifier); + $package = $this->loader->load($packageData); + if ($this->loader instanceof ValidatingArrayLoader && $this->loader->getWarnings()) { + throw new InvalidPackageException($this->loader->getErrors(), $this->loader->getWarnings(), $packageData); + } + $this->addPackage($package); } catch (TransportException $e) { if ($verbose) { $this->io->write('Skipped branch '.$branch.', no composer file was found'); diff --git a/src/Composer/Util/ConfigValidator.php b/src/Composer/Util/ConfigValidator.php index a2310611d..6607e840c 100644 --- a/src/Composer/Util/ConfigValidator.php +++ b/src/Composer/Util/ConfigValidator.php @@ -97,7 +97,7 @@ class ConfigValidator } try { - $loader = new ValidatingArrayLoader(new ArrayLoader(), false); + $loader = new ValidatingArrayLoader(new ArrayLoader()); if (!isset($manifest['version'])) { $manifest['version'] = '1.0.0'; } @@ -107,6 +107,7 @@ class ConfigValidator $loader->load($manifest); } catch (InvalidPackageException $e) { $errors = array_merge($errors, $e->getErrors()); + $warnings = array_merge($warnings, $e->getWarnings()); } return array($errors, $publishErrors, $warnings); diff --git a/tests/Composer/Test/Package/Loader/ValidatingArrayLoaderTest.php b/tests/Composer/Test/Package/Loader/ValidatingArrayLoaderTest.php index 5d7ba98e4..84ec6d53f 100644 --- a/tests/Composer/Test/Package/Loader/ValidatingArrayLoaderTest.php +++ b/tests/Composer/Test/Package/Loader/ValidatingArrayLoaderTest.php @@ -29,7 +29,7 @@ class ValidatingArrayLoaderTest extends \PHPUnit_Framework_TestCase ->method('load') ->with($config); - $loader = new ValidatingArrayLoader($internalLoader, false); + $loader = new ValidatingArrayLoader($internalLoader); $loader->load($config); } @@ -148,12 +148,12 @@ class ValidatingArrayLoaderTest extends \PHPUnit_Framework_TestCase } /** - * @dataProvider failureProvider + * @dataProvider errorProvider */ public function testLoadFailureThrowsException($config, $expectedErrors) { $internalLoader = $this->getMock('Composer\Package\Loader\LoaderInterface'); - $loader = new ValidatingArrayLoader($internalLoader, false); + $loader = new ValidatingArrayLoader($internalLoader); try { $loader->load($config); $this->fail('Expected exception to be thrown'); @@ -166,9 +166,24 @@ class ValidatingArrayLoaderTest extends \PHPUnit_Framework_TestCase } /** - * @dataProvider failureProvider + * @dataProvider warningProvider */ - public function testLoadSkipsInvalidDataWhenIgnoringErrors($config) + public function testLoadWarnings($config, $expectedWarnings) + { + $internalLoader = $this->getMock('Composer\Package\Loader\LoaderInterface'); + $loader = new ValidatingArrayLoader($internalLoader); + + $loader->load($config); + $warnings = $loader->getWarnings(); + sort($expectedWarnings); + sort($warnings); + $this->assertEquals($expectedWarnings, $warnings); + } + + /** + * @dataProvider warningProvider + */ + public function testLoadSkipsWarningDataWhenIgnoringErrors($config) { $internalLoader = $this->getMock('Composer\Package\Loader\LoaderInterface'); $internalLoader @@ -176,12 +191,12 @@ class ValidatingArrayLoaderTest extends \PHPUnit_Framework_TestCase ->method('load') ->with(array('name' => 'a/b')); - $loader = new ValidatingArrayLoader($internalLoader, true); + $loader = new ValidatingArrayLoader($internalLoader); $config['name'] = 'a/b'; $loader->load($config); } - public function failureProvider() + public function errorProvider() { return array( array( @@ -192,6 +207,32 @@ class ValidatingArrayLoaderTest extends \PHPUnit_Framework_TestCase 'name : invalid value, must match [A-Za-z0-9][A-Za-z0-9_.-]*/[A-Za-z0-9][A-Za-z0-9_.-]*' ) ), + array( + array( + 'name' => 'foo/bar', + 'homepage' => 43, + ), + array( + 'homepage : should be a string, integer given', + ) + ), + array( + array( + 'name' => 'foo/bar', + 'support' => array( + 'source' => array(), + ), + ), + array( + 'support.source : invalid value, must be a string', + ) + ), + ); + } + + public function warningProvider() + { + return array( array( array( 'name' => 'foo/bar',