Page MenuHomePhabricator

Complete TitleFactory to be a real factory service
Open, LowPublic

Description

As this comment says, TitleFactory is currently just a thin wrapper for Title:: static methods. However, we should make it a real factory, and deprecate static methods in the Title class.

I think that a possible implementation plan would be:

  • Remove external callers to Title::__construct / hard-deprecate external calls
  • Add parameters to Title::__construct for initializing things like mInterwiki, mNamespace etc.
  • Methods in TitleFactory would then use TitleParser::splitTitleString where applicable, and use the retval to build Title's

Note: this may be insufficient. I didn't check all methods in TitleFactory, nor all methods in Title. I chose not to investigate further, in case there's already a plan somewhere and I missed it.

Event Timeline

Adding those involved with original creation at https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/543918/
Explanation for current wrapper, from commit message:

Makes it possible to mock static Title methods in tests, where they are one of the more common reasons for not being able to use MediaWikiUnitTestCase.
Actually introducing dependency injection to Title is left for the future.

Change 579570 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] TitleFactory #1: Add params to Title::__construct, kill secureAndSplit

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

Change 579615 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] [WIP Make TitleFactory a real factory

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

The question is, will Title be around long enough for this to be worth the effort? I imagine at some point it will either be killed (in favor of TitleValue, PermissionManager, and a new Page / PageStore class) or almost all current functionality will be ripped out of it, but I have no idea if that's near or far. @daniel any thoughts?

The question is, will Title be around long enough for this to be worth the effort?

This is a good question.

I imagine at some point it will either be killed (in favor of TitleValue, PermissionManager, and a new Page / PageStore class) or almost all current functionality will be ripped out of it

Yeah, I guess this will happen at some point. I wonder if, even then, there might be some value in having a Factory class.

Anyway, writing the factory itself isn't too hard. Alternatively, we could avoid deprecating static methods in the Title class, and using TitleFactory only when it would help with e.g. unit tests. An example is https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/574261/, where we cannot add pure unit tests just because of the global state in Title.

Remove external callers to Title::__construct / hard-deprecate external calls

Direct calls to Title's constructor were never properly supported and should indeed be replaced throughout the code base. They should be rare anyway.

Add parameters to Title::__construct for initializing things like mInterwiki, mNamespace etc.

Could be done, but with the constructor being private, this is mostly cosmetic.

Methods in TitleFactory would then use TitleParser::splitTitleString where applicable, and use the retval to build Title's

Perhaps the code in the static pseudo-constructors in title should be moved into the factory class, and Title would then delegate to the factory? This is cleaner in a way, but also introduces a cyclic dependency.

Anyway, writing the factory itself isn't too hard. Alternatively, we could avoid deprecating static methods in the Title class, and using TitleFactory only when it would help with e.g. unit tests.

I don't see a strong reason to deprecate the static pseudo-constructors in Title and update calling code. As you said, calling code can be changed to use the factory if that aids testing. Otherwise, I'd leave it be. We'll eventually just deprecate the entire Title class. This is stuck mostly on two things: PageIdentity/PageStore, and ActingUser/Authority.

Direct calls to the Title constructor seems to be used primarily to construct empty dummies. Perhaps the factory should have a getDummyTitle() method instead. That should not create an empty Title though, but one pointing to Special:BadTitle.

Direct calls to Title's constructor were never properly supported and should indeed be replaced throughout the code base. They should be rare anyway.

We've already removed them in all WMF-deployed code. The constructor was marked as @protected, hence no-one should have called it directly to start with. This, together with the new stable interface, lead me to a "removal" (i.e. make private) without deprecation (https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/577770/), partly because it's hard to use wfDeprecated for external callers (see PS 6 of that patch).

Add parameters to Title::__construct for initializing things like mInterwiki, mNamespace etc.

Could be done, but with the constructor being private, this is mostly cosmetic.

It is. Having code like

$t = new Title();
$t->secureAndSplit();
// And inside secureAndSplit:
$this->setFragment( '#' . $parts['fragment'] );
$this->mInterwiki = $parts['interwiki'];
$this->mLocalInterwiki = $parts['local_interwiki'];
$this->mNamespace = $parts['namespace'];
$this->mDefaultNamespace = $defaultNamespace;
// ...

looks unnecessarily complicated. I've also tried to make it similar to the constructor of TitleValue.

Methods in TitleFactory would then use TitleParser::splitTitleString where applicable, and use the retval to build Title's

Perhaps the code in the static pseudo-constructors in title should be moved into the factory class, and Title would then delegate to the factory? This is cleaner in a way, but also introduces a cyclic dependency.

This is what I did in r579615. Yes, it does introduce a cyclic dependency, but I thought this is acceptable for BC code? I should also note that using services makes some unit tests fail. Of course, these tests aren't pure unit tests, but fixing failures is annoying. Perhaps we could keep the duplicated code in Title for the time being.

Anyway, writing the factory itself isn't too hard. Alternatively, we could avoid deprecating static methods in the Title class, and using TitleFactory only when it would help with e.g. unit tests.

I don't see a strong reason to deprecate the static pseudo-constructors in Title and update calling code. As you said, calling code can be changed to use the factory if that aids testing. Otherwise, I'd leave it be.

I believe this is perfectly fine. Adding or removing @deprecated annotations is very quick.

Change 579615 abandoned by Daimona Eaytoy:
[mediawiki/core@master] [WIP] Make TitleFactory a real factory

Reason:

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

I think this should probably be declined in favour of phasing out the Title class entirely.

I sent some patches to update existing code to stop creating TitleFactory manually ( https://gerrit.wikimedia.org/r/q/topic:%22titlefactory-not-newable%22+(status:open%20OR%20status:merged) ) and once those are merged can start moving logic from Title static constructors into the TitleFactory class, like for the User->UserFactory migration

Change 673799 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] TitleFactory: add getSelectFields and logic for newFromIDs

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

Change 773882 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] Title: Remove deprecated Title::getDefaultNamespace

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

Change 773882 merged by jenkins-bot:

[mediawiki/core@master] Title: Remove deprecated Title::getDefaultNamespace

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

Change 773929 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Replace Title::mDefaultNamespace with a constant

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

Change 773929 merged by jenkins-bot:

[mediawiki/core@master] Replace Title::mDefaultNamespace with a constant

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