Page MenuHomePhabricator

Investigation: Figure out what needs to be rewritten in GlobalPreferences
Closed, ResolvedPublic5 Story Points

Description

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.

Event Timeline

kaldari created this task.Aug 16 2017, 11:43 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 16 2017, 11:43 PM
kaldari triaged this task as Normal priority.Aug 16 2017, 11:46 PM
kaldari set the point value for this task to 5.

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

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.

MaxSem claimed this task.Aug 21 2017, 6:40 PM
MaxSem moved this task from Ready to In Development on the Community-Tech-Sprint board.
MaxSem added a subscriber: MaxSem.

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

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)

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?

Sounds like a plan!

MaxSem reassigned this task from MaxSem to Samwilson.Aug 28 2017, 11:11 PM

Change 370152 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/GlobalPreferences@master] [WiP] Refactor, clean up hooks, improve UI

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

Change 374451 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/core@master] Make it possible to inject a different Preferences object

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

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.

I asked for more input about this on MW:Manual talk:User preferences.

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?

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/

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

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.

Change 370152 merged by jenkins-bot:
[mediawiki/extensions/GlobalPreferences@master] Switch to using PreferencesFactory, and improve UI

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

MaxSem closed this task as Resolved.Jan 4 2018, 12:23 AM

Weeee!

TBolliger moved this task from Untriaged to Archive on the Community-Tech board.Jan 31 2018, 12:44 AM