Page MenuHomePhabricator

Introduce MagicWordFactory service
Closed, ResolvedPublic

Description

A MagicWordFactory service should be introduced to replace MagicWord::get().

The static members of MagicWord such as $mVariableIDsInitialised and getVariableIDs() should become non-static members of the MagicWordFactory instance.

The MagicWord static methods should be hard-deprecated and callers in core should be migrated.

The public static variables in MagicWord should be removed without deprecation. This should be announced on wikitech-l per deprecation policy. An extension survey indicates that only BlueSpiceFoundation and PageTools will be broken by this, they both assign to a public static variable. PageTools can use a *.i18n.magic.php file instead. BlueSpiceFoundation can use a GetDoubleUnderscoreIDs hook.

Details

Related Gerrit Patches:
mediawiki/core : masterHard-deprecate MagicWord static methods
mediawiki/extensions/BlueSpiceFoundation : masterDo not access MagicWord static variables directly
mediawiki/extensions/Flow : masterDon't use deprecated MagicWord static methods
mediawiki/extensions/ParserFunctions : masterDon't use deprecated MagicWord static methods
mediawiki/core : masterConvert remaining MagicWord:: calls to MagicWordFactory
mediawiki/core : masterUpdate a couple of tests to use MagicWordFactory
mediawiki/core : masterUpdate CoreParserFunctions to use MagicWordFactory
mediawiki/extensions/Wikibase : masterDon't use deprecated MagicWord static methods
mediawiki/extensions/WikiEditor : masterDon't use deprecated MagicWord static methods
mediawiki/core : masterUpdate MagicWordArray to use MagicWordFactory
mediawiki/extensions/GeoData : masterDon't use MagicWord static methods
mediawiki/extensions/MobileFrontend : masterDon't construct our own parser
mediawiki/core : masterMagicWordFactory to replace MagicWord static members/methods
mediawiki/core : masterUpdate Parser to use MagicWordFactory
mediawiki/extensions/Translate : masterParser constructor needs second argument in 1.32
mediawiki/extensions/PageTools : masterDo not access MagicWord static variables directly

Event Timeline

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

Change 447647 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] MagicWordFactory to replace MagicWord static members/methods

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

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

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

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

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

Change 447788 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Update MagicWordArray to use MagicWordFactory

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

Change 447791 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/GeoData@master] Update TagTest to use MagicWordFactory

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

@tstarling, what do you want me to do about things like InfoAction or ApiQuerySiteinfo? They use static MagicWord methods, but it doesn't seem like it would be easy to give them a MagicWordFactory member that the caller creates. Should they use MediaWikiServices (which seems to defeat the point of the exercise), or create their own MagicWordFactory, or what?

If we have nothing in core calling MediaWikiServices::getInstance()->getMagicWordFactory(), it seems like we could eliminate MagicWordFactory::clearCache() entirely, which would be nice.

Change 447798 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/MobileFrontend@master] Parser constructor needs second argument in 1.32

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

Change 447799 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Update a couple of tests to use MagicWordFactory

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

Change 448030 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/Translate@master] Parser constructor needs second argument in 1.32

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

@tstarling, what do you want me to do about things like InfoAction or ApiQuerySiteinfo? They use static MagicWord methods, but it doesn't seem like it would be easy to give them a MagicWordFactory member that the caller creates. Should they use MediaWikiServices (which seems to defeat the point of the exercise), or create their own MagicWordFactory, or what?
If we have nothing in core calling MediaWikiServices::getInstance()->getMagicWordFactory(), it seems like we could eliminate MagicWordFactory::clearCache() entirely, which would be nice.

As I said in T200246, just use MediaWikiServices::getInstance() in non-service classes. In fact I don't think I agree with you adding a MagicWordFactory to the Parser constructor in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/447786 , that is not really a helpful step. When we make Parser into a service, it will not be directly constructed with "new" by calling code, the only way to make one will be with a factory service (ParserFactory or some such) that has a MagicWordFactory dependency in its service wiring. The complexity of DI constructor parameters is supposed to be confined to ServiceWiring.php and the factory classes, we don't want to have everything that needs a Parser to be gathering together 20 different dependencies and passing them as type hinted constructor parameters.

Hmm, okay. Then I don't understand what the eventual plan is. https://www.mediawiki.org/wiki/Dependency_Injection#Concepts_and_Best_Practices says that when a service (or application logic) needs access to another service, it asks for it in the constructor. So what's that supposed to look like? And how is Parser not a service class, insofar as you can in fact get one from MediaWikiServices?

Perhaps a discussion on IRC would be a good idea, if you're around right now.

Change 447786 abandoned by simetrical:
Update Parser to use MagicWordFactory

Reason:
Wrong approach

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

Change 447786 restored by simetrical:
Update Parser to use MagicWordFactory

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

So I think I need an explanation of exactly how you want classes to access these services like MagicWordFactory or ContLang, since apparently I don't understand how DI is supposed to work. Let's say in Parser -- how should it have access to these two services, other than just going to MediaWikiServices? The more detail you provide, the more likely I'll be able to write patches in a timely fashion, given that we have a one- or two-day lag in communications.

Or, if there's anyone in a more compatible timezone who understands what I'm supposed to be doing, maybe I could talk to them on IRC. I work from about 2:00-9:30 PM in UTC+0300, so most places other than Australia should overlap with me.

(Reminder: I work on Sunday rather than Friday and will see what you say this Sunday.)

You can talk to @Legoktm about it on IRC, he should be around, 9:30pm for you would be 11:30am for him, and he has a similar outlook to me on these questions.

Or, if there's anyone in a more compatible timezone who understands what I'm supposed to be doing, maybe I could talk to them on IRC. I work from about 2:00-9:30 PM in UTC+0300, so most places other than Australia should overlap with me.
(Reminder: I work on Sunday rather than Friday and will see what you say this Sunday.)

Sorry I missed you on IRC yesterday, I'm just going to stay up a bit later today to hopefully overlap with you.

So I think I need an explanation of exactly how you want classes to access these services like MagicWordFactory or ContLang, since apparently I don't understand how DI is supposed to work. Let's say in Parser -- how should it have access to these two services, other than just going to MediaWikiServices? The more detail you provide, the more likely I'll be able to write patches in a timely fashion, given that we have a one- or two-day lag in communications.

I don't think it's valuable to have everything that wants to create a new Parser class be required to pass in a MagicWordFactory - mostly because even though Parser technically a service, it's not practically used as one yet.

Instead I'd suggest just doing $this->mwFactory = MediaWikiServices::getInstance()->getMagicWordFactory() unconditionally in the constructor. It's not actually DI, but we still get a decent amount of benefits to make it worthwhile. Things like mocking during testing is straightforward and now possible. And whenever someone does turn Parser into a proper DI service, then it'll be trivial for them to inject the MagicWordFactory in. We could implement a Parser->getMagicWordFactory() function so that parser functions/tags can get the factory instead of needing to get it from the service container.

So I think the changes to Parser.php are fine (minus the constructor), and then the other places you injected the service in should be undone/backed out.

On a slightly more lame note, whenever I read "mwFactory" I only see "MediaWiki factory" - at least for me it would be clearer if it just said "magicWordFactory".

Change 449165 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/BlueSpiceFoundation@master] Do not access MagicWord static variables directly

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

Change 449173 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/PageTools@master] Do not access MagicWord static variables directly

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

Change 447791 abandoned by simetrical:
Don't construct our own parser

Reason:
I don't think we want to deprecate this after all.

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

Change 447798 abandoned by simetrical:
Don't construct our own parser

Reason:
I don't think we want to deprecate this after all.

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

Change 449205 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Convert remaining MagicWord:: calls to MagicWordFactory

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

Change 449206 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/Flow@master] Don't use MagicWord static methods

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

Change 449208 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/GeoData@master] Don't use MagicWord static methods

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

Change 449209 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/Wikibase@master] Don't use MagicWord static methods

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

Change 449165 abandoned by simetrical:
TESTING

Reason:
I can't easily figure this out, so I'll file a bug instead.

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

Change 449215 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Hard-deprecate MagicWord static methods

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

Change 449244 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/WikiEditor@master] Don't use MagicWord static methods

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

Change 449173 merged by jenkins-bot:
[mediawiki/extensions/PageTools@master] Do not access MagicWord static variables directly

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

Change 447791 restored by Tim Starling:
Don't construct our own parser

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

Change 447647 merged by jenkins-bot:
[mediawiki/core@master] MagicWordFactory to replace MagicWord static members/methods

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

Change 447798 restored by Tim Starling:
Don't construct our own parser

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

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

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

Change 448030 abandoned by simetrical:
Parser constructor needs second argument in 1.32

Reason:
We don't want callers passing the second argument to the Parser constructor, so since this can't use the global parser, it will have to stick with just "new Parser()" until we have a proper factory function.

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

Change 447798 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Don't construct our own parser

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

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

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

Change 449208 merged by jenkins-bot:
[mediawiki/extensions/GeoData@master] Don't use MagicWord static methods

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

Change 447788 merged by jenkins-bot:
[mediawiki/core@master] Update MagicWordArray to use MagicWordFactory

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

Change 449244 merged by jenkins-bot:
[mediawiki/extensions/WikiEditor@master] Don't use deprecated MagicWord static methods

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

Change 449209 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Don't use deprecated MagicWord static methods

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

Change 447799 abandoned by simetrical:
Update a couple of tests to use MagicWordFactory

Reason:
Superseded by I49c507f3875e46a8e15fd2c28d61c17188aabffc

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

Change 449676 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/extensions/ParserFunctions@master] Don't use deprecated MagicWord static methods

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

The MagicWord static methods should be hard-deprecated and callers in core should be migrated.

@tstarling to clarify, are you advocating for hard deprecation in 1.32? These functions (specifically ::get) are pretty common, so I (and MaxSem on Gerrit) think we should follow the standard practice of just soft deprecating now, and then hard deprecating after at least one more release.

Change 449206 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Don't use deprecated MagicWord static methods

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

Change 449676 merged by jenkins-bot:
[mediawiki/extensions/ParserFunctions@master] Don't use deprecated MagicWord static methods

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

Change 449205 merged by jenkins-bot:
[mediawiki/core@master] Convert remaining MagicWord:: calls to MagicWordFactory

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

Simetrical closed this task as Resolved.Aug 1 2018, 11:09 AM

Looks like this is all merged except for the hard deprecation of MagicWord static methods (https://gerrit.wikimedia.org/r/c/mediawiki/core/+/449215).

Change 449165 restored by Pwirth:
TESTING

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

Change 449165 merged by jenkins-bot:
[mediawiki/extensions/BlueSpiceFoundation@master] Do not access MagicWord static variables directly

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

Change 449215 had a related patch set uploaded (by simetrical; owner: simetrical):
[mediawiki/core@master] Hard-deprecate MagicWord static methods

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

Change 449215 merged by jenkins-bot:
[mediawiki/core@master] Hard-deprecate MagicWord static methods

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