Topic on Talk:Requests for comment/TitleValue

Your proposal is to make ServiceRegistry a singleton, rather than a property of RequestContext? What is the rationale for that? There was a lot of venom directed at the singleton concept at the last architecture meeting.

I have detailed the rationale for this concrete case in the #Refactoring of the Title class section below.

In general though: having a global registry with limited scope, which is used where injection is not possible at the moment, seems acceptable (and unavoidable). Ideally, it would only be used to bootstrap the dependency injection from a static entry point. A typical example would be a static function handling a hook:

 public static final onSomeHook( $stuff ) {
     $someService = Registry::getDefaultInstance()->getSomeService();
     $anotherService = Registry::getDefaultInstance()->getAnotherService();

     $handler = new MySomeHookHandler( $someService, $anotherService );
     $handler->onSomeHook( $stuff );
 }

This allows the MySomeHookHandler class to be tested in isolation, using mock implementations of the services. The code that relies on global state would be minimal. Ideally, this would be the only way the global registry would be used. However, it may also be used for B/C code, as in the case of the Title object, as in the section below.

The alternative would be to use closures to handle the hook, and have the services, or at least the registry/factory, instantiated when the hook is registered:

 $registry = ...;
 $wgHooks['SomeHook'][] = function( $stuff ) use ( $registry ) {
     $someService = $registry->getSomeService();
     $anotherService = $registry->getAnotherService();

     $handler = new MySomeHookHandler( $someService, $anotherService );
     $handler->onSomeHook( $stuff );
 }

The main argument against using the RequestContext as the registry (or making the registry a member of RequestContext) is the "kitchen sink" anti-pattern: if access to all services is bundled in a single object, any code that needs any service will (at least formally) depend on every service, causing a holistic knot of interdependent code.

The argument is against passing the Registry, or a RequestContext, to MySomeHookHandler instead of the individual services is closely related: the Demeter Principle (ask for it, don't look for it). To any code that needs to instantiate MySomeHookHandler (e.g. a test case), it should be clear exactly what that class depends on, that is, what other classes it needs to operate. If it was asking for a "kitchen sink" registry, you will have to either provide everything, or you'd have to guess which services are actually needed. This means less isolation, and makes refactoring harder: if MySomeHookHandler is changed to require an additional dependency, adding a constructor parameter makes it easy to find and fix the code that needs to be adapted. If everything comes from the "kitchen sink" registry, it's hard to find the places where an incomplete registry might be used, so the requirements are not met (again, the prime example are unit tests).

The reason I suggest using RequestContext is for lifetime control. If you store an instance in a static variable, you are stuck with infinite lifetime, even beyond the end of a unit test unless you add special teardown code. And if bundling all services into a single object is bad, maybe I misunderstand the role of ServiceRegistry? I thought your proposed ServiceRegistry was a bundle of all service objects, that seemed to be implied by its name.

What I'm proposing is that the calling code would be similar:

public static final onSomeHook( $stuff ) {
    $someService = RequestContext::getMain()->getServiceRegistry()->getSomeService();

Or even identical:

class ServiceRegistry {
    static function getDefaultInstance() {
        return RequestContext::getMain()->getServiceRegistry()->getSomeService();
    }
}

You are right that the ServiceRegistry is again bundling all the services in a single object. The point is to restrict access to ServiceRegistry to "bootstrapping code" (e.g. in static hook handlers) only. RequestContext is used all over the code, so all the code is dependent on anything in the RequestContext. That makes me very very reluctant to add more stuff to it.

You are right about life time control. Essentially, I think about it this way:

• code that uses global state is not testable with unit tests
• so, try hard to avoid any access to global state
• and isolate the code that does need such access

In the example, the static onSomeHook would not be covered by unit tests, because it relies on global state (via Registry::getDefaultInstance() or RequestContext::getMain(), it doesn't matter). But the MySomeHookHandler could be fully tested, without elaborate fixtures or leaking state.