Pinging @Niharika and @MaxSem.

We discussed this for the other ticket (linked) but it was fairly generally. I summed up the idea here. Please see if it makes sense, and @Niharika please approve (or comment on changing?) if you're approving the change to the product behavior.

With this, the behavior will only be different to specific options that are picked, while for everything else status quo is the same.

One question is also where this wg variable "lives"; GlobalPreferences shouldn't really have a list of options in it, so we can put this in Config, but we will have to make sure that all given options are global (not per wiki). Could use your input on this, @MaxSem

@Mooeypoo

What if, instead of adding a global config, we create a new hook for GlobalPreferences (or anything else) to listen to? Something like UserSettingsPreSaveAlter? That way GlobalPreferences can do it's thing without core being aware of it. We could add this to a convenience method in User, something like this:

public function saveModifiedSettings() {
    $this->loadOptions();
    Hooks::run( 'UserSettingsPreSaveAlter', [ &$this->mOptions ] );
    $this->saveSettings();
}

This way our code can call this method, rather than saveSettings(). This lets the code decide if it would like to opt-into the new behavior (handling the global prefs) or continue to use the old behavior.

What do you think?

GlobalPreferences is listening to the UserSaveOptions hook, which hands over the $options array that can then be handled. See https://github.com/wikimedia/mediawiki-extensions-GlobalPreferences/blob/master/includes/Hooks.php#L91

Right now, it's just not doing anything with any preference unless the save came from Special:GlobalPreferences. The idea above is to expand the same hook and give it a way to know which options to make the process work for.

In T224245#5209566, @Mooeypoo wrote:

GlobalPreferences is listening to the UserSaveOptions hook, which hands over the $options array that can then be handled. See https://github.com/wikimedia/mediawiki-extensions-GlobalPreferences/blob/master/includes/Hooks.php#L91

Right now, it's just not doing anything with any preference unless the save came from Special:GlobalPreferences. The idea above is to expand the same hook and give it a way to know which options to make the process work for.

I understand the solution, I'm saying the simpler solution might be to add another hook / method. The new hook would do something with all the options (just like if the form was being saved). This way, code can save the settings locally (unmodified) or globally (modified).

In T224245#5209567, @dbarratt wrote:

I understand the solution, I'm saying the simpler solution might be to just add another hook / method. The new hook would do something with *all* the options (just like if the form was being saved). This way, code can save the settings locally (unmodified) or globally (modified).

We could do that, but there are a few problems with this since core isn't supposed to know about other extensions. If we create this in core, it's redundant (saveOptions does the same thing) and the uses of it in other places will be inconsistent. Why would something use this instead of the regular call? What if GlobalPreferences isn't installed or enabled? etc.

On top of that, I think it makes more sense to bring the logic "upwards" to GlobalPreferences. If GlobalPreferences exists, it does what it needs to do. If it doesn't, the system needs to be agnostic to it. The only place where we want to connect core to GlobalPreferences is our production environment (or, for that matter, a 3rd party production environment). Any other case, core -- and any other extension -- should "just" be doing their saving regardless of whether GlobalPreferences exists or not.

Am I misunderstanding your proposal? What does the extra call give us as on top of the regular existing call that evokes the UserSaveOptions hook?

The problem is, is if you make a whitelist and put the email-blacklist preference on it, that specific preference is going to have a different behavior in the Action API than all of the other ones. You've changed the behavior of anything using User::saveSettings() with that preference (which includes things like the Action API).

In other words, where the preference is being saved from is more important than the preferences themselves.

Eh, yes, good point about the API. We can find a solution for that, but to be fair, that might well be acceptable behavior. The API module is split between local and global preferences, since the users of the API don't have core's problem of not having to know about the existence of GlobalPreferences. The explicit product choice (until now at least) was that whoever wants to change the preferences through the API needs to explicitly choose either local API or global.

That said, I don't think it *should* be acceptable behavior. It should be consistently doing the same thing between the API and local saveSettings.

I'm really not enamored with the idea of creating another method. The saveOptions method was intended to allow extensions and anything else to manipulate the options and even stop the saving process (see the comment and code at https://github.com/wikimedia/mediawiki-extensions-GlobalPreferences/blob/master/includes/Hooks.php#L91). The purpose of that method is exactly what we want -- we just end up not doing anything with what it gives us...

Perhaps we are over complicating this. What if we added a SpecialMuteSave hook and allowed GlobalPreferences to modify the preferences used on that page?

I think I already asked this but it escapes me if I did. Why was it that we can't check if a given option is already a global preference or a local exception?
In other words, when saving "whatever-option" on onUserSaveOptions we could determine if it is a global preference and/or if it has a local override and make the changes wherever they need to be done.
Considering that the choice to make something a global or a local exception is made by the user I would say that it will be expected that any changes to those options will be stored based on the user's choice.

In T224245#5210956, @dmaza wrote:

I think I already asked this but it escapes me if I did. Why was it that we can't check if a given option is already a global preference or a local exception?
In other words, when saving "whatever-option" on onUserSaveOptions we could determine if it is a global preference and/or if it has a local override and make the changes wherever they need to be done.

Because that would make core aware of the extension, which we can't do.

@dbarratt I meant to check on GlobalPreferences not on core. Same thing @Mooeypoo is suggesting but checking against what we already have on the db instead of a config

In T224245#5210988, @dmaza wrote:

@dbarratt I meant to check on GlobalPreferences not on core. Same thing @Mooeypoo is suggesting but checking against what we already have on the db instead of a config

Because when GlobalPreferences was made, they made the decision (for whatever reason) not to do that. Perhaps it would be good to understand those reasons?

I wonder if @MaxSem might be able to give us some of the context for why it was built that way?

We've discussed it in the office and decided the following (mostly @Mooeypoo's plan):

• Certain settings are whitelisted as transparently settable. Instead of having a list, I personally would rather have a flag in the preferences array, even if that means that we'll have to put it there with a hook for core settings.
• When such a setting is getting modified, GlobalPreferences intercepts this and, if the option is global and not overridden locally, a global value is set instead. Modifying a locally overridden value transparently should already work as expected (but we need to verify this regardless).

In T224245#5225176, @MaxSem wrote:

• When such a setting is getting modified, GlobalPreferences intercepts this and, if the option is global and not overridden locally, a local value is set instead. Modifying a locally overridden value transparently should already work as expected (but we need to verify this regardless).

I think I'm misunderstanding it here, what do you mean "if the option is global and not overridden locally, a local value is set instead" ? think I'm not completely understanding that part.

Here's what I thought we agreed to (correct me if I'm wrong) --

If the setting is whitelisted (in however manner we do that, based on your first bulletpoint) GlobalPreferences will check and act according to:

• If, for the user, the setting is not global -> save in local
• If, for the user, the setting is global -> save in global
• If, for the user, the setting is global *but* uses local override -> save in local override.

Basically, GlobalPreferences knows where the setting is actually active for the user, and wherever it is active, it will save it to.

Anything asking for the current value of the preference gets the current value of whatever preference is active anyways (global/local/local-override) so doing a merge for the list in Mute, for example, will be trivial:

• It will get the value that already exists wherever the setting is actually active
• Then it will merge into that list whatever user you muted
• Then it will save that new list wherever the setting is active (Which is where the data came from)

Does that make sense?

Yeah, I've corrected my text.

In T224245#5225204, @MaxSem wrote:

Yeah, I've corrected my text.

Awesome, thanks! I added a small bit just verifying regarding getting the value of a preference. I'm fairly sure that's already happening, right? (just verifying) -- whatever code is asking for the value of the preference, they get the value of the preference that is active (global/local or local-override). Right?

Yup.

I'm not sure how this resolves the problem(s) I mentioned above. You're allowing an extension to change the behavior of a preference regardless of where it is saved.

This means, if I whitelist the email-blacklist preference, then everywhere that is saved, it will go through this process.

This is a problem, because if I save that preference with the API, it may or may not be global depending on the configuration of the wiki, and it will only be that preference, not any of the others.

Yes, I don't think that's a problem, that's the intent.

The whole issue here is that when the extension was written, it was deemed important to make sure that it's not having too much of unintended consequences of globalizing preferences, so the decision was made to split the behavior and make sure that global preferences are only stored if the user intentionally chose them to be that in Special:GlobalPreferences or if the product intentionally chose to override/do something by using the specific API.

Whether that's a good decision or not is a separate discussion, though there's a lot to say about testing the implications of making something "automatically global" before deciding to do that to all preferences. Right now, this behavior of Mute is a single case, and is not yet shown to be the norm, and our capacity is not big enough to risk falling into a trap of running around production fixing every product that may be intended to work differently -- so the tendency here is to try and be careful.

So by saying "here's a whitelist of preference names" we are basically taking a step in that direction: We don't change the behavior for *everything* where we risk missing edge cases that we can't account for (and will need to run around fixing.) Instead, we start with "only these preferences that we know of are always saved where they are active" and they will be saved like that no matter where you save them from -- intentionally. Because they're whitelisted in GlobalPreferences.

In a few months or whenever else, if we see that the whitelist is growing, and/or that the need is bigger than we though, then changing the code from "only do this to whitelisted preferences" into "do this to all preferences" is trivial.

This is the iteration-step, really. And if we think about the way we want to product to behave, having the whitelisted preference names act differently than other preferences (because they're whitelisted) is exactly the intent, not a bug.

So there are three potential behaviors with the API:

1. The preference is saved locally
2. The preference is saved locally OR globally depending on the user's preference for that preference
3. The preference is saved locally OR globally depending on the user's preference for that preference IF the preference is in the config (which is different for each wiki and is not exposed with the API) OTHERWISE it is saved locally.

What you are proposing is #3 which, to me, is super complicated for the consumer to understand and is just a way to avoid doing #2 properly. I think there is a greater risk in being stuck in #3 permanently than there is in creating issues with #2. Also, this behavior is completely hidden to the API consumer. There isn't a way to know that a preference will be saved with the new behavior or the old one unless we create a new API endpoint to expose this config.

Also, the hypothesis assumes that email-blacklist is not one of those preferences that would in fact break if you suddenly change the behavior. And it would be really difficult for a developer to debug the problem because it's not obvious that this preference is stored differently. Likewise, it will work one way on some wikis and a different way on others.

I think the risk in doing #3 is way too high, and I would recommend pursuing an alternative solution.

In T224245#5227624, @dbarratt wrote:

So there are three potential behaviors with the API:

1. The preference is saved locally
2. The preference is saved locally OR globally depending on the user's preference for that preference
3. The preference is saved locally OR globally depending on the user's preference for that preference IF the preference is in the config (which is different for each wiki and is not exposed with the API) OTHERWISE it is saved locally.

@Mooeypoo can correct me if I'm wrong here but I thought we were defining the config list globally and not per-wiki. I agree that doing it per-wiki would unnecessarily complicate things, like David mentions.

What you are proposing is #3 which, to me, is super complicated for the consumer to understand and is just a way to avoid doing #2 properly. I think there is a greater risk in being stuck in #3 permanently than there is in creating issues with #2. Also, this behavior is completely hidden to the API consumer. There isn't a way to know that a preference will be saved with the new behavior or the old one unless we create a new API endpoint to expose this config.

If we do it globally and not per-wiki, then #3 to me is an intermediate step to doing #2. It gives us some cushion to do testing and QA before enabling that for all preferences.

Also, the hypothesis assumes that email-blacklist is not one of those preferences that would in fact break if you suddenly change the behavior. And it would be really difficult for a developer to debug the problem because it's not obvious that this preference is stored differently. Likewise, it will work one way on some wikis and a different way on others.

I think the risk in doing #3 is way too high, and I would recommend pursuing an alternative solution.

Do you think the risk is still high if the config list is global and not per-wiki?

In T224245#5227647, @Niharika wrote:

Do you think the risk is still high if the config list is global and not per-wiki?

I think that slightly mitigates it, but it still needs to be exposed somehow to API consumers which preferences are whitelisted (maybe if we add a flag in the preferences array like @MaxSem this would be the case?)

But also, if we can't commit to moving to #2, then I think the risk of permanently being in the #3 state is really high and I don't think that is a good place to be in for long. I can't imagine having to explain this behavior to someone.

@dbarratt I don't see any reason why we can't move to enabling it for all preferences after testing it on a whitelisted set of preferences. There are some weird preferences out there. I am being a bit cautious so we don't cause a change that catches users by surprise.

In T224245#5227660, @Niharika wrote:

@dbarratt I don't see any reason why we can't move to enabling it for all preferences after testing it on a whitelisted set of preferences. There are some weird preferences out there. I am being a bit cautious so we don't cause a change that catches users by surprise.

I mean that's totally fine then. However, then this system is for testing purposes only and it doesn't really matter which ones you test on.

What if we did this the other way around? What if we made a blacklist instead of a whitelist?

@dbarratt Just so we don't accidentally cause any breakage, I would prefer to stick with a whitelist that we can grow and test with. There are way too many preferences to know which ones to blacklist to start with. For example, ULS does a thing when you change the language, it only sets it locally and doesn't want to make it global. So it is either a local pref or a local override. It is their product decision and us making this change might break the way ULS users expect it to work.
There are probably some other such preferences. We should do an announcement before we make this change for all preferences but meanwhile we can start with a whitelist.

Change 516983 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/GlobalPreferences@master] WIP: support for autoglobal preferences

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

Ready for review.

Change 516983 merged by jenkins-bot:
[mediawiki/extensions/GlobalPreferences@master] Support for autoglobal preferences

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

Concentrating on Special:Mute and email-blacklist option.

With:

$wgGlobalPreferencesAutoPrefs = ['email-blacklist'];

and muting/unmuting users via Special:Mute:

• If option is global, then Special:Mute will set the option globally.
• If option is global but is locally overridden, it will set the option locally.
• If option is not global, then it will set the option locally.

I tested this on a local farm of wikis. Special:Mute will only set options globally on those wikis in the farm which have $wgGlobalPreferencesAutoPrefs appropriately set. Otherwise, it will continue to try to set it locally (even if it is global).

With:

$wgGlobalPreferencesAutoPrefs = ['language'];

Setting language via the API (api.php?action=options) leads to the same outcomes listed above.

I notice that the API response does not state whether the option was set locally or globally. Nor does it report failure if it tries to locally set a global option. I don't know if this matters.

@dom_walden Does the state (when loading the Special:Mute page or via the API) reflect what it should? (i.e. if it's globally set to true but locally set to false does it display as true?)

In T224245#5315324, @dbarratt wrote:

@dom_walden Does the state (when loading the Special:Mute page or via the API) reflect what it should? (i.e. if it's globally set to true but locally set to false does it display as true?)

Both the API and Special:Mute reflect the global status in this case.