Page MenuHomePhabricator

Create a mechanism for SpecialPages and API modules to have dependencies injected into them
Closed, ResolvedPublic

Description

Problem

Instances of SpecialPage and ApiBase need to be able to use services from the container. However, there is currently not a way to inject services into these classes without making the method aware of the container. This makes testing difficult as the classes have to be tested with the container rather than mocked dependencies. This does not mean that SpecialPages and API modules need to become services themselves (and thus be a part of the container).

Proposals

Solution #1: Inject Container into Factory Closure

SpecialPageFactory::getPage() allows a Special Pages's definition to be a callable:

if ( is_callable( $rec ) ) {
  // Use callback to instantiate the special page
  $page = $rec();
}

we could change this to inject the container in the callable:

if ( is_callable( $rec ) ) {
  // Use callback to instantiate the special page
  $page = $rec( MediaWikiServices::getInstance() );
}

this would allow SpecialPages to be instantiated with an anonymous function (much like includes/ServiceWiring.php). SpecialPageFactory::$coreList would become a non-static and the SpecialPages with dependencies would be closures rather than strings.

An implementation would look like:

'EditTags' => function( ServiceContainer $container ) {
  return new \SpecialEditTags(
    $container->get( 'PermissionManager')
  );
}

Likewise in ApiModuleManager::instantiateModule allows a factory key to be defined as a callable:

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!"
    );
  }
}

we could change this to inject the container into the factory:

if ( $factory !== null ) {
  // create instance from factory
  $instance = call_user_func( $factory, $this->mParent, $name, MediaWikiServices::getInstance() );

  if ( !$instance instanceof $class ) {
    throw new MWException(
      "The factory function for module $name did not return an instance of $class!"
    );
  }
}

again, ApiMain::$Modules would become non-static and an implementation could look like this:

'revisiondelete' => [
  'class' => ApiRevisionDelete::class,
  'factory' => function ( ApiMain $main, $name, ServiceContainer $container ) : ApiRevisionDelete {
    return new ApiRevisionDelete(
      $main,
      $name,
      '',
      $container->get( 'PermissionManager' )
    );
  },
],
Solution #2: Implement an interface with a static method for injecting the container to create the object

A new interface for SpecialPage could be created like this:

interface ContainerInjectionInterface {

  public static function create( ServiceContainer $container );

}

then SpecialPageFactory::getPage() can be updated from this:

elseif ( is_string( $rec ) ) {
  $className = $rec;
  $page = new $className;
}

to something like this:

elseif ( is_string( $rec ) ) {
  $className = $rec;
  if ( is_subclass_of( $className, ContainerInjectionInterface::class ) ) {
    $page = $className::create( MediaWikiServices::getInstance() );
  } else {
    $page = new $className;
  }
}

an implementation might look like this:

class SpecialEditTags extends UnlistedSpecialPage implements ContainerInjectionInterface {

  public static function create( ServiceContainer $container ) {
    return new self(
      $container->get( 'PermissionManager' )
    );
  }

}

likewise, a new interface would be created for API modules:

interface ApiContainerInjectionInterface {

  public static function create( ServiceContainer $container, ApiMain $mainModule, $moduleName );

}

and ApiModuleManager::instantiateModule() would be updated from this:

// create instance from class name
$instance = new $class( $this->mParent, $name );

to this:

// create instance from class name
if ( is_subclass_of( $class, ApiContainerInjectionInterface::class ) ) {
  $instance = $class::create( MediaWikiServices::getInstance(), $this->mParent, $name );
} else {
  $instance = new $class( $this->mParent, $name );
}

and an implementation might look like this:

class ApiRevisionDelete extends ApiBase implements ApiContainerInjectionInterface {

  public static function create( ServiceContainer $container, ApiMain $mainModule, $moduleName ) {
    return new self(
      $mainModule,
      $moduleName,
      '',
      $container->get( 'PermissionManager' )
    );
  }

}
Solution #3: Allow Special Pages and API Modules to be services in the service container.

Instead of including the service wiring in SpecialPageFactory or ApiModuleManager instead add the service wiring to includes/ServiceWiring.php like any other service. Then the Special Page or API Module configuration would accept a reference to a service (that implements an interface?) rather than a reference to a class.

Chosen solution: Utilize ObjectFactory

With T222409: Standardize declarative object construction in MediaWiki, ObjectFactory now supports instantiating objects from a spec with services specified in the spec. It is now a service of its own, which allows it to be injected in any class that needs to construct objects that need services.

This transforms constructing code from

if ( is_callable( $rec ) ) {
	$page = $rec();
} elseif ( is_string( $rec ) ) {
	$page = new $rec;
}

to:

$page = $this->objectFactory->createObject(
	$rec,
	[
		'allowClassName' => true,
		'allowCallable' => true
	]
);

The spec notes which services are needed:

$rec = [
	'class' => SpecialAllPages::class,
	'services' => [
		'PagerFactory'
	]
];

ApiModuleManager::instantiateModule now takes the previously supported spec syntax for ApiModuleManager::addModules and applies it to ApiModuleManager::addModule, replacing the $class and $factory parameters with a single one that fulfills the function of both through the ObjectFactory spec.

Details

Related Gerrit Patches:
mediawiki/extensions/GoogleLogin : masterInject ObjectFactory into ApiModuleManager
mediawiki/extensions/ReadingLists : masterInject ObjectFactory into ApiModuleManager
mediawiki/extensions/Flow : masterInject ObjectFactory into ApiModuleManager
mediawiki/core : masterUse ObjectFactory to create API modules
mediawiki/core : masterSupport the creation of special pages with services injected

Event Timeline

dbarratt updated the task description. (Show Details)
Tgr added a subscriber: Tgr.

Ideally, we'd avoid injecting the container, as that's generally an antipattern in DI.

dmaza added a subscriber: dmaza.May 3 2019, 8:15 PM

I think that supporting the definition to be either a callable or a string would be ideal (so a combination of your suggestions). It will make it backward compatible and we can start migrating into DI in incremental changes. SpecialPageFactory::getPage() already supports a string or a callable so a "quick" change would be to make $coreList non-static and adjust the SpecialPageFactory accordingly.
I'm not sure about Api Modules 'cause I haven't seen how that works

Regarding Option 2 I don't think that the static factory method is suitable for this particular case. I think that's more useful for other things like object pooling or a more complex class hierarchy where object creation is more dynamic and/or complex. It might be an overkill for SpecialPages (to my knowledge at least)

Anomie moved this task from Unsorted to Blocked on the MediaWiki-API board.May 8 2019, 3:11 PM
Anomie added a subscriber: Anomie.

I think it would likely be best to block this on T222409: Standardize declarative object construction in MediaWiki, at which point we'll likely find that this has already been resolved.

I think the options here aren't considering the whole DI stack, as both are using MediaWikiServices::getInstance() when, with full DI, we'd actually need to be injecting that into SpecialPageFactory and ApiModuleManager too and we're likely to wind up with the antipattern @Tgr mentions.

daniel added a subscriber: daniel.May 8 2019, 6:31 PM

For the record, both SpecialPageFactory and ApiModuleManager can take an instantiator callback instead of a class name. This provides a way for extensions to registers models/pages that have their dependencies injected.

I see no advantage of an InjectorInterface over a simple callback. That's not to say that the existing mechanism can't be improved. I'm sure it can.

daniel moved this task from Inbox to Watching on the TechCom board.May 9 2019, 5:29 AM
abi_ awarded a token.May 13 2019, 8:48 AM
abi_ added a subscriber: abi_.
CCicalese_WMF triaged this task as Normal priority.May 14 2019, 12:59 PM

See T222409: Standardize declarative object construction in MediaWiki for a discussion of a generalized mechanism for allowing dependency injection when creating objects from some kind of non-code specification, as may be provided via extension.json.

dbarratt updated the task description. (Show Details)May 20 2019, 10:03 PM
In T222388#5198972, @dbarrat added:

Solution #3: Allow Special Pages and API Modules to be services in the service container.

Instead of including the service wiring in SpecialPageFactory or ApiModuleManager instead add the service wiring to includes/ServiceWiring.php like any other service. Then the Special Page or API Module configuration would accept a reference to a service (that implements an interface?) rather than a reference to a class.

IMO that would significantly bloat the service container with too many single-purpose services that no other caller should ever use. Keeping the wiring for individual special pages local to SpecialPageFactory and for individual API modules local to ApiModuleManager seems like the better organization to me. T222409 seems well on the way to solving the "lazy instantiation from wiring" question in a DI-sensible manner.

daniel added a comment.EditedMay 24 2019, 5:00 PM

IMO that would significantly bloat the service container with too many single-purpose services that no other caller should ever use. Keeping the wiring for individual special pages local to SpecialPageFactory and for individual API modules local to ApiModuleManager seems like the better organization to me. T222409 seems well on the way to solving the "lazy instantiation from wiring" question in a DI-sensible manner.

I agree that API modules and special pages should not be in the top level container. Factory/Registry services should act as second level of containers (for handler-type sub-services) that make use of some form of wiring. They are different from service containers in that all the sub-services implement the same interface, rather than distinct interfaces.

SlotRoleRegistry (rMW023fec5d7b3a1a4b57d7bd3c555eeb06fd6bae22) already uses the registry-as-a-lazy-factory-with-DI approach: it gets a bunch of callables injected. For REST Router it seems we are going to use a similar approach, with injecting the name of a JSON file which contains object declarations for use with ObjectFactory.

Change 534770 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/core@master] Support the creation of special pages with services injected

https://gerrit.wikimedia.org/r/534770

Change 534788 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/core@master] [POC] Use ObjectFactory to create API modules

https://gerrit.wikimedia.org/r/534788

@Mainframe98 would you mind updating the task description? I don't think it reflects the solution you've implemented. :)

Mainframe98 updated the task description. (Show Details)Sep 6 2019, 6:42 PM

Change 534944 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/extensions/GoogleLogin@master] Inject ObjectFactory into ApiModuleManager

https://gerrit.wikimedia.org/r/534944

Change 534945 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/extensions/Flow@master] Inject ObjectFactory into ApiModuleManager

https://gerrit.wikimedia.org/r/534945

Change 534946 had a related patch set uploaded (by Mainframe98; owner: Mainframe98):
[mediawiki/extensions/ReadingLists@master] Inject ObjectFactory into ApiModuleManager

https://gerrit.wikimedia.org/r/534946

Change 534770 merged by jenkins-bot:
[mediawiki/core@master] Support the creation of special pages with services injected

https://gerrit.wikimedia.org/r/534770

Change 534788 merged by jenkins-bot:
[mediawiki/core@master] Use ObjectFactory to create API modules

https://gerrit.wikimedia.org/r/534788

Anomie closed this task as Resolved.Sep 9 2019, 7:41 PM
Anomie assigned this task to Mainframe98.

Change 534945 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Inject ObjectFactory into ApiModuleManager

https://gerrit.wikimedia.org/r/534945

Change 534946 merged by jenkins-bot:
[mediawiki/extensions/ReadingLists@master] Inject ObjectFactory into ApiModuleManager

https://gerrit.wikimedia.org/r/534946

Change 534944 merged by jenkins-bot:
[mediawiki/extensions/GoogleLogin@master] Inject ObjectFactory into ApiModuleManager

https://gerrit.wikimedia.org/r/534944