Page MenuHomePhabricator

RfC: Dependency Injection for MediaWiki core
Open, NormalPublic

Description

This RFC describes some principles to adopt to make use of dependency injection in MediaWiki. Details and examples are described at https://www.mediawiki.org/wiki/Requests_for_comment/Dependency_injection. The core of the proposal is to adopt the following as best practice:

  1. When a service (or application logic) needs access to another a service, it asks for it in the constructor. This is the actual injection of dependencies.
  2. Objects that need access to services can only be constructed via factories (not directly using the new operator).
  3. Services are constructed by other services (factories and registries being special types of service). At the top of this chain of registries/factories there is the application scope service locator which acts as the top level service registry.
  4. Access to global default instances ("singletons") should be restricted to static entry points (e.g. hook handlers and callbacks in bootstrap code). Ideally, there is only one such global default instance, namely the service locator.
  5. Injecting/passing around factories, registries, and especially "kitchen sinks" like RequestContext should be avoided. The service locator should never be passed as a parameter.
  6. Mutable global state should especially be avoided.
  7. Services should be represented by narrow interfaces (e.g. UserLookup).
  8. Registries use constructor callbacks (aka factory functions) to instantiate services.
  9. Bootstrap code should avoid instantiating services, but define constructor callback instead.

These principles effectively provide dependency injection "on foot", without the need for a DI framework or a declarative syntax for networks of service objects. Decisions regarding the life cycle of service objects (lazy initialization, etc) are left to plain old PHP code.

Initial code experiments:

NOTE: Proposal reworked October 2015. Please see the full version of the proposal, including example code, at https://www.mediawiki.org/wiki/Requests_for_comment/Dependency_injection

Details

Reference
fl577

Event Timeline

flimport raised the priority of this task from to High.
flimport set Reference to fl577.

qgil wrote on 2014-09-03 06:04:45 (UTC)

This RFC has been scheduled to be discussed in the Architecture RfC meeting on 2014-09-10.

Qgil edited projects, added TechCom-RFC; removed Architecture.Oct 22 2014, 8:45 PM
tstarling set Security to None.
daniel moved this task from Inbox to Backlog on the TechCom-RFC board.Feb 27 2015, 3:43 PM
daniel claimed this task.
daniel added subscribers: daniel, AndyRussG.

I'm claiming this for myself since I want to push for DI to happen.
I'm not supporting the RFC as currently stated, as I would prefer something more light weight.

daniel lowered the priority of this task from High to Normal.Apr 9 2015, 10:41 AM

IRC Meeting summary from 2014-09-10 had "ACTION: parent5446 to do some profiling of alternative solutions and publish findings"

The Flow and OOUIPlayground extensions use Pimple.

Proposing this for the dev summit 2016. I think we really need agreement on if and how we want to do DI. It needs quite a bit of work however to form the basis of a useful discussion.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 30 2015, 4:07 PM
bd808 added a subscriber: bd808.Sep 30 2015, 5:20 PM
daniel updated the task description. (Show Details)Oct 1 2015, 4:51 PM
daniel moved this task from Backlog to Inbox on the TechCom-RFC board.
daniel updated the task description. (Show Details)Oct 1 2015, 5:15 PM

Looks good, and essentially describes what we've been doing in Wikibase during the last two years.

Services should be represented by narrow interfaces (e.g. UserLookup).

This point might need clarification. I can imagine very non-narrow UserLookup interfaces :)

daniel added a comment.Oct 2 2015, 9:41 AM

@JeroenDeDauw look at the full version at https://www.mediawiki.org/wiki/Requests_for_comment/Dependency_injection where the code for the interface is linked.

Perhaps I should make the link in the description more obvious... where would you put it?

@daniel, thanks! I might be missing something, but to me it looks like the service registry is still request-global? Isn't that roughly the same as using a singleton, except one level up?

Yes, the top level service registry has a global default instance. There is no way to avoid this entirely. The idea is limit the negative impact by

  • pushing access to statics instances up the stack as far as possible (by only using them in static entry points).
  • reduce the number of global service/registries (ideally leaving only one in the end).
  • keeping the logic in static context minimal - all the static handler function does is access the global service registry, get the service it needs, set up a handler instance, and call the non-static, testable handler method.

The idea is to have a single static top level registry instance that you use in two places only:

  • in static hook handler functions (with minimal logic)
  • in constructor callbacks defined in bootstrap code
daniel updated the task description. (Show Details)Oct 2 2015, 10:16 AM
phuedx added a subscriber: phuedx.Oct 2 2015, 10:19 AM

@daniel, to me it sounds like RequestContext would actually be easier to swap out for testing or sub-requests than any of the global options.

Tgr added a subscriber: Tgr.Oct 3 2015, 12:49 AM
daniel added a comment.Oct 3 2015, 8:40 PM

@daniel, to me it sounds like RequestContext would actually be easier to swap out for testing or sub-requests than any of the global options.

It's not really RequestContext vs globals. In the places where you would access global service singletons, you would also access a global singleton of RequestContext. In the places where you can inject a RequestContext, you can just as well inject specific services.

The idea is that you never have to swap out any globals for testing for testing, because the only code that ever touches globals is trivial bootstrap code. So fort testing, you provide a fake of what is requested by the constructor, no more, no less. For example, see the unit test for ParserLimitHookHandlersTest.php.

Perhaps I should add an example to the RFC page to illustrate this point.

We could of course inject a fake RequestContext for unit testing. But that fake RequestContext would need to contain fakes of all the things RequestContext knows about - which is quite a lot.

Or we could reduce the usage to RequestContext to static entry points. RequestContext would essentially act as the global top level service registry. That would work fine from the DI standpoint. But it's not really what RequestContext was made for, and it would perhaps be better to start fresh.

Qgil added a comment.Oct 3 2015, 8:57 PM

Congratulations! This is one of the 52 proposals that made it through the first deadline of the Wikimedia-Developer-Summit-2016 selection process. Please pay attention to the next one: > By 6 Nov 2015, all Summit proposals must have active discussions and a Summit plan documented in the description. Proposals not reaching this critical mass can continue at their own path out of the Summit.

This comment was removed by GWicke.

We could of course inject a fake RequestContext for unit testing. But that fake RequestContext would need to contain fakes of all the things RequestContext knows about - which is quite a lot.

All we really care about is getting a reference to an object implementing the interface we expect. In my experience, passing in such a reference explicitly is easier to reason about, test and extend, and offers better type safety than a global. Implementations often use techniques like delegation for reuse, and operations like cloning an existing context state for unit tests can be nicely encapsulated (rather than manually messing with globals).

Whether RequestContext as-is is implementing the right interface is a separate question. I do think though that it's worth considering to evolve it into the request context interface we want, rather than working around it with new globals.

daniel added a comment.EditedOct 3 2015, 9:33 PM

All we really care about is getting a reference to an object implementing the interface we expect. In my experience, passing in such a reference explicitly is easier to reason about, test and extend, and offers better type safety than a global. Implementations often use techniques like delegation for reuse, and operations like cloning an existing context state for unit tests can be nicely encapsulated (rather than manually messing with globals).

I absolutely agree. I never said I want global singletons. I hate global singletons. I don't think we can get rid of them completely, but we should have as few as possible (one), and reduce access to them to a minimum (static entry points).

Whether RequestContext as-is is implementing the right interface is a separate question. I do think though that it's worth considering to evolve it into the request context interface we want, rather than working around it with new globals.

RequestContext is a nice example of the kitchen sink anti-pattern. We should not inject something that depends on everything, dependencies are transitive. We should use narrow, specific interfaces that represent exactly the functionality needed. That's my point against RequestContext.

Tgr added a comment.Oct 5 2015, 12:03 AM

There is a PSR standard for a request context; the problem is MediaWiki's RequestContext isn't anything like that (and doesn't have much to do with the request context - how is the skin part of the request context, for example?) It's a classic monster object - basically a service locator that gets injected (something that should never be done if you do dependency injection).

To give a similar example, you can create a

class GlobalWrapper {
    public function getFoo() {
        global $wgFoo;
        return $wgFoo;
    }
    // ......
}

class, inject it everywhere, and claim you are not using globals anymore, but it would only be true in the most literal sense, and not in any useful one (even if it would improve type safety a bit). RequestContext is pretty much like that. It makes unit tests less useful because they test classes in isolation which is only useful if there is some level of isolation at runtime, and RequestContext pierces that.

daniel added a comment.Oct 5 2015, 7:35 AM

@Tgr thanks for the excellent explanation of what's wrong with RequetsContext!

daniel updated the task description. (Show Details)Oct 7 2015, 8:16 PM
daniel renamed this task from RfC: Dependency injection to RfC: Dependency Injection for MediaWiki core.Oct 7 2015, 8:46 PM

As of this writing, I've put this on the top of the agenda for E84: Dependency Injection for MediaWiki core (RFC Meeting 2015-10-28). See comments in E84 for more about the meeting.

daniel updated the task description. (Show Details)Oct 28 2015, 5:50 PM
daniel updated the task description. (Show Details)Oct 28 2015, 6:04 PM
Mattflaschen-WMF updated the task description. (Show Details)
daniel updated the task description. (Show Details)Oct 28 2015, 7:47 PM
daniel updated the task description. (Show Details)Oct 28 2015, 8:46 PM
daniel updated the task description. (Show Details)
Spage added a comment.EditedOct 30 2015, 2:13 AM

We held an RFC office hour meeting about this, E84: Dependency Injection for MediaWiki core (RFC Meeting 2015-10-28), see summary, full log. No conclusion reached, but people are willing to develop an implementation. Daniel will update the RFC's wiki page.

Notes from the RFC meeting on October 28

Meeting started by TimStarling at 21:00:17 UTC. The full logs are
available at
https://tools.wmflabs.org/meetbot/wikimedia-office/2015/wikimedia-office.2015-10-28-21.00.log.html
.

Meeting summary

  • '''Dependency Injection for MediaWiki core | Wikimedia meetings channel | Please note: Channel is logged and publicly posted (DO NOT REMOVE THIS NOTE) | Logs: http://bots.wmflabs.org/~wm-bot/logs/%23wikimedia-office/ (Meeting topic: RFC meeting)''' (TimStarling, 21:09:45)
    • ''LINK:'' https://www.mediawiki.org/wiki/Requests_for_comment/Dependency_injection (TimStarling, 21:09:54)
    • ''LINK:'' https://gerrit.wikimedia.org/r/#/c/245483/ (DanielK_WMDE_, 21:13:31)
    • ''LINK:'' http://pimple.sensiolabs.org/ (parent5446, 21:17:26)
    • ''ACTION:'' daniel to hack up a code experiemnt porting an api module or special page to constructor based injection using the service locator. (DanielK_WMDE_, 21:18:10)
    • ''ACTION:'' daniel to check out http://pimple.sensiolabs.org/ (DanielK_WMDE_, 21:21:27)
    • Krinkle: there is no difference between a service locator object and a dependency injection container; the difference is in how you use them (DanielK_WMDE_, 21:25:09)
    • ''ACTION:'' DanielK_WMDE_ to write up a short blurb about what the salient differences between Pimple and his proposal are (TimStarling, 21:33:51)
    • ''ACTION:'' daniel to look into refactoring an all-static class like ObjectCache, Interwiki, or Linker as an example (DanielK_WMDE_, 21:46:26)
    • ''ACTION:'' mention replacement for getContext()->getFoo in extension code (which in turn replaced direct $wgFoo access) (DanielK_WMDE_, 21:53:34)
    • bd808 does not agree with "Objects that need access to services can only be constructed via factories" (bd808, 21:53:59)
    • ''ACTION:'' DanielK_WMDE_ mention existing related implementations (Pimple , includes/libs/ObjectFactory.php , IEG, Wikibase?) in RFC (spagewmf, 22:01:15)

Meeting ended at 22:01:44 UTC.

Action items

  • daniel to hack up a code experiemnt porting an api module or special page to constructor based injection using the service locator.
  • daniel to check out http://pimple.sensiolabs.org/
  • DanielK_WMDE_ to write up a short blurb about what the salient differences between Pimple and his proposal are
  • daniel to look into refactoring an all-static class like ObjectCache, Interwiki, or Linker as an example
  • mention replacement for getContext()->getFoo in extension code (which in turn replaced direct $wgFoo access)
  • DanielK_WMDE_ mention existing related implementations (Pimple , includes/libs/ObjectFactory.php , IEG, Wikibase?) in RFC

Action items, by person

  • DanielK_WMDE_
    • DanielK_WMDE_ to write up a short blurb about what the salient differences between Pimple and his proposal are
    • DanielK_WMDE_ mention existing related implementations (Pimple , includes/libs/ObjectFactory.php , IEG, Wikibase?) in RFC
  • UNASSIGNED
    • daniel to hack up a code experiemnt porting an api module or special page to constructor based injection using the service locator.
    • daniel to check out http://pimple.sensiolabs.org/
    • daniel to look into refactoring an all-static class like ObjectCache, Interwiki, or Linker as an example
    • mention replacement for getContext()->getFoo in extension code (which in turn replaced direct $wgFoo access)

People present (lines said)

  • DanielK_WMDE_ (76)
  • brion (43)
  • gwicke (39)
  • TimStarling (39)
  • Krinkle (30)
  • bd808 (29)
  • tgr (27)
  • ottomata (23)
  • parent5446 (21)
  • RoanKattouw (17)
  • aude (13)
  • ori (10)
  • robla (9)
  • paravoid (8)
  • spagewmf (6)
  • _joe_ (4)
  • wm-labs-meetbot (3)
  • legoktm (2)
  • AaronSchulz (1)
  • addshore (1)
  • jzerebecki (1)
  • cscott (1)

Generated by MeetBot 0.1.4 (http://wiki.debian.org/MeetBot)

daniel moved this task from Inbox to Under discussion on the TechCom-RFC board.Nov 4 2015, 9:20 PM
Izno added a subscriber: Izno.Nov 20 2015, 3:41 PM
Addshore added a subscriber: Addshore.

Status update: The discussion at https://gerrit.wikimedia.org/r/#/c/245483/ indicates that extensibility is a requirement for the service locator / DI container. This means that there should be generic getService/setService methods, so extensions can use the same container as core. A code proposal exists at https://gerrit.wikimedia.org/r/#/c/251007/

If we want an extensible / configurable container, we could use something existing, like Pimple. In the spirit of framework isolation, it would however be good to hide Pimple (or whatever we use internally) from extensions and other code that wants to access services.

Wikimedia Developer Summit 2016 ended two weeks ago. This task is still open. If the session in this task took place, please make sure 1) that the session Etherpad notes are linked from this task, 2) that followup tasks for any actions identified have been created and linked from this task, 3) to change the status of this task to "resolved". If this session did not take place, change the task status to "declined". If this task itself has become a well-defined action which is not finished yet, drag and drop this task into the "Work continues after Summit" column on the project workboard. Thank you for your help!

Next step for resolving this: add a service locator to MEdiaWiki, to provide a "composition root" for the dependency injection. This is being tracked as T124792: RFC: Service Locator for MediaWiki core.

Session notes: https://etherpad.wikimedia.org/p/WikiDev16-T119032 (scroll down to "Dependency injection"). Transcript:


  • Rationale: avoid strong coupling, decouple dev, improve testabiltiy, and higher re-use
  • Services with narrow interfaces
  • Needs a top level service that glues them all together
  • Recent RFC discussion about this. Some consensus was reached.
    • How should services configure their containers?
    • Should services be able to detect what wiring they need? Some people are against this
  • Shall we use a pre existing framework or roll our own?
    • Current frameworks do too much. Not worth the pain of depending on a third party

Questions:

  • Timo: Avoid gloval variable rather than singletons
  • Daniel: Avoid access to static variables and global scope. We don't want to modify all the hook signatures
  • Chad: Avoid 3rd party framework if we will only use a fraction of it. PHP tends to do auto-wiring with reflection. What is the overhead of this? Biggest comparsion is java
  • Daniel: It would happen on every request. Orginal RFC suggested auto-wiring with performance being a big concern. Too much magic.
  • Chad: Avoid auto-wiring because its too magical. Can be nice for quick writing of scode but the magic can be scary.
  • Daniel: I agree
  • Timo: We've had to remove reflection in codebase
  • [missed question]
  • Kunal: Main concern is how we're goign to get people to use this. No one is using TitleValue
  • Daniel: Several problems. Doesn't have all the info from the page table. With a DI framework you would have hub for services
  • ChrisS: Auth plugin kind of worked but became a bear to work with. If we do them right then yes but we design them with a rigid interface that ends up not working well. Caution agaisnt extensions depending on these interfaces. Vote no due to auth plugin experience
  • Daniel : Maybe an interim solution of a whilelist of what can be replaces
  • ChrisS: Need to document the security properties and wether is standard docs or within the class itself
  • KatieF: Would be nice to define a formatter for these new types coming from extensions
  • Timo: Using a library as a pattern but not a framework
  • RobL: How do we more forward with these two areas?
  • Daniel: What should the role of arch comittee be with this?
  • Greg: Why is the arch comittee not driving this forward?
  • RobL: They are overseeing it but others need to be involved
  • Daniel: We need time and resources. It won't immediately help build shiny new features. No one is responsible for building this kind of thing
  • Katie: As people have time we can tackle this
  • Chad: Not hearing any real objections but instead a resourcing issue. No one truly own nebulous parts of core like this. Should we give the arch committe the resources to own this?
  • RobL: Whenever we have a hard question we say that we should just throw resources at it. This is a cop-out. We don't have huge budget to do these things. Some of these don't require full time resources
  • Daniel: Need to follow principles and not make the problem worse
  • Greg: We need to make this a project that a team owns. Need to elevate arch decisions to the same level as user facing features
  • Brad: Speaking to lack of resources, and also the "team autonomy" point in the SOA discussion earlier: We used to have a team that was nominally responsible for stuff like this. But the WMF Engineering Reorg of Doom™ seemed to be aiming for more team autonomy in creating these "verticals" and killing the teams that used to work across the vertical lines like this.
  • Ori: We're too insular to working with enterprise users
daniel moved this task from Under discussion to Backlog on the TechCom-RFC board.Jan 26 2016, 5:29 PM
He7d3r added a subscriber: He7d3r.Apr 2 2016, 3:12 PM
Addshore moved this task from incoming to monitoring on the Wikidata board.May 11 2016, 2:07 PM
daniel moved this task from Inbox to Revisit on the User-Daniel board.Jan 30 2017, 2:05 PM
daniel moved this task from Revisit to Project on the User-Daniel board.
Osnard added a subscriber: Osnard.Feb 9 2018, 6:57 AM