Page MenuHomePhabricator
Create Task
Maniphest T200247

Introduce MagicWordFactory service
Closed, ResolvedPublic
Actions

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 Changes in Gerrit:
Show related patches Customize query in gerrit

Related Objects

Event Timeline

tstarling created this task.Jul 24 2018, 6:45 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 24 2018, 6:45 AM
Aklapper added a project: MediaWiki-General.Jul 24 2018, 5:42 PM
gerritbot subscribed.Jul 24 2018, 6:02 PM

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

gerritbot added a project: Patch-For-Review.Jul 24 2018, 6:02 PM
gerritbot added a comment.Jul 25 2018, 12:15 PM

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

gerritbot added a comment.Jul 25 2018, 12:15 PM

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

gerritbot added a comment.Jul 25 2018, 12:24 PM

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

gerritbot added a comment.Jul 25 2018, 12:35 PM

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

Simetrical added a comment.Jul 25 2018, 1:11 PM

@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.

gerritbot added a comment.Jul 25 2018, 1:25 PM

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

gerritbot added a comment.Jul 25 2018, 1:34 PM

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

Simetrical mentioned this in T200246: Introduce ContentLanguage service to replace $wgContLang.Jul 25 2018, 3:17 PM
gerritbot added a comment.Jul 26 2018, 12:19 PM

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 added a comment.Jul 26 2018, 12:47 PM
In T200247#4450272, @Simetrical wrote:

@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.

Simetrical added a comment.Jul 26 2018, 1:04 PM

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.

gerritbot added a comment.Jul 26 2018, 1:48 PM

Change 447786 abandoned by simetrical:
Update Parser to use MagicWordFactory

Reason:
Wrong approach

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

gerritbot added a comment.Jul 26 2018, 1:48 PM

Change 447786 restored by simetrical:
Update Parser to use MagicWordFactory

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

Simetrical added a comment.Jul 26 2018, 6:28 PM

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.)

tstarling added a comment.Jul 27 2018, 6:47 AM

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.

Legoktm added a comment.Jul 30 2018, 9:10 AM
In T200247#4454163, @Simetrical wrote:

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.

Legoktm added a comment.Jul 30 2018, 9:36 AM
In T200247#4454163, @Simetrical wrote:

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".

gerritbot added a comment.Jul 30 2018, 12:13 PM

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

gerritbot added a comment.Jul 30 2018, 12:36 PM

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

gerritbot added a comment.Jul 30 2018, 1:55 PM

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

gerritbot added a comment.Jul 30 2018, 1:55 PM

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

gerritbot added a comment.Jul 30 2018, 2:38 PM

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

gerritbot added a comment.Jul 30 2018, 2:47 PM

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

gerritbot added a comment.Jul 30 2018, 2:51 PM

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

gerritbot added a comment.Jul 30 2018, 2:56 PM

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

gerritbot added a comment.Jul 30 2018, 3:02 PM

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

gerritbot added a comment.Jul 30 2018, 3:02 PM

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

Simetrical mentioned this in T200701: Don't access MagicWord static member variables directly.Jul 30 2018, 3:08 PM
gerritbot added a comment.Jul 30 2018, 4:29 PM

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

tstarling moved this task from Backlog to In Progress on the Core-Platform-Team-Old (CPT-Q1-Jul-Sep-2018) board.Jul 30 2018, 11:25 PM
gerritbot added a comment.Jul 31 2018, 5:23 AM

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

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

gerritbot added a comment.Jul 31 2018, 5:32 AM

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

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

gerritbot added a comment.Jul 31 2018, 5:34 AM

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

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

gerritbot added a comment.Jul 31 2018, 5:35 AM

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

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

gerritbot added a comment.Jul 31 2018, 5:37 AM

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

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

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-07-31 (1.32.0-wmf.15)).Jul 31 2018, 6:01 AM
gerritbot added a comment.Jul 31 2018, 12:05 PM

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

gerritbot added a comment.Jul 31 2018, 12:46 PM

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

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

gerritbot added a comment.Jul 31 2018, 6:23 PM

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

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

ReleaseTaggerBot edited projects, added MW-1.32-notes (WMF-deploy-2018-08-07 (1.32.0-wmf.16)); removed MW-1.32-notes (WMF-deploy-2018-07-31 (1.32.0-wmf.15)).Jul 31 2018, 8:01 PM
gerritbot added a comment.Jul 31 2018, 9:51 PM

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

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

gerritbot added a comment.Aug 1 2018, 4:35 AM

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

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

gerritbot added a comment.Aug 1 2018, 4:53 AM

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

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

gerritbot added a comment.Aug 1 2018, 5:43 AM

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

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

gerritbot added a comment.Aug 1 2018, 8:11 AM

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

Reason:
Superseded by I49c507f3875e46a8e15fd2c28d61c17188aabffc

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

gerritbot added a comment.Aug 1 2018, 8:25 AM

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

Legoktm added a comment.Aug 1 2018, 9:37 AM

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.

gerritbot added a comment.Aug 1 2018, 10:02 AM

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

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

gerritbot added a comment.Aug 1 2018, 10:02 AM

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

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

gerritbot added a comment.Aug 1 2018, 10:03 AM

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).

gerritbot added a comment.Aug 3 2018, 12:24 PM

Change 449165 restored by Pwirth:
TESTING

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

gerritbot added a comment.Aug 3 2018, 3:36 PM

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

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

• CCicalese_WMF moved this task from In Progress to Closed on the Core-Platform-Team-Old (CPT-Q1-Jul-Sep-2018) board.Aug 6 2018, 9:45 PM
gerritbot added a comment.Apr 9 2019, 8:23 AM

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

gerritbot added a comment.Apr 9 2019, 10:15 AM

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

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

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.25; 2019-04-09).Apr 9 2019, 11:01 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits