Page MenuHomePhabricator

Determine purpose of GlobalVarConfig-backend entries in ConfigFactory (via wgConfigRegistry)
Open, MediumPublic

Description

As part of T187154#6034369 I noticed the following on enwiki in production:

"wgConfigRegistry": {
  "vector": "GlobalVarConfig::newInstance",
  "cite": "GlobalVarConfig::newInstance",
  "categorytree": "GlobalVarConfig::newInstance",
  "timedmediahandler": "GlobalVarConfig::newInstance",
  "CirrusSearch": "CirrusSearch\\SearchConfig::newFromGlobals",
  "globalcssjs": "GlobalVarConfig::newInstance",
  "globaluserpage": "GlobalVarConfig::newInstance",
  "ApiFeatureUsage": "GlobalVarConfig::newInstance",
  "templatestyles": "GlobalVarConfig::newInstance",
  "ArticleCreationWorkflow": "GlobalVarConfig::newInstance",
  "popups": "GlobalVarConfig::newInstance",
  "visualeditor": "GlobalVarConfig::newInstance",
  "citoid": "GlobalVarConfig::newInstance",
  "mobilefrontend": "GlobalVarConfig::newInstance",
  "minerva": "GlobalVarConfig::newInstance",
  "textextracts": "GlobalVarConfig::newInstance",
  "RelatedArticles": "GlobalVarConfig::newInstance",
  "revisionslider": "GlobalVarConfig::newInstance",
  "ExternalGuidance": "GlobalVarConfig::newInstance",
  "mwoauth": "GlobalVarConfig::newInstance",
  "quicksurveys": "GlobalVarConfig::newInstance",
  "PageViewInfo": "GlobalVarConfig::newInstance",
  "ReadingLists": "GlobalVarConfig::newInstance"
}

It seems a fairly small and arbitrary subset of extensions are using this to create their own copy of GlobalVarConfig, but I can't tell whether this is unguided copy-pasta or adding value for someone. E.g. what is the value today in Cite doing ConfigFactory::makeConfig('cite') compared to getMainConfig()?

Being able to have separate objects and multiple sources of configuration is certainly useful. And being able to read config from Etcd like we do in production already is an example of that. (ConfigFactory is great, I'm not suggesting we change anything about that).

But the way some extensions anticipate some imaginary future where extensions might have their own config source, is not serving a clear purpose to me. Especially since 1) afaik it is still to-be-determined how and when that would work and 2) there is no restrictions enforced right now on what this object should or shouldn't be capable of. That is, there is nothing stopping Cite from using its copy of the Config to also read out core configuration variables. I've already seen developers be confused by this, thinking the local object should be used for all config keys, including when you need those from core.

If there is an intentional and tangible benefit from this today, this task will track documenting that as a best-practice to eventually being adopted across the board.

I not, I suggest we remove this pattern from the few extensions that accidentally adopted it in favour of using the Config object from the Service container like everything else does.

See also:

Event Timeline

Krinkle renamed this task from Remove use of ConfigFactory entries backed by GlobalVarConfig (via wgConfigRegistry) to Determine purpose of GlobalVarConfig-backend entries in ConfigFactory (via wgConfigRegistry).Apr 6 2020, 10:44 PM
Anomie subscribed.

This is the practice currently recommended by https://www.mediawiki.org/wiki/Manual:Configuration_for_developers#Configuration_using_extension.json_(recommended). Extensions aren't supposed to use the core Config object to access the extension's configuration, and any doing so are taking advantage of behavior that isn't guaranteed to continue working in the future. But many extensions are probably just continuing to assume global variables instead of going through Config at all.

As far as I know, the intended purpose is indeed that "imaginary future where extensions might have their own config source". I don't know what benefit that future is supposed to bring versus one shared configuration namespace. I think @Legoktm is the person to ask.

Personally I wouldn't object to losing ConfigFactory in favor of one Config object providing that shared namespace.

The intention was that each extension would only its own config instance and eventually once the extension was switched to using Config we'd switch it away from GlobalVarConfig to....something else that was never built.

In practice this really hasn't worked because everything is global and everyone just grabs the main config instance and of course it works. I think we need to go back to the drawing board to figure out what we actually want now and how to achieve it.

See also T292402 and the dozens of SettingsLoader tasks linked from it. From what I've seen, there is nothing in those plans that suggests a separate source of config per extension, nor a specific use case or need for it.

So as to get us to a less confusing today before it becomes significantly more complex with SettingsLoader, I propose to remove use of ConfigRegistry and makeConfig from the few extensions that adopted it as early experiment.

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

[mediawiki/extensions/TimedMediaHandler@master] Consistently use MainConfig service in hooks

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

From what I've seen, there is nothing in those plans that suggests a separate source of config per extension, nor a specific use case or need for it.

You are correct, there is no plan to use this in SettingsLoader. We have been wondering about the same question and have decided that the ConfigFactory and different configs for different extensions should be removed. We just did not want to pile it into SettingsLoader project cause it's orthogonal.

Change 758985 merged by jenkins-bot:

[mediawiki/extensions/TimedMediaHandler@master] Consistently use MainConfig service in hooks

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

Changed by @Func is merged:

[mediawiki/skins/Vector] Drop the Vector.Config service

https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/777356

Krinkle triaged this task as Medium priority.EditedJun 2 2022, 2:35 PM
Krinkle added a project: Technical-Debt.
Krinkle moved this task from Unsorted to Migrate / Replace on the Technical-Debt board.
Krinkle added a project: good first task.
Krinkle added a subscriber: Func.

Feel free to ping me for any help or pointers, I'm happy to collab with someone who's interested in learning about MediaWiki config and extension registry.

MSantos subscribed.

Hey, I have a couple of questions. @Krinkle is this a good task for the codejam? Also, @Func are you still interested on working on this task?

@MSantos Yes, these are all fine to remove in favour of main config service as far as I'm concerned.