From 90cfe33631568af839d79438f3f48c2397853970 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Thu, 15 Dec 2016 15:09:26 -0800 Subject: [PATCH] registration: Improve dependency checking * Pass $coreVersion to VersionChecker's constructor, don't require a setter. * Bump ExtensionRegistry::CACHE_VERSION * Return single strings from handle* functions, avoid array_merge calls * Improve invalid version error message * Fix naming of VersionCheckerTest class Change-Id: Id4f66b815aa41dbbc4b966095d6b99e542e548b4 --- includes/registration/ExtensionRegistry.php | 5 +- includes/registration/VersionChecker.php | 69 +++++++++---------- .../registration/VersionCheckerTest.php | 18 ++--- 3 files changed, 42 insertions(+), 50 deletions(-) diff --git a/includes/registration/ExtensionRegistry.php b/includes/registration/ExtensionRegistry.php index 76d25b68e6..c5b21500f5 100644 --- a/includes/registration/ExtensionRegistry.php +++ b/includes/registration/ExtensionRegistry.php @@ -31,7 +31,7 @@ class ExtensionRegistry { /** * Bump whenever the registration cache needs resetting */ - const CACHE_VERSION = 4; + const CACHE_VERSION = 5; /** * Special key that defines the merge strategy @@ -203,8 +203,7 @@ class ExtensionRegistry { $autoloadClasses = []; $autoloaderPaths = []; $processor = new ExtensionProcessor(); - $versionChecker = new VersionChecker(); - $versionChecker->setCoreVersion( $wgVersion ); + $versionChecker = new VersionChecker( $wgVersion ); $extDependencies = []; $incompatible = []; foreach ( $queue as $path => $mtime ) { diff --git a/includes/registration/VersionChecker.php b/includes/registration/VersionChecker.php index 2a9401eab6..5aaaa1b8ab 100644 --- a/includes/registration/VersionChecker.php +++ b/includes/registration/VersionChecker.php @@ -45,8 +45,12 @@ class VersionChecker { */ private $versionParser; - public function __construct() { + /** + * @param string $coreVersion Current version of core + */ + public function __construct( $coreVersion ) { $this->versionParser = new VersionParser(); + $this->setCoreVersion( $coreVersion ); } /** @@ -65,9 +69,8 @@ class VersionChecker { * Set MediaWiki core version. * * @param string $coreVersion Current version of core - * @return VersionChecker $this */ - public function setCoreVersion( $coreVersion ) { + private function setCoreVersion( $coreVersion ) { try { $this->coreVersion = new Constraint( '==', @@ -77,8 +80,6 @@ class VersionChecker { } catch ( UnexpectedValueException $e ) { // Non-parsable version, don't fatal. } - - return $this; } /** @@ -87,14 +88,14 @@ class VersionChecker { * * Example $extDependencies: * { - * 'GoogleAPIClient' => { + * 'FooBar' => { * 'MediaWiki' => '>= 1.25.0', * 'extensions' => { - * 'FakeExtension' => '>= 1.25.0' + * 'FooBaz' => '>= 1.25.0' * }, - * 'skins' => { - * 'FakeSkin' => '>= 1.0.0' - * } + * 'skins' => { + * 'BazBar' => '>= 1.0.0' + * } * } * } * @@ -107,18 +108,18 @@ class VersionChecker { foreach ( $dependencies as $dependencyType => $values ) { switch ( $dependencyType ) { case ExtensionRegistry::MEDIAWIKI_CORE: - $errors = array_merge( - $errors, - $this->handleMediaWikiDependency( $values, $extension ) - ); + $mwError = $this->handleMediaWikiDependency( $values, $extension ); + if ( $mwError !== false ) { + $errors[] = $mwError; + } break; case 'extensions': case 'skin': foreach ( $values as $dependency => $constraint ) { - $errors = array_merge( - $errors, - $this->handleExtensionDependency( $dependency, $constraint, $extension ) - ); + $extError = $this->handleExtensionDependency( $dependency, $constraint, $extension ); + if ( $extError !== false ) { + $errors[] = $extError; + } } break; default: @@ -138,24 +139,23 @@ class VersionChecker { * * @param string $constraint The required version constraint for this dependency * @param string $checkedExt The Extension, which depends on this dependency - * @return array An empty array, if MediaWiki version is compatible with $constraint, an array - * with an error message, otherwise. + * @return bool|string false if no error, or a string with the message */ private function handleMediaWikiDependency( $constraint, $checkedExt ) { if ( $this->coreVersion === false ) { // Couldn't parse the core version, so we can't check anything - return []; + return false; } // if the installed and required version are compatible, return an empty array if ( $this->versionParser->parseConstraints( $constraint ) ->matches( $this->coreVersion ) ) { - return []; + return false; } // otherwise mark this as incompatible. - return [ "{$checkedExt} is not compatible with the current " - . "MediaWiki core (version {$this->coreVersion->getPrettyString()}), it requires: " - . $constraint . '.' ]; + return "{$checkedExt} is not compatible with the current " + . "MediaWiki core (version {$this->coreVersion->getPrettyString()}), it requires: " + . "$constraint."; } /** @@ -164,15 +164,12 @@ class VersionChecker { * @param string $dependencyName The name of the dependency * @param string $constraint The required version constraint for this dependency * @param string $checkedExt The Extension, which depends on this dependency - * @return array An empty array, if installed version is compatible with $constraint, an array - * with an error message, otherwise. + * @return bool|string false for no errors, or a string message */ private function handleExtensionDependency( $dependencyName, $constraint, $checkedExt ) { - $incompatible = []; // Check if the dependency is even installed if ( !isset( $this->loaded[$dependencyName] ) ) { - $incompatible[] = "{$checkedExt} requires {$dependencyName} to be installed."; - return $incompatible; + return "{$checkedExt} requires {$dependencyName} to be installed."; } // Check if the dependency has specified a version if ( !isset( $this->loaded[$dependencyName]['version'] ) ) { @@ -180,9 +177,10 @@ class VersionChecker { if ( $constraint === '*' ) { wfDebug( "{$dependencyName} does not expose it's version, but {$checkedExt} mentions it with constraint '*'. Assume it's ok so." ); + return false; } else { // Otherwise, mark it as incompatible. - $incompatible[] = "{$dependencyName} does not expose it's version, but {$checkedExt} + return "{$dependencyName} does not expose it's version, but {$checkedExt} requires: {$constraint}."; } } else { @@ -193,22 +191,21 @@ class VersionChecker { $this->versionParser->normalize( $this->loaded[$dependencyName]['version'] ) ); } catch ( UnexpectedValueException $e ) { - // Non-parsable version, don't fatal, output an error message that the version + // Non-parsable version, output an error message that the version // string is invalid - return [ "Dependency $dependencyName provides an invalid version string." ]; + return "$dependencyName does not have a valid version string."; } // Check if the constraint actually matches... if ( - isset( $installedVersion ) && !$this->versionParser->parseConstraints( $constraint )->matches( $installedVersion ) ) { - $incompatible[] = "{$checkedExt} is not compatible with the current " + return "{$checkedExt} is not compatible with the current " . "installed version of {$dependencyName} " . "({$this->loaded[$dependencyName]['version']}), " . "it requires: " . $constraint . '.'; } } - return $incompatible; + return false; } } diff --git a/tests/phpunit/includes/registration/VersionCheckerTest.php b/tests/phpunit/includes/registration/VersionCheckerTest.php index 2bb1fe4e59..9ee58816df 100644 --- a/tests/phpunit/includes/registration/VersionCheckerTest.php +++ b/tests/phpunit/includes/registration/VersionCheckerTest.php @@ -1,15 +1,14 @@ setCoreVersion( $coreVersion ); + $checker = new VersionChecker( $coreVersion ); $this->assertEquals( $expected, !(bool)$checker->checkArray( [ 'FakeExtension' => [ 'MediaWiki' => $constraint, @@ -46,9 +45,8 @@ class CoreVersionCheckerTest extends PHPUnit_Framework_TestCase { * @dataProvider provideType */ public function testType( $given, $expected ) { - $checker = new VersionChecker(); + $checker = new VersionChecker( '1.0.0' ); $checker - ->setCoreVersion( '1.0.0' ) ->setLoadedExtensionsAndSkins( [ 'FakeDependency' => [ 'version' => '1.0.0', @@ -85,15 +83,14 @@ class CoreVersionCheckerTest extends PHPUnit_Framework_TestCase { * returns any error message. */ public function testInvalidConstraint() { - $checker = new VersionChecker(); + $checker = new VersionChecker( '1.0.0' ); $checker - ->setCoreVersion( '1.0.0' ) ->setLoadedExtensionsAndSkins( [ 'FakeDependency' => [ 'version' => 'not really valid', ], ] ); - $this->assertEquals( [ "Dependency FakeDependency provides an invalid version string." ], + $this->assertEquals( [ "FakeDependency does not have a valid version string." ], $checker->checkArray( [ 'FakeExtension' => [ 'extensions' => [ @@ -103,9 +100,8 @@ class CoreVersionCheckerTest extends PHPUnit_Framework_TestCase { ] ) ); - $checker = new VersionChecker(); + $checker = new VersionChecker( '1.0.0' ); $checker - ->setCoreVersion( '1.0.0' ) ->setLoadedExtensionsAndSkins( [ 'FakeDependency' => [ 'version' => '1.24.3', -- 2.20.1