Page MenuHomePhabricator

ObjectFactory should allow using services
Closed, ResolvedPublic

Description

ObjectFactory is intended to be an object construction helper, but object construction is the responsibility of the dependency injection container, so the preferred method should be by specifying a service (typically for the factory, although there might be instances where the service is the actual class to be returned). There should be a parameter for that.

Rough plan:

  • make MediaWikiServices PSR-11 compatible (easy; just add get() and has() aliases, and make sure it and the exceptions it throws implement the right interfaces)
  • make ObjectFactory::getObjectFromSpec take a PSR-11 service container as an optional argument
  • add a service property to the spec array, to be used instead of class, and a factory_service property, to be used instead of factory. Not sure how the latter should look, as one would also need to specify what to call on the factory...

Details

Related Gerrit Patches:
mediawiki/libs/ObjectFactory : masterImplement spec 'services' for DI
mediawiki/core : masterHave ServiceContainer implement PSR-11
mediawiki/vendor : masterAdd PSR-11 (container interface)

Event Timeline

Tgr created this task.May 2 2019, 11:50 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 2 2019, 11:50 PM
Anomie added a subscriber: Anomie.May 3 2019, 5:42 PM

I doubt the cases where anything else is wanting to use ObjectFactory would really be suited to just pulling "a service" out of the container, as such code could as well just get the service directly from the DI container. And I think the type of "factory_service" that would be usable by ObjectFactory uses cases is very likely to be exactly the code that's trying to use ObjectFactory in the first place.

Perhaps you should invert the relationship: think of ObjectFactory as the code the DI container uses to create those objects based on the wiring configuration, at least when the wiring configuration is more complicated than just function ( $container ) {}, rather than trying to prefer having the wiring configuration assume the DI container.

Tgr added a comment.Jun 4 2019, 10:17 AM

Agreed, this would not be super useful to third parties. TBH I don't know if ObjectFactory is useful to third parties in the first place, especially now that PHP has the spread operator. So I'm not sure if that's something to be concerned about.

The DI tools I have seen typically take one of three approaches:

  • Class-based (e.g. Aura: the DI container has a map or class name => instantiation logic so you just need put the class name in your config). Often involves autowiring of some sort: e.g. the DIC uses reflection to look at paramater types and gets the corresponding service (e.g. PHP-DI or - in one possible usage pattern - Laravel), or maybe annotations are used to specify what service / config setting is expected. Works well with ObjectFactory-type declaratory object creation, but lacks flexibility (classes and services tend not to map 1:1) and autowiring tends to add a lot of complexity (it is typically slow enough to require some kind of build step). We have been intentionally avoiding this in our DI setup.
  • Declarative wiring (e.g. Symfony): you have some kind of static description (YAML, PHP arrays, some kind of fluent interface) of how to get a service. High-level classes like controllers get direct access to the DI container and pull services from there, the container checks the service spec and instantiates it. (This is I think what you mean by inverting the relationship?) Typically there's some way to use service names as parameters. (E.g. in Symfony if your parameter is a string that starts with @, it's interpreted as a service name, and replaced with the actual service object.) This is what ObjectFactory has been gesturing towards.
  • Functional wiring (e.g. Pimple): everything is happening in the wiring / setup file, and everything is created from a closure. We are moving in this direction with ServiceWiring.

So as I see we have a bit of a mismatch between the DI paradigm assumed by ObjectFactory and ServiceWiring. The declarative approach would be to have ObjectFactory contain the service container (as in your patch) or in some other way be closely associated with, and provide some way for declaring service arguments, e.g. [ 'class' => 'MyApiModule', 'args' => [ '@RevisionStore' ] ] (or [ 'class' => 'MyApiModule', 'args' => [ new ServiceDefinition( 'RevisionStore' ) ] ] for a little more type safety), which ObjectFactory could resolve using the container. Given that ObjectFactory is older and kind of randomly evolved, while ServiceWiring/ServiceContainer/MediaWikiServices is newer and TechCom-approved, this seems like moving in the wrong direction. I'm not sure what the right direction is, though, short of replacing all the declarative specs mentioned in T222409 with service wiring.

Anomie added a comment.Jun 4 2019, 8:39 PM

and provide some way for declaring service arguments, e.g. [ 'class' => 'MyApiModule', 'args' => [ '@RevisionStore' ] ] (or [ 'class' => 'MyApiModule', 'args' => [ new ServiceDefinition( 'RevisionStore' ) ] ] for a little more type safety), which ObjectFactory could resolve using the container.

That I could see a use for. If you want to rewrite this task to talk about that instead of the current idea of replacing 'class'/'factory', I'd support it.

It should be something representable in extension.json rather than requiring a ServiceDefinition object. Would we need to worry about the possibility that someone might want to use a string beginning with '@' as the actual parameter? An alternative would be something like

{
    "class": "FooBar",
    "args": [ "x", "y", "z" ],
    "services": [ "RevisionStore", "ActorMigration", "BlockManager" ]
}

which would result in the object being constructed as

new FooBar(
    $services->get( 'RevisionStore' ),
    $services->get( 'ActorMigration' ),
    $services->get( 'BlockManager' ),
    'x', 'y', 'z'
);

Given that ObjectFactory is older and kind of randomly evolved, while ServiceWiring/ServiceContainer/MediaWikiServices is newer and TechCom-approved, this seems like moving in the wrong direction. I'm not sure what the right direction is, though, short of replacing all the declarative specs mentioned in T222409 with service wiring.

I think that replacement you suggest would itself be the wrong direction. We don't need everything in MediaWiki to be a global public service any more than we need $wgUser, $wgParser, and so on for every service.

IMO ObjectFactory specs and MediaWikiServices are not as incompatible as you seem to think they are. I see no reason to restrict the possibilities for all wiring to only a single global array of function ( $services ) { ... }.

Change 514635 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] [PoC] Have ServiceContainer implement PSR-11

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

Legoktm added a subscriber: Legoktm.
  • make MediaWikiServices PSR-11 compatible (easy; just add get() and has() aliases, and make sure it and the exceptions it throws implement the right interfaces)

I was bored and did this for fun. Don't know if it'll be useful or not, let me know.

Change 516255 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/vendor@master] Add PSR-11 (container interface)

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

Change 516255 merged by jenkins-bot:
[mediawiki/vendor@master] Add PSR-11 (container interface)

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

Change 514635 merged by jenkins-bot:
[mediawiki/core@master] Have ServiceContainer implement PSR-11

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

Change 516523 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/libs/ObjectFactory@master] Implement spec 'services' for DI

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

Change 516523 merged by jenkins-bot:
[mediawiki/libs/ObjectFactory@master] Implement spec 'services' for DI

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

Tgr closed this task as Resolved.Jul 12 2019, 2:02 PM
Tgr assigned this task to Anomie.