Right now, GlobalPreferences is a bit hacky and could probably benefit from some core changes. Figure out how we could make the code less fragile and more future-proof, but without rewriting the whole preferences infrastructure.
Description
Description
Details
Details
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Niharika | T139145 global disabling of compact language list | |||
| Resolved | None | T16950 [Epic] Support global preferences on Wikimedia wikis | |||
| Resolved | Samwilson | T178267 Dynamically enable/disable the related pref when toggling "Set a local exception for this global preference" | |||
| Resolved | Samwilson | T178044 Add local exceptions to global prefs | |||
| Resolved | Samwilson | T68869 Update Global preferences UI | |||
| Resolved | Samwilson | T173476 Investigation: Figure out what needs to be rewritten in GlobalPreferences | |||
| Resolved | Samwilson | T178449 RFC: How to modify all preferences? |
Event Timeline
Comment Actions
@Legoktm, thoughts on this? I recall you said at one point that the extension was more of a proof of a concept. What are the more fragile/hacky parts of it that need some rewriting?
Comment Actions
One thing (I think) is the way GlobalPreferences attempts to be the last GetPreferences hook call; see T172029: Fix a gotcha with hook handlers order.
Comment Actions
My initial thoughts:
- Create a service in the core, called e.g. PreferencesProvider
- Move some stuff into it:
- Stuff from User class that operate on preferences and are contributing to it being a god class: getDefaultOptions(), loadOptions(), saveOptions(), etc.
- Make Preferences an instantiable class and make PreferencesProvider return it.
- Make core factory for PreferencesProvider overridable.
- Have an override in GlobalPreferences
Pros:
- Less reliance on hooks
- This extension avoids becoming an annoying analog of CentralAuth that embeds itself into too many things
- Way more stable unit tests
Cons:
- This is a relatively large rewrite, though it still can be argued whether it falls within this ticket's requirements
without rewriting the whole preferences infrastructure
Comment Actions
That sounds reasonable to me. How would fallback work to local preferences if the user doesn't have a global preference set for that option?
The other hacky part is the way that Special:GlobalPreferences is created, and how it uses the late hook and tries to add checkboxes for everything. I'd rather we subclass PreferencesForm, and create a custom HTMLFormField that adds the "use this globally" checkbox. I think Sam was looking into something similar? (given rMW05a7239ab382: Make it possible for subclasses to provide a different form)
Comment Actions
Sounds good Max.
And yeah, as part of T68869: Update Global preferences UI I've added GlobalPreferencesForm which so far doesn't do very much other than add a section header and change the form layout from table-based to div-based. I've experimented with overriding all the HTMLFormField types, in order to add a common parent to them which would then insert the extra checkbox.
The idea would be to have two PreferencesForm subclasses in this extension wouldn't it? GlobalPreferencesForm and LocalPreferencesForm? The former to make the form fields all use new custom subclasses of themselves, and the latter to add the links and help texts about what's global?
If we do customize the form class, then we can get rid of GlobalPreferences\Hooks::onGetPreferences() and don't need to introduce a new hook for processing the fully-built preferences array. Shall I do that in T68869?
Comment Actions
Change 370152 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/GlobalPreferences@master] [WiP] Refactor, clean up hooks, improve UI
Comment Actions
Change 374451 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/core@master] Make it possible to inject a different Preferences object
Comment Actions
Patch: https://gerrit.wikimedia.org/r/374451
As the first part of this, I've added a new hook to core, called PreferenceFactory. This will mean we can replace the Preferences object that's used by SpecialPreferences with our own LocalPreferences object, and so do whatever we want (i.e. modify the form and the preferences list appropriately).
Do you think this is an okay direction?
I realise that it's a bit of an odd way to treat the Preferences class, which is never (as it stands) instantiated. But I'd suggest that it should be, and that its static methods should be refctored (e.g. almost all of them use a context and/or a user, and these should be injected to the class). I've not done any great refactoring here, though, because I'm just doing the minimum required for the task at hand.
The patch I've got for GlobalPreferences https://gerrit.wikimedia.org/r/#/c/370152/ uses this new hook, so have a look at that to see what I'm on about.
Comment Actions
The MobileFrontend extension could perhaps also benefit from these changes, where it's overriding Special:Preferences. @Jdlrobson are you someone to ask to look at the above patch?
Comment Actions
Change 374451 abandoned by Samwilson:
Make it possible to inject a different Preferences object
Reason:
This change is being done in https://gerrit.wikimedia.org/r/#/c/389665/
Comment Actions
This patch is ready for review: https://gerrit.wikimedia.org/r/#/c/370152
and is now live on the commtech test wiki: http://localhost/wikimedia/wiki1/index.php/Special:GlobalPreferences
There are a bunch of things lacking still, but they're recorded under separate tickets. This one just focuses on switching to the new PreferencesFactory, and fixing for T68869: Update Global preferences UI.
Comment Actions
Change 370152 merged by jenkins-bot:
[mediawiki/extensions/GlobalPreferences@master] Switch to using PreferencesFactory, and improve UI