Revert "User::saveOptions() optimization"

The hooks are not actually being called multiple times,
since getDefaultOptions caches its return value
(except while unit testing).

However, this change caused a regression due to different
handling of saved values (e.g. false no longer saves
when the default is true).

This reverts commit ff355e87e2f7a41a87c37f43232a44113cb033e0.

Bug: 60653
Change-Id: Ibbd34dde5ec5fafbdf6097337cc0fa94614f0b85

diff --git a/includes/User.php b/includes/User.php
index 7f7a22d..ca3f79b 100644 (file)
--- a/includes/User.php
+++ b/includes/User.php
@@ -4629,20 +4629,20 @@ class User {
                        return;
                }
 
-               $defaultOptions = self::getDefaultOptions();
-
                $userId = $this->getId();
                $insert_rows = array();
-               $changedOptions = array_diff_assoc( $saveOptions, $defaultOptions );
-               foreach ( $changedOptions as $key => $value ) {
-                       if ( $value === false || is_null( $value ) ) {
-                               continue;
+               foreach ( $saveOptions as $key => $value ) {
+                       // Don't bother storing default values
+                       $defaultOption = self::getDefaultOption( $key );
+                       if ( ( is_null( $defaultOption ) &&
+                                       !( $value === false || is_null( $value ) ) ) ||
+                                       $value != $defaultOption ) {
+                               $insert_rows[] = array(
+                                               'up_user' => $userId,
+                                               'up_property' => $key,
+                                               'up_value' => $value,
+                                       );
                        }
-                       $insert_rows[] = array(
-                               'up_user' => $userId,
-                               'up_property' => $key,
-                               'up_value' => $value,
-                       );
                }
 
                $dbw = wfGetDB( DB_MASTER );

diff --git a/tests/phpunit/includes/UserTest.php b/tests/phpunit/includes/UserTest.php
index b7df3f6..ff33e82 100644 (file)
--- a/tests/phpunit/includes/UserTest.php
+++ b/tests/phpunit/includes/UserTest.php
@@ -234,49 +234,4 @@ class UserTest extends MediaWikiTestCase {
                $this->assertEquals( $wgDefaultUserOptions['cols'], $this->user->getOption( 'cols' ) );
                $this->assertEquals( 'test', $this->user->getOption( 'someoption' ) );
        }
-
-       /**
-        * Helper, fetch user properties from the database.
-        * @param int $userId
-        */
-       function dbUserProperties( $userId ) {
-               $res = wfGetDB( DB_SLAVE )->select(
-                       'user_properties',
-                       array( 'up_property', 'up_value' ),
-                       array( 'up_user' => $userId ),
-                       __METHOD__
-               );
-               $ret = array();
-               foreach( $res as $row ) {
-                       $ret[$row->up_property] = $row->up_value;
-               }
-               return $ret;
-       }
-
-       public function testOnlySaveChangedOptions() {
-               $user = User::newFromName( 'UnitTestUser2' );
-               $user->addToDatabase();
-
-               // Fresh user only has default, so nothing should be in the DB
-               $dbProps = $this->dbUserProperties( $user->getId() );
-               $this->assertEmpty( $dbProps,
-                       "A new user should not have any user property saved in the DB" );
-
-               // Make sure we only save the altered option
-               $user->setOption( 'changed_opt', 'alix_20281' );
-               $user->setOption( 'switch', 1 );
-               $user->setOption( 'anotherswitch', 1 );
-               $user->saveSettings();
-
-               $expected = array (
-                       'changed_opt' => 'alix_20281',
-                       'switch' => '1',
-                       'anotherswitch' => '1',
-               );
-               $dbProps = $this->dbUserProperties( $user->getId() );
-
-               $this->assertEquals( $expected, $dbProps,
-                       "non default options should be saved, and default ones should not" );
-
-       }
 }