Page MenuHomePhabricator

Factor code for handling for options (preferences) out of User class
Open, Needs TriagePublic

Description

Logic for loading and storing user options (aka preferences) should live in a dedicated service. Methods to be factored out of the User class include:

  • loadOptions
  • saveOptions
  • getDefaultOptions
  • getDefaultOption
  • getOption

We'll probably want a UserPreferences class that defines a value object that holds user preferences, and a UserPreferencesStore to return these:

  • getDefaultPreferences(): UserPreferences;
  • loadPreferences( UserIdentity $id ): UserPreferences;
  • savePreferences( UserIdentity $id, UserPreferences $pref );

UserPreferencesStore should cache options locally, but the cache can be very small (3 objects or something), since in nearly all cases, the only preferences needed are the ones for the user who is requesting the page. Alternatively, the service could be hard coded to only load and save the current user's preferences, or it could know the current user's name, and cache only the preferences associated with that user. Thinking this further, we may want to set up this service in a way that makes it impossible to access or modify another user's preferences during a web request, and reserve such activities to maintenance scripts.

Caveat: A PreferencesFactory service exists, but it constructs *forms*, and should probably be renamed to PreferencesUIFactory or some such.

Event Timeline

In the current state, memcache entries for User instances include their configured preferences as an array. Should this be kept for B/C during the refactor?

In the current state, memcache entries for User instances include their configured preferences as an array. Should this be kept for B/C during the refactor?

Very good point. My intuition is that this should be cached alongside the session data. But we'll probably want some kind of caching, yes.

Change 511339 had a related patch set uploaded (by TK-999; owner: TK-999):
[mediawiki/core@master] Decoupling: Create initial concept of PreferenceStore service

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

The above patch is an initial implementation of what such a preference service could look like. With this approach, a migration path could look something like this:

  • Migrate uses of deprecated User::get/setOption** accessors to the service
  • Once all uses of these accessors have been migrated, remove the accessors, remove User::saveSettings, and remove the "shouldUpdateUserTouched" param from the service interface
  • (in parallel to the above two points) Migrate deprecated hook handlers

What is the status of this work? It is currently on the contractor workboard, but I'm not sure that is correct.

The above patch is an initial implementation of what such a preference service could look like. With this approach, a migration path could look something like this:

  • Migrate uses of deprecated User::get/setOption** accessors to the service
  • Once all uses of these accessors have been migrated, remove the accessors, remove User::saveSettings, and remove the "shouldUpdateUserTouched" param from the service interface
  • (in parallel to the above two points) Migrate deprecated hook handlers

@TK-999 are you still working on this task?

@DannyS712 Thanks for reaching out :) Unfortunately this tasks seems to have stalled and I couldn't find the time to get back to it. I am not sure about next steps.

@DannyS712 Thanks for reaching out :) Unfortunately this tasks seems to have stalled and I couldn't find the time to get back to it. I am not sure about next steps.

It is currently listed as open - are you saying it should be stalled?

I've taken a stab on this, but it doesn't yet resolve all the issues pointed out in this issue. In particular, it doesn't yet include out-of-the process caching, plus a restructuring of preferencesFactory vs options management is not done yet either. I'd rather split those steps of the work, so not merging the tickets, but adding as a subtask for now.

Change 511339 abandoned by TK-999:
Decoupling: Create initial concept of PreferenceStore service

Reason:
Superseded by T248527.

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

Change 632812 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] Move User::requiresHTTPS to UserOptionsLookup

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

Change 632812 abandoned by DannyS712:

[mediawiki/core@master] Move User::requiresHTTPS to UserOptionsLookup

Reason:

the HTTPS-related hooks were removed already, and this should be done together with moving User::getStubThreshold()

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