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.

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 :)

@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?

In T112553#1695845, @GWicke wrote:

@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, to me it sounds like RequestContext would actually be easier to swap out for testing or sub-requests than any of the global options.

In T384#1698050, @GWicke wrote:

@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.

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.

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.

In T384#1700114, @GWicke wrote:

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.

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.

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

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.

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

• '''RFC meeting | 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/''' (TimStarling, 21:00:58)

• '''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)

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