Page MenuHomePhabricator

GlobalPreferences blocks API changes to preference table (mediawiki.user saveOption function doesn't work)
Open, MediumPublic5 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

So how should the client handle this? There is no obvious way on the client to determine skin is a global preference from what I can see, and no easy way to detect this in the API itself (unless we resort to hacky string indexOf checks on the warning). At the very least it should be throwing an error?

So how should the client handle this? There is no obvious way on the client to determine skin is a global preference from what I can see, and no easy way to detect this in the API itself (unless we resort to hacky string indexOf checks on the warning).

As of today, checking whether the skin is set globally via action=query&meta=globalpreferences would be a way. That said, I realize it's not an ideal solution, as it always makes two API calls.

At the very least it should be throwing an error?

Sorry if that wasn't clear in my original comment: I do agree that an error is better than the current behavior. I just don't think it should silently do something that might not be the user's intention.

Thanks! Two API calls are not ideal but I guess that could work in the mean time. Are there any wikis where GlobalPreferences may not be installed or can we assume safely that API is always available?

All (public) Wikimedia wikis have the GlobalPreferences extension enabled.
For other wikis, that may or may not be the case.

So we have two options here: 1) make the API throw an error, wait for the train to deploy the change and update the banner or 2) update the banner with two API requests to get around existing constraints.

ovasileva raised the priority of this task from Medium to High.Nov 15 2022, 6:33 PM
ovasileva lowered the priority of this task from High to Medium.Nov 15 2022, 6:41 PM
LGoto renamed this task from GlobalPreferences blocks API changes to preference table to [M] GlobalPreferences blocks API changes to preference table.Nov 15 2022, 6:41 PM
LGoto raised the priority of this task from Medium to High.
Jdlrobson set the point value for this task to 1.Nov 15 2022, 6:42 PM
LGoto lowered the priority of this task from High to Medium.Nov 15 2022, 6:42 PM

Thinking about this some more, I'm using the code:

new mw.Api().saveOption( 'skin', 'vector-2022' ).then( function () {
                location.reload();
} );

Shouldn't GlobalPreferences be supplementing/updating this frontend library or providing its own like-for-like replacement ? Perhaps we could add a mw.hook in core to allow GlobalPreferences to update the API query before its fired?

Jdlrobson removed a project: Web-Team-Backlog.
Jdlrobson added a subscriber: JMcLeod_WMF.

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 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.