Page MenuHomePhabricator

ApiQueryBlocks should not be aware of the container
Closed, ResolvedPublic

Description

Problem
In T219684 dependencies were added to ApiQueryBlocks and the class became aware of the container.

Solution
ApiQueryBlocks should have its dependencies injected into it.

Event Timeline

Anomie subscribed.

This task seems like a good idea in itself, but it assumes we have all the DI infrastructure already in the API which I'm not entirely sure actually exists (yeah, Technical-Debt). So a prerequisite might be to verify that what's already in the API is sufficient for the sort of DI that you're wanting to do here. I'll give an overview of what that is, then we can discuss if there seems to be anything missing.

The "service wiring" for creating API modules is handled by the ApiModuleManager class. The wiring configuration itself is done by PHP arrays: keys are the module names (e.g. the "block" in "action=block"), while values vary depending on how the module is to be instantiated:

  • Most modules currently just supply a class name here, e.g. ApiBlock::class. The module is instantiated by calling new $className( $parentModule, $name ) (where $parentModule is the ApiMain, or ApiQuery, or whatever "owns" the ApiModuleManager instance and $name is the key from the wiring config array).
  • The other option is to supply a PHP callable array (i.e. [ ClassName, staticMethod ]), which will be called with the same parameters. Inside that callable is where you'd inject any other dependencies. Note the code as written doesn't support Closures or 'ClassName::staticMethod'-style callables.

For API modules in core, the array is stored in a private static array in the "parent" module, e.g. https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/master/includes/api/ApiMain.php#89 for action=block. Extensions have two mechanisms they can use: either adding values to configuration globals such as $wgAPIModules, or using a hook such as 'ApiMain::moduleManager'. Each "parent" module would have its own global and hook.

So, that's the overview. If you have questions or want more details or clarifications, feel free to ask. Unfortunately I may not be able to reply right away, but I'll do my best.

I don't really like the fact that we have part of the array statically defined and another part included inside the constructor, it makes it even harder for people to find where the definition of a module lives. What would you say to moving the closures to static factory methods of their classes, so the "wiring" for these modules could still be in the self::$Modules initializer?

I don't really like the fact that we have part of the array statically defined and another part included inside the constructor, it makes it even harder for people to find where the definition of a module lives. What would you say to moving the closures to static factory methods of their classes, so the "wiring" for these modules could still be in the self::$Modules initializer?

Sounds good to me. Maybe the classes could implement an interface with a create() method that also passes in an instance of the container? That way they don't have to (again) rely on the global container?

Sounds good to me. Maybe the classes could implement an interface with a create() method that also passes in an instance of the container? That way they don't have to (again) rely on the global container?

Sorry I didn't get a chance to reply to this earlier. For the record, it looks like things have moved on and that is being answered by T222388 and T222409 now.

Zabe subscribed.

All services are being injected into ApiQueryBlocks, so this seems to be done.