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

daniel created this task.May 13 2019, 1:01 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 13 2019, 1:01 PM
TK-999 added a subscriber: TK-999.May 19 2019, 10:40 AM

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
CCicalese_WMF added a subscriber: CCicalese_WMF.EditedJan 17 2020, 9:01 PM

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?

Pchelolo added a subscriber: Pchelolo.EditedMar 27 2020, 3:37 PM

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