Page MenuHomePhabricator

UserOptionsManager makes it impossible to set option to an empty string when there is no pre-existing user_properties row
Closed, ResolvedPublicBUG REPORT

Description

UserOptionsManager apparently makes it impossible for callers to set the user option value to an empty string, when there was no pre-existing user_properties row (when the previous value was the option's default).

See my shell.php fiddling below (note I have $wgDefaultUserOptions['userjs-foo'] = 123; in my LocalSettings.php; at the start of the experiments, there are no user_properties rows existing for userjs-foo):

I have no name!@5f2d8572a442:/var/www/html/w$ php maintenance/shell.php 

*******************************************************************************
NOTE: Do not run maintenance scripts directly, use maintenance/run.php instead!
      Running scripts directly has been deprecated in MediaWiki 1.40.
      It may not work for some (or any) scripts in the future.
*******************************************************************************

PHP Notice:  Writing to directory /.config/psysh is not allowed. in /var/www/html/w/vendor/psy/psysh/src/ConfigPaths.php on line 327
Psy Shell v0.12.0 (PHP 8.1.20 — cli) by Justin Hileman
> $uom = \MediaWiki\MediaWikiServices::getInstance()->getUserOptionsManager()
= MediaWiki\User\Options\UserOptionsManager {#5766}

> $user = User::newFromId(1)
= MediaWiki\User\User {#7123
    +mId: 1,
    +mName: null,
    +mActorId: null,
    +mRealName: null,
    +mEmail: null,
    +mTouched: null,
    +mEmailAuthenticated: null,
    +mFrom: "id",
    mId: 1,
    mName: null,
    mActorId: null,
    mRealName: null,
    mEmail: null,
    mTouched: null,
    mEmailAuthenticated: null,
    mFrom: "id",
  }

> $uom->getOption($user, 'userjs-foo')       # this is ok, 123 is the default value for userjs-foo
= 123

> $uom->setOption($user, 'userjs-foo', '')    # i want 
= null

> $uom->getOption($user, 'userjs-foo')     # this claims everything worked well
= ""

> $uom->saveOptions($user)
= null

> $uom->getOption($user, 'userjs-foo')     # UserOptionsManager claims the operation worked even after saving into the DB
= ""

> ^D

   INFO  Ctrl+D.

I have no name!@5f2d8572a442:/var/www/html/w$ php maintenance/shell.php 

*******************************************************************************
NOTE: Do not run maintenance scripts directly, use maintenance/run.php instead!
      Running scripts directly has been deprecated in MediaWiki 1.40.
      It may not work for some (or any) scripts in the future.
*******************************************************************************

PHP Notice:  Writing to directory /.config/psysh is not allowed. in /var/www/html/w/vendor/psy/psysh/src/ConfigPaths.php on line 327
Psy Shell v0.12.0 (PHP 8.1.20 — cli) by Justin Hileman
> $uom = \MediaWiki\MediaWikiServices::getInstance()->getUserOptionsManager()
= MediaWiki\User\Options\UserOptionsManager {#5766}

> $user = User::newFromId(1)
= MediaWiki\User\User {#7123
    +mId: 1,
    +mName: null,
    +mActorId: null,
    +mRealName: null,
    +mEmail: null,
    +mTouched: null,
    +mEmailAuthenticated: null,
    +mFrom: "id",
    mId: 1,
    mName: null,
    mActorId: null,
    mRealName: null,
    mEmail: null,
    mTouched: null,
    mEmailAuthenticated: null,
    mFrom: "id",
  }

> $uom->getOption($user, 'userjs-foo')       # despite changing userjs-foo value to '' in the previous session, the change did not persist
= 123

Despite UserOptionsManager first claiming everything works correctly, in the second session, it insists on userjs-foo evaluating to 123 (the default value), which is not expected.

If I first set the option to 321 and then change it to an empty string, everything works as expected:

I have no name!@5f2d8572a442:/var/www/html/w$ php maintenance/shell.php 

*******************************************************************************
NOTE: Do not run maintenance scripts directly, use maintenance/run.php instead!
      Running scripts directly has been deprecated in MediaWiki 1.40.
      It may not work for some (or any) scripts in the future.
*******************************************************************************

PHP Notice:  Writing to directory /.config/psysh is not allowed. in /var/www/html/w/vendor/psy/psysh/src/ConfigPaths.php on line 327
Psy Shell v0.12.0 (PHP 8.1.20 — cli) by Justin Hileman
> $uom = \MediaWiki\MediaWikiServices::getInstance()->getUserOptionsManager()
= MediaWiki\User\Options\UserOptionsManager {#5766}

> $user = User::newFromId(1)
= MediaWiki\User\User {#7123
    +mId: 1,
    +mName: null,
    +mActorId: null,
    +mRealName: null,
    +mEmail: null,
    +mTouched: null,
    +mEmailAuthenticated: null,
    +mFrom: "id",
    mId: 1,
    mName: null,
    mActorId: null,
    mRealName: null,
    mEmail: null,
    mTouched: null,
    mEmailAuthenticated: null,
    mFrom: "id",
  }

> $uom->getOption($user, 'userjs-foo')
= 123

> $uom->setOption($user, 'userjs-foo', 321)
= null

> $uom->saveOptions($user)
= null

> ^D

   INFO  Ctrl+D.

I have no name!@5f2d8572a442:/var/www/html/w$ mysql -hmariadb-main -uwikiuser -p awiki
Enter password: 
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A

Welcome to the MariaDB monitor.  Commands end with ; or \g.
Your MariaDB connection id is 637
Server version: 10.11.4-MariaDB-log Source distribution

Copyright (c) 2000, 2018, Oracle, MariaDB Corporation Ab and others.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

MariaDB [awiki]> select * from user_properties where up_property='userjs-foo' and up_user=1;
+---------+-------------+----------+
| up_user | up_property | up_value |
+---------+-------------+----------+
|       1 | userjs-foo  | 321      |
+---------+-------------+----------+
1 row in set (0.006 sec)

MariaDB [awiki]> Bye

I have no name!@5f2d8572a442:/var/www/html/w$ php maintenance/shell.php 

*******************************************************************************
NOTE: Do not run maintenance scripts directly, use maintenance/run.php instead!
      Running scripts directly has been deprecated in MediaWiki 1.40.
      It may not work for some (or any) scripts in the future.
*******************************************************************************

PHP Notice:  Writing to directory /.config/psysh is not allowed. in /var/www/html/w/vendor/psy/psysh/src/ConfigPaths.php on line 327
Psy Shell v0.12.0 (PHP 8.1.20 — cli) by Justin Hileman
> $uom = \MediaWiki\MediaWikiServices::getInstance()->getUserOptionsManager()
= MediaWiki\User\Options\UserOptionsManager {#5766}

> $user = User::newFromId(1)
= MediaWiki\User\User {#7123
    +mId: 1,
    +mName: null,
    +mActorId: null,
    +mRealName: null,
    +mEmail: null,
    +mTouched: null,
    +mEmailAuthenticated: null,
    +mFrom: "id",
    mId: 1,
    mName: null,
    mActorId: null,
    mRealName: null,
    mEmail: null,
    mTouched: null,
    mEmailAuthenticated: null,
    mFrom: "id",
  }

> $uom->getOption($user, 'userjs-foo')      # this is expected, 321 is in the DB table
= "321"

> $uom->setOption($user, 'userjs-foo', '')  # try setting it to an empty string now
= null

> $uom->saveOptions($user)
= null

> ^D

   INFO  Ctrl+D.

I have no name!@5f2d8572a442:/var/www/html/w$ mysql -hmariadb-main -uwikiuser -p awiki
Enter password: 
Reading table information for completion of table and column names
You can turn off this feature to get a quicker startup with -A

Welcome to the MariaDB monitor.  Commands end with ; or \g.
Your MariaDB connection id is 639
Server version: 10.11.4-MariaDB-log Source distribution

Copyright (c) 2000, 2018, Oracle, MariaDB Corporation Ab and others.

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

MariaDB [awiki]> select * from user_properties where up_property='userjs-foo' and up_user=1;
+---------+-------------+----------+
| up_user | up_property | up_value |
+---------+-------------+----------+
|       1 | userjs-foo  |          |
+---------+-------------+----------+
1 row in set (0.004 sec)

MariaDB [awiki]>

Noticed while working at T353225: Echo: Make use of conditional user defaults.

Event Timeline

Urbanecm_WMF changed the subtype of this task from "Task" to "Bug Report".Jan 15 2024, 6:07 PM
Urbanecm_WMF triaged this task as High priority.

I investigated the issue and I'm convinced the issue is in UserOptionsManager::saveOptionsInternal. This method does the following for each of the modified options (as in, options that someone called setOption for):

$defaultValue = $this->defaultOptionsLookup->getDefaultOption( $key, $user );
$oldValue = $this->optionsFromDb[$userKey][$key] ?? null;
if ( $value === null || $this->isValueEqual( $value, $defaultValue ) ) {
	if ( array_key_exists( $key, $this->optionsFromDb[$userKey] ) ) {
		// Delete the default value from the database
		$keysToDelete[] = $key;
	}
} elseif ( !$this->isValueEqual( $value, $oldValue ) ) {
	// Update by deleting (if old value exists) and reinserting
	$rowsToInsert[] = [
		'up_user' => $user->getId(),
		'up_property' => $key,
		'up_value' => mb_strcut( $value, 0, self::MAX_BYTES_OPTION_VALUE ),
	];
	if ( array_key_exists( $key, $this->optionsFromDb[$userKey] ) ) {
		$keysToDelete[] = $key;
	}
}

However, $oldValue from this snippet is not, in fact, guaranteed to be the old value ($this->optionsFromDb is a local cache of user_properties rows; when there is no such row, the fallback should be $defaultValue, not null). As such, the comparsion in the elseif branch might not pass (when it should have), as it happened in my example above.

Change 990735 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/core@master] UserOptionsManager: Make it possible to set option values to an empty string

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

Change 990735 merged by jenkins-bot:

[mediawiki/core@master] UserOptionsManager: Do not consider null to be equal to anything but a null

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

This now works as expected:

urbanecm@deployment-mwmaint02:~$ mwscript userOptions.php --wiki=enwiki --old-is-default --old=0 --new='' --nowarn --touserid 1 'echo-subscriptions-web-article-linked'                                                                             
Setting echo-subscriptions-web-article-linked for Abuse filter from '0' to ''
urbanecm@deployment-mwmaint02:~$
wikiadmin@172.16.3.239(enwiki)> select * from user_properties where up_user=1 and up_property='echo-subscriptions-web-article-linked'; -- before                                                                                                              
Empty set (0.002 sec)

wikiadmin@172.16.3.239(enwiki)> select * from user_properties where up_user=1 and up_property='echo-subscriptions-web-article-linked'; -- after                                                                                                             
+---------+---------------------------------------+----------+
| up_user | up_property                           | up_value |
+---------+---------------------------------------+----------+
|       1 | echo-subscriptions-web-article-linked |          |
+---------+---------------------------------------+----------+
1 row in set (0.001 sec)

wikiadmin@172.16.3.239(enwiki)>

Resolving!

Change 991397 had a related patch set uploaded (by Abijeet Patro; author: Abijeet Patro):

[mediawiki/extensions/Translate@master] TranslateSandbox: Use null to unset sandbox user reminder user option

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

Change #991397 merged by jenkins-bot:

[mediawiki/extensions/Translate@master] TranslateSandbox: Use null to unset sandbox user reminder user option

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