Page MenuHomePhabricator

Updating user preferences via API gives inconsistent results
Open, MediumPublic

Description

We are experiencing this with the MediaViewer enabled flag. MediaViewer is set up as follows:

the preference is defined as

	$prefs['multimediaviewer-enable'] = array(
		'type' => 'toggle',
		'label-message' => 'multimediaviewer-optin-pref',
		'section' => 'rendering/files',
	);

additionally, when the viewer is enabled by default (this depends on the site and whether the user is logged in)

	$wgDefaultUserOptions['multimediaviewer-enable'] = true;

is set.

Using the Special:Preferences page, when MediaViewer is enabled by default, enabled/disabled state is stored in user_properties.up_value as no row / '' respectively. When it is disabled by default, enabled/disabled is stored as '1' / no row respectively.

When $wgDefaultUserOptions['multimediaviewer-enable'] is true, this behavior can be replicated via the options the API by using a value of '1' (or anything other truthy) for enabling, and '' for disabling. When the default is not set to true, the behavior can be replicated by using '1' (needs to be exactly that) for enabled and no value (missing optionvalue parameter) for disabled.

However, there is no way to tell from the client side what the default is, and there doesn't seem to be any way of using the API which will work in both cases; instead of the database row just disappearing when set to the default state, it will be set to some value which could never be achieved via Special:Preferences.

So, there seems to be multiple levels of fail here:

  • the DB representation of binary preferences is messy in the first place ('1' and '' instead of '1' and '0' or some other clean setup - I presume this is because '0' can be truthy in some circumstances, but that sort of low-level consideration should be hidden from someone using the API)
  • the preference can be set to pretty much any value via the options API, not just the ones which are valid (can result from Special:Preferences)
  • setting the preference to '' does not delete the DB row when the preference defaults to false.

Version: 1.24rc
Severity: normal

Details

Reference
bz69942

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 3:37 AM
bzimport set Reference to bz69942.
bzimport added a subscriber: Unknown Object (MLST).

Doesn't setting the preference to '0' / '1' via API always work as expected?

The inconsistent values are tracked as bug 52542.

(In reply to Bartosz Dziewoński from comment #1)

Doesn't setting the preference to '0' / '1' via API always work as expected?

Depends on your definition of "work as expected". When $wgDefaultUserOptions['multimediaviewer-enable'] is not set, disabling in Special:Preferences results in the row being deleted, while setting to '0' via the API results in a DB row with '0'. When querying the preferences, you will get false either way, so functionally there isn't any difference, but there will be if $wgDefaultUserOptions is changed.

Not an API bug, the API simply calls $user->setOption().

I'm not entirely sure where the bug here lies:

  • $wgDefaultUserOptions['multimediaviewer-enable'] being assigned a non-string value.
  • User::getDefaultOptions() not correcting that.
  • The comparison in $user->saveOptions() that tries to avoid saving default options screwing up with non-string defaults.
matmarex set Security to None.
matmarex removed a subscriber: Technical13.
Umherirrender added a subscriber: Umherirrender.

It should help to always set the default option and not only when it is true

	public static function onUserGetDefaultOptions( array &$defaultOptions ) {
		global $wgMediaViewerEnableByDefault;

		if ( $wgMediaViewerEnableByDefault ) {
			$defaultOptions['multimediaviewer-enable'] = 1;
		}
	}

to

	public static function onUserGetDefaultOptions( array &$defaultOptions ) {
		global $wgMediaViewerEnableByDefault;

		$defaultOptions['multimediaviewer-enable'] = $wgMediaViewerEnableByDefault;
	}

When it is always set the remove of falsy values should work better and the same as all the other places in mediawiki/core

On the other hand the hack in multimedia viewer about the undefined is no longer working, created T299097