From b27d33cc5c1fd62119bf67153f7e5531ebab3991 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Tue, 13 Jul 2021 14:05:10 -0700 Subject: [PATCH] Introduce new hooks for UserOptionsManager Bug: T286576 Change-Id: Ib960ec594d376e086da868d216dd85c8ea6bba14 --- RELEASE-NOTES-1.37 | 4 + includes/HookContainer/DeprecatedHooks.php | 3 + includes/HookContainer/HookRunner.php | 20 ++- includes/user/Hook/UserLoadOptionsHook.php | 2 +- .../user/Hook/UserResetAllOptionsHook.php | 2 +- includes/user/Hook/UserSaveOptionsHook.php | 2 +- .../user/Options/Hook/LoadUserOptionsHook.php | 25 ++++ .../user/Options/Hook/SaveUserOptionsHook.php | 33 +++++ includes/user/UserOptionsManager.php | 14 +- .../includes/user/UserOptionsManagerTest.php | 125 ++++++++++++++++-- 10 files changed, 208 insertions(+), 22 deletions(-) create mode 100644 includes/user/Options/Hook/LoadUserOptionsHook.php create mode 100644 includes/user/Options/Hook/SaveUserOptionsHook.php diff --git a/RELEASE-NOTES-1.37 b/RELEASE-NOTES-1.37 index f1bc8d41074..07b2a05033d 100644 --- a/RELEASE-NOTES-1.37 +++ b/RELEASE-NOTES-1.37 @@ -511,6 +511,10 @@ because of Phabricator reports. * User::getOptionKinds() and ::resetOptions(), both deprecated since 1.35, now emit deprecation warnings. * wfGetLB(), deprecated since 1.27, now emits deprecation warnings. +* The following hooks were deprecated: + - UserLoadOptions: use LoadUserOptions instead. + - UserSaveOptions: use SaveUserOptions instead. + - UserResetAllOptions: no replacement was provided, the hook is not used. * … === Other changes in 1.37 === diff --git a/includes/HookContainer/DeprecatedHooks.php b/includes/HookContainer/DeprecatedHooks.php index 909792166e3..7fb56778a50 100644 --- a/includes/HookContainer/DeprecatedHooks.php +++ b/includes/HookContainer/DeprecatedHooks.php @@ -57,6 +57,9 @@ class DeprecatedHooks { 'SkinTemplateOutputPageBeforeExec' => [ 'deprecatedVersion' => '1.35' ], 'SkinTemplateToolboxEnd' => [ 'deprecatedVersion' => '1.35' ], 'UserLoadFromDatabase' => [ 'deprecatedVersion' => '1.37' ], + 'UserLoadOptions' => [ 'deprecatedVersion' => '1.37', 'silent' => true ], + 'UserResetAllOptions' => [ 'deprecatedVersion' => '1.37' ], + 'UserSaveOptions' => [ 'deprecatedVersion' => '1.37', 'silent' => true ], 'UserSetCookies' => [ 'deprecatedVersion' => '1.27' ], 'WikiPageDeletionUpdates' => [ 'deprecatedVersion' => '1.32', 'silent' => true ], ]; diff --git a/includes/HookContainer/HookRunner.php b/includes/HookContainer/HookRunner.php index 5f3478df8c9..ee7e0019599 100644 --- a/includes/HookContainer/HookRunner.php +++ b/includes/HookContainer/HookRunner.php @@ -8,6 +8,7 @@ use IContextSource; use MediaWiki\Linker\LinkRenderer; use MediaWiki\Linker\LinkTarget; use MediaWiki\Revision\RevisionRecord; +use MediaWiki\User\UserIdentity; use ParserOptions; use ResourceLoaderContext; use Skin; @@ -550,7 +551,9 @@ class HookRunner implements \MediaWiki\User\Hook\UserSendConfirmationMailHook, \MediaWiki\User\Hook\UserSetEmailAuthenticationTimestampHook, \MediaWiki\User\Hook\UserSetEmailHook, - \MediaWiki\User\Hook\User__mailPasswordInternalHook + \MediaWiki\User\Hook\User__mailPasswordInternalHook, + \MediaWiki\User\Options\Hook\LoadUserOptionsHook, + \MediaWiki\User\Options\Hook\SaveUserOptionsHook { /** @var HookContainer */ private $container; @@ -4206,6 +4209,14 @@ class HookRunner implements ); } + public function onLoadUserOptions( UserIdentity $user, array &$options ) : void { + $this->container->run( + 'LoadUserOptions', + [ $user, &$options ], + [ 'abortable' => false ] + ); + } + public function onUserLoggedIn( $user ) { return $this->container->run( 'UserLoggedIn', @@ -4287,6 +4298,13 @@ class HookRunner implements ); } + public function onSaveUserOptions( UserIdentity $user, array &$modifiedOptions ) { + return $this->container->run( + 'SaveUserOptions', + [ $user, &$modifiedOptions ] + ); + } + public function onUserSaveSettings( $user ) { return $this->container->run( 'UserSaveSettings', diff --git a/includes/user/Hook/UserLoadOptionsHook.php b/includes/user/Hook/UserLoadOptionsHook.php index d4346cd4301..2a9e1d29f0e 100644 --- a/includes/user/Hook/UserLoadOptionsHook.php +++ b/includes/user/Hook/UserLoadOptionsHook.php @@ -8,7 +8,7 @@ use User; * This is a hook handler interface, see docs/Hooks.md. * Use the hook name "UserLoadOptions" to register handlers implementing this interface. * - * @stable to implement + * @deprecated since 1.37 use LoadUserOptionsHook instead. * @ingroup Hooks */ interface UserLoadOptionsHook { diff --git a/includes/user/Hook/UserResetAllOptionsHook.php b/includes/user/Hook/UserResetAllOptionsHook.php index 7f9987f937b..654141f43cc 100644 --- a/includes/user/Hook/UserResetAllOptionsHook.php +++ b/includes/user/Hook/UserResetAllOptionsHook.php @@ -8,7 +8,7 @@ use User; * This is a hook handler interface, see docs/Hooks.md. * Use the hook name "UserResetAllOptions" to register handlers implementing this interface. * - * @stable to implement + * @deprecated since 1.37. This hook was not used and no replacement is provided * @ingroup Hooks */ interface UserResetAllOptionsHook { diff --git a/includes/user/Hook/UserSaveOptionsHook.php b/includes/user/Hook/UserSaveOptionsHook.php index 8a2a250be4d..950987e39f4 100644 --- a/includes/user/Hook/UserSaveOptionsHook.php +++ b/includes/user/Hook/UserSaveOptionsHook.php @@ -8,7 +8,7 @@ use User; * This is a hook handler interface, see docs/Hooks.md. * Use the hook name "UserSaveOptions" to register handlers implementing this interface. * - * @stable to implement + * @deprecated since 1.37 use SaveUserOptionsHook instead. * @ingroup Hooks */ interface UserSaveOptionsHook { diff --git a/includes/user/Options/Hook/LoadUserOptionsHook.php b/includes/user/Options/Hook/LoadUserOptionsHook.php new file mode 100644 index 00000000000..b64ca360bc7 --- /dev/null +++ b/includes/user/Options/Hook/LoadUserOptionsHook.php @@ -0,0 +1,25 @@ +getCacheKey( $user ); - // Not using getOptions(), to keep hidden preferences in database - $optionsToSave = $this->loadUserOptions( $user, self::READ_LATEST ); - $originalOptions = $this->originalOptionsCache[$userKey] ?? []; + $modifiedOptions = $this->modifiedOptions[$userKey] ?? []; + if ( !$this->hookRunner->onSaveUserOptions( $user, $modifiedOptions ) ) { + return; + } + // TODO: only needed for old hook. + $originalOptions = $this->loadOriginalOptions( $user, self::READ_LATEST ); + $optionsToSave = array_merge( $originalOptions, $modifiedOptions ); // 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 ( !$this->hookRunner->onUserSaveOptions( User::newFromIdentity( $user ), $optionsToSave, $originalOptions ) ) { @@ -624,10 +627,11 @@ class UserOptionsManager extends UserOptionsLookup { // infinite recursion if the hook attempts to reload options $this->originalOptionsCache[$userKey] = $options; $this->queryFlagsUsedForCaching[$userKey] = $queryFlags; - // TODO: Deprecate passing full User object into the hook. + // TODO: Remove deprecated hook. $this->hookRunner->onUserLoadOptions( User::newFromIdentity( $user ), $options ); + $this->hookRunner->onLoadUserOptions( $user, $options ); $this->originalOptionsCache[$userKey] = $options; return $options; } diff --git a/tests/phpunit/includes/user/UserOptionsManagerTest.php b/tests/phpunit/includes/user/UserOptionsManagerTest.php index 276fc179e6f..22115126db4 100644 --- a/tests/phpunit/includes/user/UserOptionsManagerTest.php +++ b/tests/phpunit/includes/user/UserOptionsManagerTest.php @@ -2,6 +2,8 @@ use MediaWiki\Config\ServiceOptions; use MediaWiki\MediaWikiServices; +use MediaWiki\User\UserIdentity; +use MediaWiki\User\UserIdentityValue; use MediaWiki\User\UserOptionsLookup; use MediaWiki\User\UserOptionsManager; use Psr\Log\NullLogger; @@ -13,11 +15,15 @@ use Wikimedia\Rdbms\ILoadBalancer; */ class UserOptionsManagerTest extends UserOptionsLookupTest { - private function getManager( - string $langCode = 'qqq', - array $defaultOptionsOverrides = [], - ILoadBalancer $lbOverride = null - ) { + /** + * @param array $overrides supported keys: + * - 'language' - string language code + * - 'defaults' - array default preferences + * - 'lb' - ILoadBalancer + * - 'hookContainer' - HookContainer + * @return UserOptionsManager + */ + private function getManager( array $overrides = [] ) { $services = MediaWikiServices::getInstance(); return new UserOptionsManager( new ServiceOptions( @@ -27,11 +33,14 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { 'LocalTZoffset' => 0, ] ) ), - $this->getDefaultManager( $langCode, $defaultOptionsOverrides ), + $this->getDefaultManager( + $overrides['language'] ?? 'qqq', + $overrides['defaults'] ?? [] + ), $services->getLanguageConverterFactory(), - $lbOverride ?? $services->getDBLoadBalancer(), + $overrides['lb'] ?? $services->getDBLoadBalancer(), new NullLogger(), - $services->getHookContainer() + $overrides['hookContainer'] ?? $services->getHookContainer() ); } @@ -39,7 +48,10 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { string $langCode = 'qqq', array $defaultOptionsOverrides = [] ) : UserOptionsLookup { - return $this->getManager( $langCode, $defaultOptionsOverrides ); + return $this->getManager( [ + 'language' => $langCode, + 'defaults' => $defaultOptionsOverrides, + ] ); } /** @@ -104,7 +116,7 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { /** * @covers MediaWiki\User\UserOptionsManager::loadUserOptions */ - public function testLoadUserOptionsHook() { + public function testUserLoadOptionsHook() { $user = $this->getTestUser()->getUser(); $this->setTemporaryHook( 'UserLoadOptions', @@ -117,10 +129,26 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { $this->assertSame( 'value_from_hook', $this->getManager()->getOption( $user, 'from_hook' ) ); } + /** + * @covers MediaWiki\User\UserOptionsManager::loadUserOptions + */ + public function testLoadUserOptionsHook() { + $user = UserIdentityValue::newRegistered( 42, 'Test' ); + $manager = $this->getManager( [ + 'hookContainer' => $this->createHookContainer( [ + 'LoadUserOptions' => function ( UserIdentity $hookUser, array &$options ) use ( $user ) { + $this->assertTrue( $hookUser->equals( $user ) ); + $options['from_hook'] = 'value_from_hook'; + } + ] ) + ] ); + $this->assertSame( 'value_from_hook', $manager->getOption( $user, 'from_hook' ) ); + } + /** * @covers MediaWiki\User\UserOptionsManager::saveOptions */ - public function testSaveUserOptionsHookAbort() { + public function testUserSaveOptionsHookAbort() { $user = $this->getTestUser()->getUser(); $this->setTemporaryHook( 'UserSaveOptions', @@ -137,7 +165,24 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { /** * @covers MediaWiki\User\UserOptionsManager::saveOptions */ - public function testSaveUserOptionsHookModify() { + public function testSaveUserOptionsHookAbort() { + $manager = $this->getManager( [ + 'hookContainer' => $this->createHookContainer( [ + 'SaveUserOptions' => static function () { + return false; + } + ] ) + ] ); + $user = UserIdentityValue::newRegistered( 42, 'Test' ); + $manager->setOption( $user, 'will_be_aborted_by_hook', 'value' ); + $manager->saveOptions( $user ); + $this->assertNull( $this->getManager()->getOption( $user, 'will_be_aborted_by_hook' ) ); + } + + /** + * @covers MediaWiki\User\UserOptionsManager::saveOptions + */ + public function testUserSaveOptionsHookModify() { $user = $this->getTestUser()->getUser(); $this->setTemporaryHook( 'UserSaveOptions', @@ -154,6 +199,36 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { $this->assertSame( 'value_from_hook', $this->getManager()->getOption( $user, 'from_hook' ) ); } + /** + * @covers MediaWiki\User\UserOptionsManager::saveOptions + */ + public function testSaveUserOptionsHookModify() { + $user = UserIdentityValue::newRegistered( 42, 'Test' ); + $manager = $this->getManager( [ + 'defaults' => [ + 'reset_to_default_by_hook' => 'default', + ], + 'hookContainer' => $this->createHookContainer( [ + 'SaveUserOptions' => function ( UserIdentity $hookUser, array &$modifiedOptions ) use ( $user ) { + $this->assertTrue( $user->equals( $hookUser ) ); + $modifiedOptions['reset_to_default_by_hook'] = null; + unset( $modifiedOptions['blocked_by_hook'] ); + $modifiedOptions['new_from_hook'] = 'value_from_hook'; + } + ] ), + ] ); + $manager->setOption( $user, 'reset_to_default_by_hook', 'not default' ); + $manager->setOption( $user, 'blocked_by_hook', 'blocked value' ); + $manager->saveOptions( $user ); + $this->assertSame( 'value_from_hook', $manager->getOption( $user, 'new_from_hook' ) ); + $this->assertSame( 'default', $manager->getOption( $user, 'reset_to_default_by_hook' ) ); + $this->assertNull( $manager->getOption( $user, 'blocked_by_hook' ) ); + $manager->clearUserOptionsCache( $user ); + $this->assertSame( 'value_from_hook', $manager->getOption( $user, 'new_from_hook' ) ); + $this->assertSame( 'default', $manager->getOption( $user, 'reset_to_default_by_hook' ) ); + $this->assertNull( $manager->getOption( $user, 'blocked_by_hook' ) ); + } + /** * @covers MediaWiki\User\UserOptionsManager::saveOptions */ @@ -229,6 +304,28 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { $this->assertSame( 1, $recursionCounter ); } + /** + * @covers \MediaWiki\User\UserOptionsManager::loadUserOptions + */ + public function testInfiniteRecursionOnLoadUserOptionsHook() { + $user = UserIdentityValue::newRegistered( 42, 'Test' ); + $manager = $this->getManager( [ + 'hookContainer' => $this->createHookContainer( [ + 'LoadUserOptions' => function ( UserIdentity $hookUser ) use ( $user, &$manager, &$recursionCounter ) { + if ( $hookUser->equals( $user ) ) { + $recursionCounter += 1; + $this->assertSame( 1, $recursionCounter ); + $manager->loadUserOptions( $hookUser ); + } + } + + ] ) + ] ); + $recursionCounter = 0; + $manager->loadUserOptions( $user, UserOptionsManager::READ_LATEST ); + $this->assertSame( 1, $recursionCounter ); + } + public function testSaveOptionsForAnonUser() { $this->expectException( InvalidArgumentException::class ); $this->getManager()->saveOptions( $this->getAnon( __METHOD__ ) ); @@ -266,7 +363,9 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { ->method( 'getConnectionRef' ) ->willReturn( $mockDb ); $user = $this->getTestUser()->getUser(); - $manager = $this->getManager( 'qqq', [], $mockLoadBalancer ); + $manager = $this->getManager( [ + 'lb' => $mockLoadBalancer, + ] ); $manager->getOption( $user, 'test_option',