Page MenuHomePhabricator

GlobalPreferences blocks API changes to preference table (mediawiki.user saveOption function doesn't work)
Closed, ResolvedPublic5 Estimated Story PointsBUG REPORT

Description

Background

We ran into issues relating to a new feature and we or other teams are likely to run it into again. Why this should be fixed:

  • This delayed our project roll out as was not intuitive and is likely to delay others in future.
  • To workaround it we add to write double the code we normally would have done. Other teams are likely to copy that approach leading to additional technical debt.
  • Some lesser used features may be incompatible with GlobalPreferences because they haven't realized this bug is present in their software.

Steps to replicate the issue (include links if applicable):

What happens?:

I get error:

Option `skin` is globally overridden. You can use the \"globalpreferences\" API to change the option globally, or the \"globalpreferenceoverrides\" API to set a local override.

What should have happened instead?:

My intent is pretty obvious here. GlobalPreferences should handle it - it should call the API for me to set up a local override.
At the very least it should throw an error with error code so I can handle this error case.

Developer notes

  • The existing saveOption client side function should take into account GlobalPreferences one way or the other. Suggest using hook or providing an alternative client side library inside extension.
var api = new mw.Api();
       api.saveOption( 'skin', 'vector-2022' )
  • The client side function may need knowledge of which preferences are global and which are not.
  • The API should throw an error when an attempt to save a preference that is a global preference is made.

Workaround

The problem can be worked around with the following code:

var api = new mw.Api();
 api.saveOption( 'skin', 'vector-2022' ).then( function (r) {
     try { 
         if ( r.warnings.options.warnings.indexOf('globalpreferenceoverrides') > -1 ) {
             return api.postWithToken( 'csrf',
                 { action: 'globalpreferenceoverrides', optionname: 'skin', optionvalue: 'vector-2022' }
             );
         }
     } catch ( e ) {
         // do nothing
     }
 } ).then( function (r) {
     // success!
 } );

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

The web team is not familiar with this extension, and have found another way to workaround this problem, so are not planning to work on the solution. This extension does appear to be maintained by community tech team. We've tried our best to capture the issue (@JMcLeod_WMF I'm not sure if you untagged on the basis that web team was working on this so may want to re-add that tag)

I've added some notes based on what we've learned this last 2 weeks.

Jdlrobson renamed this task from [M] GlobalPreferences blocks API changes to preference table to GlobalPreferences blocks API changes to preference table (mediawiki.user saveOption function doesn't work).Nov 21 2022, 6:59 PM
Jdlrobson removed the point value 1 for this task.

Thanks, @Jdlrobson. I have retagged Community-Tech and we will revisit this later. It sounds as if it is not urgent as you have found a work-around?

Yep not urgent from our side now, but likely to pop up again next time somebody tries something similar :-).

Jdlrobson updated the task description. (Show Details)

Yes. This is a request to improve how this code works. We have had to workaround this bug, and this is likely to confuse someone else in future. This task is requesting that the existing API code is abstracted in such a way that developers do not need to understand the internals of how GlobalPreferences work and to avoid having to make multiple API calls to save a single preference.

Basically this bug means that any code using the api will not work for users who are managing using global preferences:
Presumably every bit of code that matches this regex: https://codesearch.wmcloud.org/deployed/?q=saveOptions&i=nope&files=.*%5C.js&excludeFiles=&repos=

Let me know if you need me to walk through this problem to a developer for better understanding.

On T198913 we had multiple engineers arguing that users should be informed of global preference updates or overrides, so the default behaviour of action=options, where there is a non-overridden global preference and the extension has not suitably informed the user, should be to fail. As such, it seems to me that the apiwarn-globally-overridden warning should have been an error.

However, extensions do need an efficient and convenient way to query the global preference state and to opt in to updating it. Something like

var saveOpts = {};
if ( mw.user.options.isGlobal( 'vector-limited-width' ) ) {
    saveOpts.global = shouldUpdateGlobal() ? 'update' : 'override';
}
new mw.Api().saveOption( 'vector-limited-width', '1', saveOpts );

Core is allowed to provide concepts which are implemented by extensions. It's permissible for core to provide an interface into global preferences even when it doesn't implement them. A stub implementation is more convenient for callers than client-side detection of extension presence.

(unless we resort to hacky string indexOf checks on the warning).

The typical API response is:

{
    "warnings": {
        "options": {
            "warnings": "Option `skin` is globally overridden. You can use the \"globalpreferences\" API to change the option globally, or the \"globalpreferenceoverrides\" API to set a local override."
        }
    },
    "options": "success"
}

It gives the text of the warning in the user's language, it doesn't give a machine-readable code.

This was addressed in 2016 as part of T47843. But for backwards compatibility, it is necessary to opt in to the fix with e.g. errorformat=plaintext. Then you get:

{
    "warnings": [
        {
            "code": "globally-overridden",
            "text": "Option `skin` is globally overridden. You can use the \"globalpreferences\" API to change the option globally, or the \"globalpreferenceoverrides\" API to set a local override.",
            "module": "options"
        }
    ],
    "options": "success"
}

The default error format is "bc".

As such, it seems to me that the apiwarn-globally-overridden warning should have been an error.

This turns out to be inconsistent with the way the module handles errors. Any problem with an individual option is delivered as a warning -- that's how it processes a batch of option changes.

Is this still planned tor May 20th? Now we have shipped to beta features this is becoming more urgent to fix. Thanks in advance!

Change #1036076 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Move "reset kinds" concept to PreferencesFactory

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

Change #1036329 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/extensions/GlobalPreferences@master] Update for core deprecation of UserOptionsManager::getOptionKinds

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

Change #1036780 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Remove "options" parameter to User::createNew()

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

Change #1036781 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] user: Introduce UserOptionsStore

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

Change #1037372 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/extensions/GlobalPreferences@master] Use new ApiOptionsBase base class

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

Change #1037373 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/extensions/GlobalPreferences@master] Add and register GlobalUserOptionsStore

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

Change #1037374 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/extensions/GlobalPreferences@master] Remove ApiOptions hook

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

Change #1037375 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] Split a base class out of ApiOptions

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

Change #1037376 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] user: Add "global" parameter to ApiOptions

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

I just discovered T224245, which is a different approach to the same problem, already implemented.

I think nobody from that task is subscribed to this task. I'm subscribing the people from that task who still work for us. This is basically my first time touching this extension.

My core-integrated solution with action=options, global=update is almost done, but I am happy to abandon it if the existing solution meets @Jdlrobson's needs.

It doesn't seem the problem. This doesn't seem to be documented on https://www.mediawiki.org/wiki/Extension:GlobalPreferences#Configuration, but reading through T224245, it doesn't look like this meets our needs (given the comment in T224245#5314103) and I confirmed it using the following steps.

According to production, $wgGlobalPreferencesAutoPrefs is only set to [ 'email-blacklist', 'echo-notifications-blacklist' ]

So I ran the following code:

const api = new mw.Api();
api.saveOptions( { 'echo-notifications-blacklist': 1 } )

However when I go to French Wikipedia it's not defined there:

mw.user.options.values['echo-notifications-blacklist'] !== 1

I then made sure it was a global preference set to 2 by going to https://en.wikipedia.org/wiki/Special:ApiSandbox#action=globalpreferences&format=json&optionname=echo-notifications-blacklist&optionvalue=2&token=08401f17816649f6e361eec44e3a605d665a2c95%2B%5C&formatversion=2

After which on French
mw.user.options.values['echo-notifications-blacklist'] === '2'

and on English mw.user.options.values['echo-notifications-blacklist'] == ='2'

I then repeated the following code on English:

const api = new mw.Api();
api.saveOptions( { 'echo-notifications-blacklist': 1 } )

and hit the same warning:

{
    "warnings": {
        "options": {
            "warnings": "Option `echo-notifications-blacklist` is globally overridden. You can use the \"globalpreferences\" API to change the option globally, or the \"globalpreferenceoverrides\" API to set a local override."
        }
    },
    "options": "success"
}

Change #1038451 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/core@master] user: In ApiOptions escalate failure of all changes to an error

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

Change #1039346 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Add support for setting global preferences via API

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

Change #1038452 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] WIP: Make all frontend stored preferences global

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

Change #1036076 merged by jenkins-bot:

[mediawiki/core@master] user: Move "reset kinds" concept to PreferencesFactory

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

Change #1036329 merged by jenkins-bot:

[mediawiki/extensions/GlobalPreferences@master] Update for core deprecation of UserOptionsManager::getOptionKinds

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

Change #1036780 merged by jenkins-bot:

[mediawiki/core@master] user: Remove "options" parameter to User::createNew()

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

Change #1036781 merged by jenkins-bot:

[mediawiki/core@master] user: Introduce UserOptionsStore

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

Change #1037375 merged by jenkins-bot:

[mediawiki/core@master] user: Split a base class out of ApiOptions

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

Change #1037372 merged by jenkins-bot:

[mediawiki/extensions/GlobalPreferences@master] Use new ApiOptionsBase base class

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

Change #1037376 merged by jenkins-bot:

[mediawiki/core@master] user: Add "global" parameter to ApiOptions

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

Change #1037373 merged by jenkins-bot:

[mediawiki/extensions/GlobalPreferences@master] Add and register GlobalUserOptionsStore

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

Change #1037374 merged by jenkins-bot:

[mediawiki/extensions/GlobalPreferences@master] Remove ApiOptions hook and add integration test

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

Change #1039346 merged by jenkins-bot:

[mediawiki/core@master] Add support for setting global preferences via API

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

hey everyone, thanks for the monumental effort here prioritizing and getting this working! I can confirm this looks like it has the desired outcome and the web team is now unblocked for rolling out dark mode with global preferences 💪💪💪💪💪! This will roll out with next week's train now!

We are still discussing https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1038451?usp=search but it's not a blocker for web team's work, so I'll leave this ticket open until that conversation has wrapped up and to allow you time to QA.

Change #1038451 abandoned by Tim Starling:

[mediawiki/core@master] user: In ApiOptions escalate failure of all changes to an error

Reason:

No consensus

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

@Jdlrobson A few potential inconsistencies in behaviour I want to check with you:

Global pref for genderLocal override pref genderChanging pref toValue of global API paramOutcome
unknownunknownmaleupdateglobal preference is changed; local override unchanged
malemalefemale / unknownupdatelocal override changed; global pref unchanged
Global pref for genderLocal override pref genderChanging pref toValue of global API paramOutcome
unknownunknownmalenone / ignoreno change to any preferences local or global
malemalefemale / unknownnone / ignorelocal override changed
Global pref for genderLocal override pref genderChanging pref toValue of global API paramOutcome
unknownno local overridemaleupdateglobal preference is changed
maleno local overrideunknownupdateglobal preference is unset (perhaps because unknown is default)

Also, if you don't set a value for a preference and global=update then the global preference will be unset. Perhaps this is desired.

I don't have any opinions around the none or ignore behaviour but can comment on the update behaviour :

maleno local overrideunknownupdateglobal preference is unset (perhaps because unknown is default)

Yes unknown is default , so that makes sense.

unknownunknownmaleupdateglobal preference is changed; local override unchanged
malemalefemale / unknownupdatelocal override changed; global pref unchanged

Just to check I understand, could you share the code / steps you used to replicate this so I can investigate further?
If a local exception is set (e.g. box is ticked), only the local exception should be updated.

I don't have any opinions around the none or ignore behaviour but can comment on the update behaviour :

maleno local overrideunknownupdateglobal preference is unset (perhaps because unknown is default)

Yes unknown is default , so that makes sense.

OK. But this does not happen, for example, if you do the same thing via action=globalpreferences (e.g. https://de.wikipedia.beta.wmflabs.org/wiki/Spezial:ApiSandbox#action=globalpreferences&format=json&optionname=gender&optionvalue=unknown&formatversion=2)

unknownunknownmaleupdateglobal preference is changed; local override unchanged
malemalefemale / unknownupdatelocal override changed; global pref unchanged

Just to check I understand, could you share the code / steps you used to replicate this so I can investigate further?
If a local exception is set (e.g. box is ticked), only the local exception should be updated.

  1. Login to beta
  2. On global preferences, set your gender to unknown (e.g. https://de.wikipedia.beta.wmflabs.org/wiki/Spezial:ApiSandbox#action=globalpreferences&format=json&optionname=gender&optionvalue=unknown&formatversion=2)
  3. On local preferences, set the local override to unknown (e.g. https://de.wikipedia.beta.wmflabs.org/wiki/Spezial:ApiSandbox#action=globalpreferenceoverrides&format=json&optionname=gender&optionvalue=unknown&formatversion=2)
  4. https://de.wikipedia.beta.wmflabs.org/wiki/Spezial:ApiSandbox#action=options&format=json&optionname=gender&optionvalue=male&global=update&formatversion=2
  5. See that the global preference is set to male but the local override is still set to unknown

Note that some bugs related to options / local exceptions with default values are being fixed today (T368595), so you may want to test again and see if there are still any inconsistencies.

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

[mediawiki/core@master] user: Write UserOptionsStore and User class docs

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

Change #1105405 merged by jenkins-bot:

[mediawiki/core@master] user: Write UserOptionsStore and User class docs

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