diff --git a/docs/hooks.txt b/docs/hooks.txt index 32d06dfdd21..0e4b423b1ac 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -3910,6 +3910,7 @@ false, e.g. to save to a global profile instead. Compare to the UserSaveSettings hook, which is called after the preferences have been saved. $user: The User for which the options are going to be saved &$options: The users options as an associative array, modifiable +$originalOptions: The user's original options being replaced. Since 1.35 'UserSaveSettings': Called directly after user preferences (user_properties in the database) have been saved. Compare to the UserSaveOptions hook, which is diff --git a/includes/HookContainer/HookRunner.php b/includes/HookContainer/HookRunner.php index a5709dd1428..4f30fe2fb1f 100644 --- a/includes/HookContainer/HookRunner.php +++ b/includes/HookContainer/HookRunner.php @@ -4412,10 +4412,10 @@ class HookRunner implements ); } - public function onUserSaveOptions( $user, &$options ) { + public function onUserSaveOptions( $user, &$options, $originalOptions ) { return $this->container->run( 'UserSaveOptions', - [ $user, &$options ] + [ $user, &$options, $originalOptions ] ); } diff --git a/includes/user/Hook/UserSaveOptionsHook.php b/includes/user/Hook/UserSaveOptionsHook.php index 55a961c718d..3899844a4ad 100644 --- a/includes/user/Hook/UserSaveOptionsHook.php +++ b/includes/user/Hook/UserSaveOptionsHook.php @@ -21,7 +21,8 @@ interface UserSaveOptionsHook { * * @param User $user The User for which the options are going to be saved * @param array &$options The user's options as an associative array, modifiable + * @param array $originalOptions The user's original options being replaced * @return bool|void True or no return value to continue or false to abort */ - public function onUserSaveOptions( $user, &$options ); + public function onUserSaveOptions( $user, &$options, $originalOptions ); } diff --git a/includes/user/UserOptionsManager.php b/includes/user/UserOptionsManager.php index 8ee009ca878..12da46a942f 100644 --- a/includes/user/UserOptionsManager.php +++ b/includes/user/UserOptionsManager.php @@ -362,18 +362,24 @@ class UserOptionsManager extends UserOptionsLookup implements IDBAccessObject { * @internal */ public function saveOptions( UserIdentity $user ) { + $userKey = $this->getCacheKey( $user ); // Not using getOptions(), to keep hidden preferences in database $saveOptions = $this->loadUserOptions( $user, self::READ_LATEST ); + $originalOptions = $this->originalOptionsCache[$userKey] ?? []; // Allow hooks to abort, for instance to save to a global profile. // Reset options to default state before saving. // TODO: Deprecate passing User to the hook. - if ( !Hooks::run( 'UserSaveOptions', [ User::newFromIdentity( $user ), &$saveOptions ] ) ) { + if ( !Hooks::run( + 'UserSaveOptions', + [ User::newFromIdentity( $user ), &$saveOptions, $originalOptions ] + ) ) { return; } + // In case options were modified by the hook + $this->optionsCache[$userKey] = $saveOptions; $userId = $user->getId(); - $insert_rows = []; // all the new preference rows foreach ( $saveOptions as $key => $value ) { // Don't bother storing default values @@ -412,7 +418,7 @@ class UserOptionsManager extends UserOptionsLookup implements IDBAccessObject { if ( !count( $keysDelete ) && !count( $insert_rows ) ) { return; } - $this->originalOptionsCache[$this->getCacheKey( $user )] = null; + $this->originalOptionsCache[$userKey] = null; if ( count( $keysDelete ) ) { // Do the DELETE by PRIMARY KEY for prior rows. @@ -477,9 +483,7 @@ class UserOptionsManager extends UserOptionsLookup implements IDBAccessObject { $this->logger->debug( 'Loading options from override cache', [ 'user_id' => $user->getId() ] ); - foreach ( $this->originalOptionsCache[$userKey] as $key => $value ) { - $options[$key] = $value; - } + return $this->originalOptionsCache[$userKey]; } else { if ( !is_array( $data ) ) { $this->logger->debug( 'Loading options from database', [ @@ -494,7 +498,6 @@ class UserOptionsManager extends UserOptionsLookup implements IDBAccessObject { __METHOD__ ); - $this->originalOptionsCache[$userKey] = []; $data = []; foreach ( $res as $row ) { // Convert '0' to 0. PHP's boolean conversion considers them both @@ -509,21 +512,23 @@ class UserOptionsManager extends UserOptionsLookup implements IDBAccessObject { } foreach ( $data as $property => $value ) { - $this->originalOptionsCache[$userKey][$property] = $value; $options[$property] = $value; } } // Replace deprecated language codes $options['language'] = LanguageCode::replaceDeprecatedCodes( $options['language'] ); - $this->optionsCache[$userKey] = $options; - + // Need to store what we have so far before the hook to prevent + // infinite recursion if the hook attempts to reload options + $this->originalOptionsCache[$userKey] = $options; // TODO: Deprecate passing full User object into the hook. Hooks::run( 'UserLoadOptions', - [ User::newFromIdentity( $user ), &$this->optionsCache[$userKey] ] + [ User::newFromIdentity( $user ), &$options ] ); + $this->originalOptionsCache[$userKey] = $options; + $this->optionsCache[$userKey] = $options; return $this->optionsCache[$userKey]; } diff --git a/tests/phpunit/includes/user/UserOptionsManagerTest.php b/tests/phpunit/includes/user/UserOptionsManagerTest.php index 424bff3a53c..485f60b676b 100644 --- a/tests/phpunit/includes/user/UserOptionsManagerTest.php +++ b/tests/phpunit/includes/user/UserOptionsManagerTest.php @@ -134,11 +134,12 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { public function testSaveUserOptionsHookModify() { $user = $this->getTestUser()->getUser(); $this->setTemporaryHook( - 'UserLoadOptions', + 'UserSaveOptions', function ( User $hookUser, &$options ) use ( $user ) { if ( $hookUser->equals( $user ) ) { $options['from_hook'] = 'value_from_hook'; } + return true; } ); $manager = $this->getManager(); @@ -146,4 +147,79 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { $this->assertSame( 'value_from_hook', $manager->getOption( $user, 'from_hook' ) ); $this->assertSame( 'value_from_hook', $this->getManager()->getOption( $user, 'from_hook' ) ); } + + /** + * @covers MediaWiki\User\UserOptionsManager::saveOptions + */ + public function testSaveUserOptionsHookOriginal() { + $user = $this->getTestUser()->getUser(); + $manager = $this->getManager(); + $originalLanguage = $manager->getOption( $user, 'language' ); + $manager->setOption( $user, 'language', 'ru' ); + $this->setTemporaryHook( + 'UserSaveOptions', + function ( User $hookUser, &$options, $originalOptions ) use ( $user, $originalLanguage ) { + if ( $hookUser->equals( $user ) ) { + $this->assertSame( $originalLanguage, $originalOptions['language'] ); + $this->assertSame( 'ru', $options['language'] ); + $options['language'] = 'tr'; + } + return true; + } + ); + $manager->saveOptions( $user ); + $this->assertSame( 'tr', $manager->getOption( $user, 'language' ) ); + } + + /** + * @covers \MediaWiki\User\UserOptionsManager::saveOptions + * @covers \MediaWiki\User\UserOptionsManager::loadUserOptions + */ + public function testLoadOptionsHookReflectsInOriginalOptions() { + $user = $this->getTestUser()->getUser(); + $manager = $this->getManager(); + $this->setTemporaryHook( + 'UserLoadOptions', + function ( User $hookUser, &$options ) use ( $user ) { + if ( $hookUser->equals( $user ) ) { + $options['from_load_hook'] = 'from_load_hook'; + } + } + ); + $this->setTemporaryHook( + 'UserSaveOptions', + function ( User $hookUser, &$options, $originalOptions ) use ( $user ) { + if ( $hookUser->equals( $user ) ) { + $this->assertSame( 'from_load_hook', $options['from_load_hook'] ); + $this->assertSame( 'from_load_hook', $originalOptions['from_load_hook'] ); + $options['from_save_hook'] = 'from_save_hook'; + } + return true; + } + ); + $manager->saveOptions( $user ); + $this->assertSame( 'from_load_hook', $manager->getOption( $user, 'from_load_hook' ) ); + $this->assertSame( 'from_save_hook', $manager->getOption( $user, 'from_save_hook' ) ); + } + + /** + * @covers \MediaWiki\User\UserOptionsManager::loadUserOptions + */ + public function testInfiniteRecursionOnUserLoadOptionsHook() { + $user = $this->getTestUser()->getUser(); + $manager = $this->getManager(); + $recursionCounter = 0; + $this->setTemporaryHook( + 'UserLoadOptions', + function ( User $hookUser ) use ( $user, $manager, &$recursionCounter ) { + if ( $hookUser->equals( $user ) ) { + $recursionCounter += 1; + $this->assertSame( 1, $recursionCounter ); + $manager->loadUserOptions( $hookUser ); + } + } + ); + $manager->loadUserOptions( $user ); + $this->assertSame( 1, $recursionCounter ); + } }