Add $originalOptions parameter to UserSaveOptions hook

Since the hook interfaces are not yet released and adding a parameter
to the hook is b/c, I have just added a parameter without introducing
a new version of the hook interface

Bug: T253149
Change-Id: Iac6c4b706ddbc7b0c9fb0b40eba05bd3530b1fdf

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

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 ]
 		);
 	}
 

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 );
 }

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];
 	}
 

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 );
+	}
 }