Page MenuHomePhabricator

Throw an error from the local preferences set API if pref is overridden
Closed, ResolvedPublic

Description

When GlobalPreferences is enabled and overrides local preferences, they can no longer be set using the API. The API should send an error message mentioning that the preference is overridden using GlobalPreferences.

See https://www.mediawiki.org/wiki/API:Options

Event Timeline

Niharika created this task.Jul 5 2018, 8:30 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 5 2018, 8:30 PM
Niharika triaged this task as Normal priority.Jul 5 2018, 8:30 PM
MaxSem added a subscriber: MaxSem.Jul 11 2018, 12:56 AM

Before we go ahead with this task, let's discuss strategies. Shall we expect every extension to be prepared that their requests to set preferences might suddenly fail? Do we expect them all to know about GlobalPreferences?

Before we go ahead with this task, let's discuss strategies. Shall we expect every extension to be prepared that their requests to set preferences might suddenly fail?

Good question. Is there a way to figure out how often preferences are set using the API? If it's a fairly low number, we shouldn't worry about this but if it's quite high, then we should figure out another way.

Do we expect them all to know about GlobalPreferences?

Can you elaborate on this? Can't we customize the error message to mention Global Preferences and leave it at that?

how often preferences are set using the API?

2.7M+ times last month. VE and RCFilters love using preferences for saving their state.

Can't we customize the error message to mention Global Preferences and leave it at that?

Yeah, this is basically a choice between failing silently and failing loudly, but my question is when extensions want to continue working on WMF production smoothly, should they all add GP support?

how often preferences are set using the API?

2.7M+ times last month. VE and RCFilters love using preferences for saving their state.

Good to know. How about we switch VE and RC Filters to use local overrides/global preferences first? I imagine that will cut down the number a lot and won't cause things to break for a lot of users.

Can't we customize the error message to mention Global Preferences and leave it at that?

Yeah, this is basically a choice between failing silently and failing loudly, but my question is when extensions want to continue working on WMF production smoothly, should they all add GP support?

That's a tough one. I imagine for every new extension that gets enabled after GlobalPrefs, it should know about GP and have support for it but there are dozens of deployed extensions which may not even have maintainers now.

Thoughts @kaldari and @Mooeypoo?

My initial thought is that regular requests to set preferences that are already set globally should fail by default, but the pref setting API should return a warning or error informing the requesting software that the preference was not successfully set due to the global preference. The requesting software can then inform the user, override the global pref (via the GlobalPrefs API), or do nothing.

How about we switch VE and RC Filters to use local overrides/global preferences

RCFilters are in the core and therefore can't know about GP.

Okay, here's my general thoughts on this, but I think we should delve a little deeper and make some decisions about what we consider a good default behavior for these things.

Ideally, we should find the most sensible default behavior for the API that will not require us to fix all extensions, and, that, by default,will make sense to the majority of extensions by what users should expect.

I think that it's reasonable that if a user purposefully selected a specific preference as "global" (and, as a result, see them in the local preferences as greyed out, as reminder) that other extensions, by default, do not change the effective preference value. The number of cases where this is potentially disruptive sounds to me to be relatively edge case.

I do think that failing silently is not good; we should, at least, provide some message about this, for the requester to know their request hasn't been fulfilled and decide what to do about it.

As for RCFilters, we spoke with Joe about it, and he said that this current behavior (not changing preferences that were explicitly chosen in global values) is a feature, not a bug -- so this emphasizes, for me, the idea that the current behavior is probably sensible as default, but we should have some error being sent back to cover our bases.

I agree that not changing prefs, and returning a warning, is a good way to go.

If setting multiple, I assume the non-global ones would succeed, and so the warning would list the specific ones that could not be set?

Elitre added a subscriber: Elitre.Jul 18 2018, 9:01 AM
MaxSem claimed this task.Jul 23 2018, 8:52 PM
MaxSem moved this task from Ready to In Development on the Community-Tech-Sprint board.

Change 447730 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/core@master] New hook ApiOptions

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

Change 447731 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/GlobalPreferences@master] Add a warning when action=options changes a globally overridden setting

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

Tested this today on Group 0 wikis and the behavior is as expected. The beta cluster has broken behavior due to some beta cluster quirks.

aezell added a subscriber: aezell.EditedAug 22 2018, 3:06 PM

Should we document those quirks so they might be fixed?

Should we document those quirks so they might be fixed?

Done. T202547: [beta cluster only] Global Preferences don't work as expected

@MaxSem I commented on the patch but this looks like it failed to build correctly. Does it need to be rebased?

@aezell, I've update the GlobalPreferences patch in line with the current version of the core patch, however since we're gridlocked on the latter, it's not necessarily the right thing.

Uh, it's even more bitrotten, let's make another pass on it.

Change 447730 merged by jenkins-bot:
[mediawiki/core@master] New hook ApiOptions

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

Change 447731 merged by jenkins-bot:
[mediawiki/extensions/GlobalPreferences@master] Add a warning when action=options changes a globally overridden setting

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

Niharika closed this task as Resolved.Nov 15 2018, 11:49 PM
Niharika moved this task from QA to Q2 2018-19 on the Community-Tech-Sprint board.