Talk:Requests for comment/TitleValue

Previous page history was archived for backup purposes at Talk:Requests for comment/TitleValue/LQT Archive 1 on 2015-07-10.

At the 2014 architecture summit in January, we decided that Daniel Kinzler will help with the process to identify desired service interfaces, and that Nik will lead the discussion.

Brion and I are both against the use of boolean parameters in public interfaces, since they severely impair the readability of calling code. This is documented at Manual:Coding conventions/PHP#Functions and parameters. The $withNamespace and $withSection parameters fall under this restriction.

Ok, will amend.

You may want to consider mutator chaining, like in Message.

Hm... you could use a mutator to strip the section, but not to strip the namespace - there is always a namespace.

I think method names that start with a preposition are usually ambiguous and should be avoided. I think that this principle applies in this case, and I think that this method name should start with a verb, such as "create".

how about "deriveSectionTitle"?

That's not what "derive" means. I would prefer something more verbose like createTitleWithSection(), it's hard to be briefer than that without coining jargon.

createTitleWithSection() doesn't convey the fact that the new object will use the original object's page name, only replacing the section part. That's why I thought "derive" would fit: "Spam#Foo" is derived from "Spam#Bar" by changing the section.

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.

You say that you want to avoid refactoring of the Title class, but I would hope that the Title class would be refactored to use TitleValue/TitleFormatter/TitleParser where appropriate, instead of duplicating the whole system. That is, secureAndSplit() etc. would be moved into TitleParser instead of being duplicated there.

I was referring to changing the interface or contract of the Title class. You are right that the implementation should be changed to use the new code. However, in order to do so, Title needs access to TitleFormatter and TitleParser instances. This kind of B/C code is the rationale for the ServiceRegistry - I see no way to inject them (directly or indirectly via a RequestContext) without changing the signature of the newXXX and makeXXX pseudo-constructor methods in a very inconvenient way.

Of course, we could use the RequestContext singleton to get the services. But I don't see how that would be better, it would cause even more dependencies to be dragged in (everything RequestContext depends on, recursively).