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

diff --git a/includes/registration/ExtensionRegistry.php b/includes/registration/ExtensionRegistry.php
index 76d25b6..c5b2150 100644 (file)
--- 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 2a9401e..5aaaa1b 100644 (file)
--- 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 2bb1fe4..9ee5881 100644 (file)
--- a/tests/phpunit/includes/registration/VersionCheckerTest.php
+++ b/tests/phpunit/includes/registration/VersionCheckerTest.php
@@ -1,15 +1,14 @@
 <?php
 
 /**
- * @covers CoreVersionChecker
+ * @covers VersionChecker
  */
-class CoreVersionCheckerTest extends PHPUnit_Framework_TestCase {
+class VersionCheckerTest extends PHPUnit_Framework_TestCase {
        /**
         * @dataProvider provideCheck
         */
        public function testCheck( $coreVersion, $constraint, $expected ) {
-               $checker = new VersionChecker();
-               $checker->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',