From eb8823743c281fe72446ae06809467fd2746f7f7 Mon Sep 17 00:00:00 2001 From: mainframe98 Date: Fri, 6 Sep 2019 13:38:55 +0200 Subject: [PATCH] Use ObjectFactory to create API modules This will allow constructing API modules that need services. This overhauls some of the internals of the ApiModuleManager, but the public interface remains unchanged. The $class parameter of addModule, (now called $spec) also allows passing an array with the spec of the module. Note that this spec requires the attribute 'class' to be present, even when 'factory' is specified. This is the same as before, where $class was always required. In a perfect DI world ObjectFactory would be injected into ApiMain::__construct and ApiMain would pass that to its instance of ApiModuleManager, but that is currently not possible, so for now it is injected in ApiModuleManager by having ApiMain::__construct call the service locator. Bug: T222388 Change-Id: Iee04afc27283547dd68d6db93f44ac2e0ebf1258 --- RELEASE-NOTES-1.34 | 11 ++ includes/api/ApiMain.php | 5 +- includes/api/ApiModuleManager.php | 139 ++++++++---------- includes/api/ApiQuery.php | 6 +- .../includes/api/ApiModuleManagerTest.php | 67 +++++++-- .../includes/api/ApiQueryLanguageinfoTest.php | 18 ++- .../includes/api/format/ApiFormatTestBase.php | 12 +- 7 files changed, 157 insertions(+), 101 deletions(-) diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index 9ac26e8389..c7527ab8b6 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -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 === * … diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php index f0e0077e43..7bbce976aa 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -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' ); diff --git a/includes/api/ApiModuleManager.php b/includes/api/ApiModuleManager.php index d2df013c08..3d4ccaf0fa 100644 --- a/includes/api/ApiModuleManager.php +++ b/includes/api/ApiModuleManager.php @@ -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; diff --git a/includes/api/ApiQuery.php b/includes/api/ApiQuery.php index c78e445f42..a7ff729e84 100644 --- a/includes/api/ApiQuery.php +++ b/includes/api/ApiQuery.php @@ -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(); diff --git a/tests/phpunit/includes/api/ApiModuleManagerTest.php b/tests/phpunit/includes/api/ApiModuleManagerTest.php index b01b90e8ef..7747c70618 100644 --- a/tests/phpunit/includes/api/ApiModuleManagerTest.php +++ b/tests/phpunit/includes/api/ApiModuleManagerTest.php @@ -1,5 +1,8 @@ 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 ); + }, + ] + ); + } } diff --git a/tests/phpunit/includes/api/ApiQueryLanguageinfoTest.php b/tests/phpunit/includes/api/ApiQueryLanguageinfoTest.php index 6bbdd3bd53..771e0395c5 100644 --- a/tests/phpunit/includes/api/ApiQueryLanguageinfoTest.php +++ b/tests/phpunit/includes/api/ApiQueryLanguageinfoTest.php @@ -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 + ); + } + ] ); } ); diff --git a/tests/phpunit/includes/api/format/ApiFormatTestBase.php b/tests/phpunit/includes/api/format/ApiFormatTestBase.php index 27ec73a94c..789908c2a2 100644 --- a/tests/phpunit/includes/api/format/ApiFormatTestBase.php +++ b/tests/phpunit/includes/api/format/ApiFormatTestBase.php @@ -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' ); -- 2.20.1