Page MenuHomePhabricator

Introduce ContentLanguage service to replace $wgContLang
Closed, ResolvedPublic

Description

$wgContLang will be replaced by a service called ContLang or some similar name.

It may be best to only soft-deprecate in the initial commit, considering about 320 files in Gerrit-hosted extensions refer to the global. This means adding @deprecated to its doc comment in Setup.php.

166 files in core refer to $wgContLang. Ideally they should be updated to use the service.

Tests that use MediaWikiTestCase::setMwGlobals() can be migrated to use setService().

$wgParser is a prior example of this kind of project, but there are some issues with it which make it not an ideal example to follow. StubObject replaces the global variable with a direct object reference on the first __call() invocation, which is nice for type hinting and performance, but it means that to set the service, both the global variable and the service itself need to be reset. Existing test cases fail to do this correctly. For example, MessageTest just sets $wgParser with setMwGlobals() and apparently neglects to set the service with MediaWikiTestCase::setService(), meaning that the test would fail if MessageCache were migrated to use the service.

Implementation options for $wgContLang initial assignment:

  1. In Setup.php have simple assignment $wgContLang = MediaWikiServices::getInstance()->getContLang();.
  2. In Setup.php set $wgContLang to a StubObject similar to $wgParser
  3. In Setup.php set $wgContLang to a new kind of object proxy which forwards calls to MediaWikiServices::getInstance()->getContLang() without replacing the global

If 1 or 2 is used, the following implementation options for keeping $wgContLang in sync with the service occur to me:

  1. Introduce MediaWikiTestCase::setContLang() which will call both setMwGlobals() and setService()
  2. Have MediaWikiTestCase::setService() call setMwGlobals() if ContLang is to be set

Ultimately, I imagine $wgContLang will be hard-deprecated, by setting it to a DeprecatedGlobal or similar, allowing it to raise a deprecation warning when it is used. But that can be left for another subtask of T160815.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 24 2018, 6:21 AM

I don't see any way out of the possibility of the global and service falling out of sync. If something assigns a new value to the global, we can't detect or intercept it, and all existing references to the service will now be out of sync to references to the global. For tests, it seems best to have setService() call setMwGlobals(), and have setMwGlobals() throw if you try to use it directly, so that tests don't try to reset the global.

Given that, option 1 seems like the simplest way to go, unless we think lazy initialization is a useful performance gain here. If we want to detect direct setting of the global, we could add checks in MediaWikiServices to warn or throw if $wgContLang has a problematic value, but I don't know if it's worth the trouble.

@tstarling Do we need the initContLang() method? I don't see anything that overrides it, or any explanation of why it was added. 43b2fb56 added it as a branch merge without mention in the commit message. It looks like the commit in the branch that made the actual change also doesn't explain why it was added:

https://phabricator.wikimedia.org/rSVN15753#change-lx4YK1Qml5dI

It would simplify overriding the ContLang service if it was just any old Language object without any initialization method having to be called on it.

(Indeed, MediaWikiTestCase::setContentLang() already doesn't bother calling initContLang().)

Change 447820 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Use setContentLang() instead of setMwGlobals()

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

Change 447821 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Introduce ContLang service to replace $wgContLang

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

@tstarling So as with T200247, I don't know how to proceed with replacing the $wgContLang occurrences. Where should I inject the service? Am I really supposed to add a Language argument and member variable to hundreds of classes' constructors (or function parameters)? What's, e.g., User::isValidUserName() supposed to look like without just calling MediaWikiServices::getInstance() (which seems to defeat the point of not using globals)?

Change 447828 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/Babel@master] Use setContentLang instead of setMwGlobals

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

Change 447829 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/Echo@master] Use setContentLang instead of setMwGlobals

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

Change 447831 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/Wikibase@master] Use setContentLang instead of setMwGlobals

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

Change 447829 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Use setContentLang instead of setMwGlobals

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

Change 447820 merged by jenkins-bot:
[mediawiki/core@master] Use setContentLang() instead of setMwGlobals()

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

Change 447831 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use setContentLang instead of setMwGlobals

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

@tstarling Do we need the initContLang() method? I don't see anything that overrides it, or any explanation of why it was added. 43b2fb56 added it as a branch merge without mention in the commit message. It looks like the commit in the branch that made the actual change also doesn't explain why it was added:
https://phabricator.wikimedia.org/rSVN15753#change-lx4YK1Qml5dI
It would simplify overriding the ContLang service if it was just any old Language object without any initialization method having to be called on it.

No, get rid of it. Looking at that patch, I think I introduced it to replace LanguageEo::setAltEncoding(), but then I ended up just removing the feature after I tried to test it. RELEASE-NOTES says "Removed the broken altencoding feature." I'm not sure if I forgot to remove initContLang() or if I left it in there for future similar features, but it is definitely past its use-by date.

@tstarling So as with T200247, I don't know how to proceed with replacing the $wgContLang occurrences. Where should I inject the service? Am I really supposed to add a Language argument and member variable to hundreds of classes' constructors (or function parameters)? What's, e.g., User::isValidUserName() supposed to look like without just calling MediaWikiServices::getInstance() (which seems to defeat the point of not using globals)?

Use MediaWikiServices::getInstance(), which does not quite defeat the purpose. It still gives us handy things like MediaWikiServices::resetGlobalInstance() and formalises dependencies between services. More generally, this kind of project is an incremental step towards DI. The nice thing about having the service container is that you can introduce services one at a time, and leave the code mostly as it is, until you are ready to change it. When MediaWikiServices was introduced, it unblocked the DI project which was up to that point stalled by the daunting prospect of having to refactor the entire object model to make the slightest improvement.

For classes that are already services, like the many references to $wgContLang in ServiceWiring.php, you can do it properly from the outset. The more things that become services, the easier it becomes to implement DI.

Change 448093 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Update ServiceWiring to use ContLang service

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

Change 448094 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Mass conversion of $wgContLang to service

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

Change 447828 merged by jenkins-bot:
[mediawiki/extensions/Babel@master] Use setContentLang instead of setMwGlobals

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

Change 449024 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/Wikibase@master] Use setContentLang instead of setMwGlobals (more)

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

Change 449024 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use setContentLang instead of setMwGlobals (more)

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

Change 449243 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/Wikibase@master] Call overrideMwServices() before overriding services

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

Change 449261 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Update MagicWord to not use $wgContLang

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

Change 449262 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Update Parser to not use $wgContLang

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

Change 449263 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Update CoreParserFunctions to not use $wgContLang

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

Change 449264 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Update LinkHolderArray to not use $wgContLang

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

Change 449243 abandoned by simetrical:
Call overrideMwServices() before overriding services

Reason:
Superseded by 449235

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

Legoktm renamed this task from Introduce ContLang service to replace $wgContLang to Introduce ContentLanguage service to replace $wgContLang.Aug 1 2018, 9:06 AM

Change 447821 merged by jenkins-bot:
[mediawiki/core@master] Introduce ContentLanguage service to replace $wgContLang

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

Change 448093 merged by jenkins-bot:
[mediawiki/core@master] Update ServiceWiring to use ContentLanguage

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

Change 449261 merged by jenkins-bot:
[mediawiki/core@master] Update MagicWord to use ContentLanguage

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

Change 449262 merged by jenkins-bot:
[mediawiki/core@master] Update Parser to use ContentLanguage

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

Change 449263 merged by jenkins-bot:
[mediawiki/core@master] Update CoreParserFunctions to use ContentLanguage

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

Change 449264 merged by jenkins-bot:
[mediawiki/core@master] Update LinkHolderArray to use ContentLanguage

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

Change 448094 merged by jenkins-bot:
[mediawiki/core@master] Mass conversion of $wgContLang to service

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

tstarling closed this task as Resolved.Aug 14 2018, 6:54 AM

Change 487985 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/core@master] ApiQuerySiteinfoTest: Conversion of $wgContLang to service

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

Change 487985 merged by jenkins-bot:
[mediawiki/core@master] ApiQuerySiteinfoTest: Conversion of $wgContLang to service

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

Change 516586 had a related patch set uploaded (by Fomafix; owner: Fomafix):
[mediawiki/extensions/TemplateData@master] Use MediaWikiServices instead of $wgContLang

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

Change 516586 merged by jenkins-bot:
[mediawiki/extensions/TemplateData@master] Use MediaWikiServices instead of $wgContLang

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