Merge "Use ObjectFactory to create API modules"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Mon, 9 Sep 2019 19:25:42 +0000 (19:25 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Mon, 9 Sep 2019 19:25:42 +0000 (19:25 +0000)
RELEASE-NOTES-1.34
includes/api/ApiMain.php
includes/api/ApiModuleManager.php
includes/api/ApiQuery.php
tests/phpunit/includes/api/ApiModuleManagerTest.php
tests/phpunit/includes/api/ApiQueryLanguageinfoTest.php
tests/phpunit/includes/api/format/ApiFormatTestBase.php

index 9ac26e8..c7527ab 100644 (file)
@@ -123,6 +123,9 @@ $wgPasswordPolicy['policies']['default']['PasswordNotInLargeBlacklist'] = false;
 * (T222388) Special pages can now be specified as an ObjectFactory spec,
   allowing the construction of special pages that require services to be
   injected in their constructor.
+* (T222388) API modules can now be specified as an ObjectFactory spec,
+  allowing the construction of modules that require services to be injected
+  in their constructor.
 
 === External library changes in 1.34 ===
 
@@ -161,6 +164,11 @@ $wgPasswordPolicy['policies']['default']['PasswordNotInLargeBlacklist'] = false;
   deprecated in 1.25, has been removed.
 
 === Action API internal changes in 1.34 ===
+* The exception thrown in ApiModuleManager::getModule has been changed
+  from an MWException to an UnexpectedValueException, thrown by ObjectFactory.
+  ApiModuleManager::getModule now also throws InvalidArgumentExceptions when
+  ObjectFactory is presented with an invalid spec or incorrectly constructed
+  objects.
 * …
 
 === Languages updated in 1.34 ===
@@ -517,6 +525,9 @@ because of Phabricator reports.
   'rc_user', 'log_user', and 'ipb_by' is deprecated. Queries should be adjusted
   to use the corresponding actor fields directly. Note that use with
   'rev_user' is *not* deprecated at this time.
+* Specifying both the class and factory parameters for
+  ApiModuleManager::addModule is now deprecated. The ObjectFactory spec should
+  be used instead.
 
 === Other changes in 1.34 ===
 * …
index f0e0077..7bbce97 100644 (file)
@@ -281,7 +281,10 @@ class ApiMain extends ApiBase {
                }
                $this->mResult->setErrorFormatter( $this->getErrorFormatter() );
 
-               $this->mModuleMgr = new ApiModuleManager( $this );
+               $this->mModuleMgr = new ApiModuleManager(
+                       $this,
+                       MediaWikiServices::getInstance()->getObjectFactory()
+               );
                $this->mModuleMgr->addModules( self::$Modules, 'action' );
                $this->mModuleMgr->addModules( $config->get( 'APIModules' ), 'action' );
                $this->mModuleMgr->addModules( self::$Formats, 'format' );
index d2df013..3d4ccaf 100644 (file)
@@ -21,6 +21,9 @@
  * @since 1.21
  */
 
+use MediaWiki\MediaWikiServices;
+use Wikimedia\ObjectFactory;
+
 /**
  * This class holds a list of modules and handles instantiation
  *
@@ -45,64 +48,35 @@ class ApiModuleManager extends ContextSource {
         * @var array[]
         */
        private $mModules = [];
+       /**
+        * @var ObjectFactory
+        */
+       private $objectFactory;
 
        /**
         * Construct new module manager
+        *
         * @param ApiBase $parentModule Parent module instance will be used during instantiation
+        * @param ObjectFactory|null $objectFactory Object factory to use when instantiating modules
         */
-       public function __construct( ApiBase $parentModule ) {
+       public function __construct( ApiBase $parentModule, ObjectFactory $objectFactory = null ) {
                $this->mParent = $parentModule;
+               $this->objectFactory = $objectFactory ?? MediaWikiServices::getInstance()->getObjectFactory();
        }
 
        /**
         * Add a list of modules to the manager. Each module is described
-        * by a module spec.
-        *
-        * Each module spec is an associative array containing at least
-        * the 'class' key for the module's class, and optionally a
-        * 'factory' key for the factory function to use for the module.
+        * by an ObjectFactory spec.
         *
-        * That factory function will be called with two parameters,
-        * the parent module (an instance of ApiBase, usually ApiMain)
-        * and the name the module was registered under. The return
-        * value must be an instance of the class given in the 'class'
-        * field.
+        * This simply calls `addModule()` for each module in `$modules`.
         *
-        * For backward compatibility, the module spec may also be a
-        * simple string containing the module's class name. In that
-        * case, the class' constructor will be called with the parent
-        * module and module name as parameters, as described above.
-        *
-        * Examples for defining module specs:
-        *
-        * @code
-        *  $modules['foo'] = 'ApiFoo';
-        *  $modules['bar'] = [
-        *      'class' => ApiBar::class,
-        *      'factory' => function( $main, $name ) { ... }
-        *  ];
-        *  $modules['xyzzy'] = [
-        *      'class' => ApiXyzzy::class,
-        *      'factory' => [ XyzzyFactory::class, 'newApiModule' ]
-        *  ];
-        * @endcode
-        *
-        * @param array $modules A map of ModuleName => ModuleSpec; The ModuleSpec
-        *        is either a string containing the module's class name, or an associative
-        *        array (see above for details).
+        * @see ApiModuleManager::addModule()
+        * @param array $modules A map of ModuleName => ModuleSpec
         * @param string $group Which group modules belong to (action,format,...)
         */
        public function addModules( array $modules, $group ) {
                foreach ( $modules as $name => $moduleSpec ) {
-                       if ( is_array( $moduleSpec ) ) {
-                               $class = $moduleSpec['class'];
-                               $factory = ( $moduleSpec['factory'] ?? null );
-                       } else {
-                               $class = $moduleSpec;
-                               $factory = null;
-                       }
-
-                       $this->addModule( $name, $group, $class, $factory );
+                       $this->addModule( $name, $group, $moduleSpec );
                }
        }
 
@@ -111,14 +85,21 @@ class ApiModuleManager extends ContextSource {
         * classes who wish to add their own modules to their lexicon or override the
         * behavior of inherent ones.
         *
+        * ObjectFactory is used to instantiate the module when needed. The parent module
+        * (`$parentModule` from `__construct()`) and the `$name` are passed as extraArgs.
+        *
+        * @since 1.34, accepts an ObjectFactory spec as the third parameter. The old calling convention,
+        *  passing a class name as parameter #3 and an optional factory callable as parameter #4, is
+        *  deprecated.
         * @param string $name The identifier for this module.
         * @param string $group Name of the module group
-        * @param string $class The class where this module is implemented.
-        * @param callable|null $factory Callback for instantiating the module.
+        * @param string|array $spec The ObjectFactory spec for instantiating the module,
+        *  or a class name to instantiate.
+        * @param callable|null $factory Callback for instantiating the module (deprecated).
         *
         * @throws InvalidArgumentException
         */
-       public function addModule( $name, $group, $class, $factory = null ) {
+       public function addModule( $name, $group, $spec, $factory = null ) {
                if ( !is_string( $name ) ) {
                        throw new InvalidArgumentException( '$name must be a string' );
                }
@@ -127,16 +108,24 @@ class ApiModuleManager extends ContextSource {
                        throw new InvalidArgumentException( '$group must be a string' );
                }
 
-               if ( !is_string( $class ) ) {
-                       throw new InvalidArgumentException( '$class must be a string' );
-               }
+               if ( is_string( $spec ) ) {
+                       $spec = [
+                               'class' => $spec
+                       ];
 
-               if ( $factory !== null && !is_callable( $factory ) ) {
-                       throw new InvalidArgumentException( '$factory must be a callable (or null)' );
+                       if ( is_callable( $factory ) ) {
+                               // Uncomment this when callers are cleaned up:
+                               // wfDeprecated( __METHOD__ . ' with $class and $factory', '1.34' );
+                               $spec['factory'] = $factory;
+                       }
+               } elseif ( !is_array( $spec ) ) {
+                       throw new InvalidArgumentException( '$spec must be a string or an array' );
+               } elseif ( !isset( $spec['class'] ) ) {
+                       throw new InvalidArgumentException( '$spec must define a class name' );
                }
 
                $this->mGroups[$group] = null;
-               $this->mModules[$name] = [ $group, $class, $factory ];
+               $this->mModules[$name] = [ $group, $spec ];
        }
 
        /**
@@ -153,7 +142,7 @@ class ApiModuleManager extends ContextSource {
                        return null;
                }
 
-               list( $moduleGroup, $moduleClass, $moduleFactory ) = $this->mModules[$moduleName];
+               list( $moduleGroup, $spec ) = $this->mModules[$moduleName];
 
                if ( $group !== null && $moduleGroup !== $group ) {
                        return null;
@@ -164,7 +153,7 @@ class ApiModuleManager extends ContextSource {
                        return $this->mInstances[$moduleName];
                } else {
                        // new instance
-                       $instance = $this->instantiateModule( $moduleName, $moduleClass, $moduleFactory );
+                       $instance = $this->instantiateModule( $moduleName, $spec );
 
                        if ( !$ignoreCache ) {
                                // cache this instance in case it is needed later
@@ -179,28 +168,22 @@ class ApiModuleManager extends ContextSource {
         * Instantiate the module using the given class or factory function.
         *
         * @param string $name The identifier for this module.
-        * @param string $class The class where this module is implemented.
-        * @param callable|null $factory Callback for instantiating the module.
+        * @param array $spec The ObjectFactory spec for instantiating the module.
         *
-        * @throws MWException
+        * @throws UnexpectedValueException
         * @return ApiBase
         */
-       private function instantiateModule( $name, $class, $factory = null ) {
-               if ( $factory !== null ) {
-                       // create instance from factory
-                       $instance = call_user_func( $factory, $this->mParent, $name );
-
-                       if ( !$instance instanceof $class ) {
-                               throw new MWException(
-                                       "The factory function for module $name did not return an instance of $class!"
-                               );
-                       }
-               } else {
-                       // create instance from class name
-                       $instance = new $class( $this->mParent, $name );
-               }
-
-               return $instance;
+       private function instantiateModule( $name, $spec ) {
+               return $this->objectFactory->createObject(
+                       $spec,
+                       [
+                               'extraArgs' => [
+                                       $this->mParent,
+                                       $name
+                               ],
+                               'assertClass' => $spec['class']
+                       ]
+               );
        }
 
        /**
@@ -213,8 +196,8 @@ class ApiModuleManager extends ContextSource {
                        return array_keys( $this->mModules );
                }
                $result = [];
-               foreach ( $this->mModules as $name => $grpCls ) {
-                       if ( $grpCls[0] === $group ) {
+               foreach ( $this->mModules as $name => $groupAndSpec ) {
+                       if ( $groupAndSpec[0] === $group ) {
                                $result[] = $name;
                        }
                }
@@ -229,9 +212,9 @@ class ApiModuleManager extends ContextSource {
         */
        public function getNamesWithClasses( $group = null ) {
                $result = [];
-               foreach ( $this->mModules as $name => $grpCls ) {
-                       if ( $group === null || $grpCls[0] === $group ) {
-                               $result[$name] = $grpCls[1];
+               foreach ( $this->mModules as $name => $groupAndSpec ) {
+                       if ( $group === null || $groupAndSpec[0] === $group ) {
+                               $result[$name] = $groupAndSpec[1]['class'];
                        }
                }
 
@@ -247,7 +230,7 @@ class ApiModuleManager extends ContextSource {
         */
        public function getClassName( $module ) {
                if ( isset( $this->mModules[$module] ) ) {
-                       return $this->mModules[$module][1];
+                       return $this->mModules[$module][1]['class'];
                }
 
                return false;
index c78e445..a7ff729 100644 (file)
@@ -20,6 +20,7 @@
  * @file
  */
 
+use MediaWiki\MediaWikiServices;
 use Wikimedia\Rdbms\IDatabase;
 
 /**
@@ -134,7 +135,10 @@ class ApiQuery extends ApiBase {
        public function __construct( ApiMain $main, $action ) {
                parent::__construct( $main, $action );
 
-               $this->mModuleMgr = new ApiModuleManager( $this );
+               $this->mModuleMgr = new ApiModuleManager(
+                       $this,
+                       MediaWikiServices::getInstance()->getObjectFactory()
+               );
 
                // Allow custom modules to be added in LocalSettings.php
                $config = $this->getConfig();
index b01b90e..7747c70 100644 (file)
@@ -1,5 +1,8 @@
 <?php
 
+use MediaWiki\MediaWikiServices;
+use Wikimedia\ObjectFactory;
+
 /**
  * @covers ApiModuleManager
  *
@@ -12,7 +15,8 @@ class ApiModuleManagerTest extends MediaWikiTestCase {
        private function getModuleManager() {
                $request = new FauxRequest();
                $main = new ApiMain( $request );
-               return new ApiModuleManager( $main );
+
+               return new ApiModuleManager( $main, MediaWikiServices::getInstance()->getObjectFactory() );
        }
 
        public function newApiLogin( $main, $action ) {
@@ -28,30 +32,55 @@ class ApiModuleManagerTest extends MediaWikiTestCase {
                                null,
                        ],
 
-                       'with factory' => [
+                       'with class and factory' => [
                                'login',
                                'action',
                                ApiLogin::class,
                                [ $this, 'newApiLogin' ],
                        ],
 
-                       'with closure' => [
-                               'logout',
+                       'with spec (class only)' => [
+                               'login',
                                'action',
-                               ApiLogout::class,
-                               function ( ApiMain $main, $action ) {
-                                       return new ApiLogout( $main, $action );
-                               },
+                               [
+                                       'class' => ApiLogin::class
+                               ],
+                               null,
+                       ],
+
+                       'with spec' => [
+                               'login',
+                               'action',
+                               [
+                                       'class' => ApiLogin::class,
+                                       'factory' => [ $this, 'newApiLogin' ],
+                               ],
+                               null,
                        ],
+
+                       'with spec (using services)' => [
+                               'logout',
+                               'action',
+                               [
+                                       'class' => ApiLogout::class,
+                                       'factory' => function ( ApiMain $main, $action, ObjectFactory $objectFactory ) {
+                                               return new ApiLogout( $main, $action );
+                                       },
+                                       'services' => [
+                                               'ObjectFactory'
+                                       ],
+                               ],
+                               null,
+                       ]
                ];
        }
 
        /**
         * @dataProvider addModuleProvider
         */
-       public function testAddModule( $name, $group, $class, $factory = null ) {
+       public function testAddModule( $name, $group, $spec, $factory ) {
                $moduleManager = $this->getModuleManager();
-               $moduleManager->addModule( $name, $group, $class, $factory );
+               $moduleManager->addModule( $name, $group, $spec, $factory );
 
                $this->assertTrue( $moduleManager->isDefined( $name, $group ), 'isDefined' );
                $this->assertNotNull( $moduleManager->getModule( $name, $group, true ), 'getModule' );
@@ -327,4 +356,22 @@ class ApiModuleManagerTest extends MediaWikiTestCase {
                        $moduleManager->getClassName( 'nonexistentmodule' )
                );
        }
+
+       /**
+        * @expectedException \InvalidArgumentException
+        * @expectedExceptionMessage $spec must define a class name
+        */
+       public function testAddModuleWithIncompleteSpec() {
+               $moduleManager = $this->getModuleManager();
+
+               $moduleManager->addModule(
+                       'logout',
+                       'action',
+                       [
+                               'factory' => function ( ApiMain $main, $action ) {
+                                       return new ApiLogout( $main, $action );
+                               },
+                       ]
+               );
+       }
 }
index 6bbdd3b..771e039 100644 (file)
@@ -44,14 +44,16 @@ class ApiQueryLanguageinfoTest extends ApiTestCase {
                                        $moduleManager->addModule(
                                                'languageinfo',
                                                'meta',
-                                               ApiQueryLanguageinfo::class,
-                                               function ( $parent, $name ) use ( $microtimeFunction ) {
-                                                       return new ApiQueryLanguageinfo(
-                                                               $parent,
-                                                               $name,
-                                                               $microtimeFunction
-                                                       );
-                                               }
+                                               [
+                                                       'class' => ApiQueryLanguageinfo::class,
+                                                       'factory' => function ( $parent, $name ) use ( $microtimeFunction ) {
+                                                               return new ApiQueryLanguageinfo(
+                                                                       $parent,
+                                                                       $name,
+                                                                       $microtimeFunction
+                                                               );
+                                                       }
+                                               ]
                                        );
                                }
                        );
index 27ec73a..789908c 100644 (file)
@@ -27,7 +27,6 @@ abstract class ApiFormatTestBase extends MediaWikiTestCase {
         *  - class: If set, register 'name' with this class (and 'factory', if that's set)
         *  - factory: Used with 'class' to register at runtime
         *  - returnPrinter: Return the printer object
-        * @param callable|null $factory Factory to use instead of the normal one
         * @return string|array The string if $options['returnPrinter'] isn't set, or an array if it is:
         *  - text: Output text string
         *  - printer: ApiFormatBase
@@ -44,8 +43,15 @@ abstract class ApiFormatTestBase extends MediaWikiTestCase {
                $context->setRequest( new FauxRequest( $params, true ) );
                $main = new ApiMain( $context );
                if ( isset( $options['class'] ) ) {
-                       $factory = $options['factory'] ?? null;
-                       $main->getModuleManager()->addModule( $printerName, 'format', $options['class'], $factory );
+                       $spec = [
+                               'class' => $options['class']
+                       ];
+
+                       if ( isset( $options['factory'] ) ) {
+                               $spec['factory'] = $options['factory'];
+                       }
+
+                       $main->getModuleManager()->addModule( $printerName, 'format', $spec );
                }
                $result = $main->getResult();
                $result->addArrayType( null, 'default' );