How does the same error not show up in other places? It seems to not have anything to do with GlobalPrefs.

https://github.com/wikimedia/mediawiki-extensions-Echo/blob/3e8c65d8bdf515a86f2e28e0aead710b4de5a11f/Hooks.php#L493

getOption() always returns a string.
namesFromCentralIds() accepts an array as the first parameter.

Patch for this - https://gerrit.wikimedia.org/r/#/c/374871/

The problem is that $user->getOption() doesn't only return a string: https://github.com/wikimedia/mediawiki/blob/32f57fa181fb040f4b11c996bae2d1b2a24d2913/includes/user/User.php#L5514 for email-blacklist it returns an array of user IDs.

We just have to make sure we do the same transformation in GlobalPreferences. I think.

Change 422354 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/GlobalPreferences@master] Fix handling for CentralIdLookup::namesFromCentralIds()

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

I think I've fixed this. Basically, the trouble was that when prefs are saved, they're first set on the user object (i.e. User::setOption()) and then GlobalPreferences does its thing and saves them to the global DB, and then the options are saved for the user. The annoying bit being that it's the final step in which option values are converted to their final saving form. Anyway, please have a test of this patch and see what you think; it's reasonably easy to reproduce the above bug by (prior to this patch I mean) trying to save either email-blacklist (core) or echo-notifications-blacklist (Echo) preference fields.

If we require that Echo is loaded before GlobalPreferences then all is okay (with the above patch). Is this an acceptable requirement?

We could add a check for the load order, but that would introduce even more of a solid link between GP and Echo (at the moment we only have to maintain a list of preferences, from wherever they come, that store lists of newline-separated user IDs).

Change 422354 merged by jenkins-bot:
[mediawiki/extensions/GlobalPreferences@master] Fix handling for CentralIdLookup::namesFromCentralIds()

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

Change 422642 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[operations/mediawiki-config@master] Make a note about the loading order of GlobalPreferences and Echo

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

Change 422642 merged by jenkins-bot:
[operations/mediawiki-config@master] Make a note about the loading order of GlobalPreferences and Echo

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

Change 423809 had a related patch set uploaded (by Dereckson; owner: Dereckson):
[operations/mediawiki-config@master] Enforce 1. Echo 2. GlobalPreferences extensions load order

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

Change 423809 abandoned by Dereckson:
Enforce 1. Echo 2. GlobalPreferences extensions load order

Reason:
I concur with Krinkle. This is not a peculiarity of our configuration here we need to be sure of, but a check the extension should do.

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

Change 426836 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/GlobalPreferences@master] Add registration check to ensure load order

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

I -2'd the patch because an explicit load order is something we're trying to move away from, and normally it really just hides a different error.

My general feeling that Echo is doing something weird with trying to have an array user preference. Does anything else use that functionality? Things like https://gerrit.wikimedia.org/r/#/c/425852/1/includes/user/User.php suggest to me that it was just kind of added in afterwards and not fully planned out.

Shouldn't the normalization of preferences to their save form be a core function instead of being replicated into GP?

It's not only Echo that has an array preference, it's core as well (email-blacklist). So really, GP should only have to convert the core one (and do whatever else core does to preference values when loading) when it loads global preferences.

The trouble is that if GP is before Echo, in UserLoadOptions we have to insert the global preference value as a string that Echo will then turn into an array later; or if Echo is before GP, we have to insert it as an array.

I don't think there are any other preference values that are done like this (see also 421459 which is a similar follow-up to your phpdoc fix). The closest is turning string zeros into ints, and that doesn't mess anything up (i.e. in many cases you can treat one as the other in PHP, so there might be code out there doing it wrong, but it doesn't break it). There are preferences that are stored as JSON strings, but they're returned as that as well so they're really just strings and preference handling code doesn't need to worry.

I think the addition of array preferences was as you say not completely thought through. I think one of the motivators (for the anti-harrasment team, who I think have implemented both these preferences) was to be able to return arrays in API responses (rather than JSON strings inside JSON reponses that then have to be decoded by the caller). This is also why the '0' → 0 change is made (i.e. it's for Javascript's benefit).

Personally, I feel that as preferences are all stored as strings internally, User::getOptions() etc. should all only deal with strings. All these mutations we're going when returning them should be done by the code that's using the specific preferences, because it's only there that it's obvious what the different types are for. Alternatively, and probably nicer, would be that core should know about preference types. :-)

Ping @dbarratt and @dmaza as the developers who, with me as Product Manager, built these two Mute/blacklist preferences that store data in an array.

As long as API:Userinfo returns an array instead of a new line delimited string, I have no preference on how it's used internally. I may have changed it in the wrong place, but I wasn't able to find another good place to change the pref after being loaded from the database, but before being rendered in the API's output. If there is a better place, please move it. :) or maybe we need to make a place to do this?

This would need to happen in core (with the email blacklist) as well as in a hook (for the echo mute list).

Change 428566 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/GlobalPreferences@master] Allow Echo and GlobalPreferences to be loaded in either order

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

In T190353#4140784, @dbarratt wrote:

As long as API:Userinfo returns an array instead of a new line delimited string

I'm not sure I agree: in every other situation (string/int and bool casting notwithstanding, but that's more about weak typing than data storage) the consumer of the preferences has to do its own thing with whatever string preference value it gets. For example, timecorrection might be the string Offset|480 which then has to be split apart and understood by whatever's using it. Similarly for uls-preferences which is a JSON string: {"webfonts":{"fonts":{"en":"OpenDyslexic"},"webfontsEnabled":true}}" — and now we've got email-blacklist and echo-notification-blacklist as strings of newline-separated integers.

I totally agree with you that it'd be better to have these things as actual arrays, but it's not what's done.

I guess I'm wondering if it's possible to backtrack and move the handling of these outwards a bit, so that as far as the preferences handling goes they're all strings? Or is it too late (they're in production, after all) and anything we do has to be backwards compatible? Or am I just complaining in an annoying way and should shut up 'cause it's mostly working? :-) The current work-around https://gerrit.wikimedia.org/r/#/c/428566/ checks loading order in GlobalPreferences specifically for the Echo preference, and that seems less than ideal given that other extensions might come along and do similar things without us realising.

Change 426836 abandoned by Samwilson:
Add registration check to ensure load order

Reason:
in favour of https://gerrit.wikimedia.org/r/#/c/428566/

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

In T190353#4152372, @Samwilson wrote:

I'm not sure I agree: in every other situation (string/int and bool casting notwithstanding, but that's more about weak typing than data storage) the consumer of the preferences has to do its own thing with whatever string preference value it gets. For example, timecorrection might be the string Offset|480 which then has to be split apart and understood by whatever's using it. Similarly for uls-preferences which is a JSON string: {"webfonts":{"fonts":{"en":"OpenDyslexic"},"webfontsEnabled":true}}" — and now we've got email-blacklist and echo-notification-blacklist as strings of newline-separated integers.

I have already created an issue for uls-preferences at T173935: ULS preferences (uls-preferences) are exposed in userinfo API as JSON-encoded string but apparently it isn't supposed to be exposed at all. and what's worse, is if the API exposes it as a newline delimited string, it breaks the JSON formatting (i.e. there's a random unescaped new line character in the middle of the JSON).

I totally agree with you that it'd be better to have these things as actual arrays, but it's not what's done.

I guess I'm wondering if it's possible to backtrack and move the handling of these outwards a bit, so that as far as the preferences handling goes they're all strings? Or is it too late (they're in production, after all) and anything we do has to be backwards compatible? Or am I just complaining in an annoying way and should shut up 'cause it's mostly working? :-) The current work-around https://gerrit.wikimedia.org/r/#/c/428566/ checks loading order in GlobalPreferences specifically for the Echo preference, and that seems less than ideal given that other extensions might come along and do similar things without us realising.

To me at least, the storage of the array as a string is an implementation detail of the database... are we saying that all extensions shouldn't be responsible for how the preference is formatted? I guess the question are:

• Should the type/format of a preference be abstracted from the database?
• If so, at what point should this happen?

Before I was arguing that it should at a minimum happen at the Action API, but one could argue that the preference should be responsible for abstracting the preference from the data storage as soon as it's retrieved/stored in the database (which is what email-blacklist and echo-notification-blacklist do). Personally, I think that's the best option, my second preference would be to abstract it at the Action API (and leave it as a string in code), but that would require extensions to implement another hook... my last preference would be to leave it as it is in the database, but as I said, that breaks JSON formatting, also, you might have arrays and objects that are stored differently (new line separated, commas separated, tab separated, or just a string of JSON or XML)

Change 428566 merged by jenkins-bot:
[mediawiki/extensions/GlobalPreferences@master] Allow Echo and GlobalPreferences to be loaded in either order

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