diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 023d708e642..5df8da73377 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -1837,7 +1837,8 @@ return [ $services->getLanguageConverterFactory(), $services->getDBLoadBalancer(), LoggerFactory::getInstance( 'UserOptionsManager' ), - $services->getHookContainer() + $services->getHookContainer(), + $services->getUserFactory() ); }, diff --git a/includes/user/User.php b/includes/user/User.php index fc20b035819..97eae05ad2b 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -49,6 +49,7 @@ use Wikimedia\Rdbms\Database; use Wikimedia\Rdbms\DBExpectedError; use Wikimedia\Rdbms\IDatabase; use Wikimedia\ScopedCallback; +use Wikimedia\Timestamp\ConvertibleTimestamp; /** * The User object encapsulates all of the user-specific settings (user_id, @@ -2221,12 +2222,12 @@ class User implements Authority, UserIdentity, UserEmailContact { * @return string Timestamp in TS_MW format */ private function newTouchedTimestamp() { - $time = time(); + $time = ConvertibleTimestamp::now( TS_UNIX ); if ( $this->mTouched ) { - $time = max( $time, wfTimestamp( TS_UNIX, $this->mTouched ) + 1 ); + $time = max( $time, ConvertibleTimestamp::convert( TS_UNIX, $this->mTouched ) + 1 ); } - return wfTimestamp( TS_MW, $time ); + return ConvertibleTimestamp::convert( TS_MW, $time ); } /** @@ -3282,7 +3283,7 @@ class User implements Authority, UserIdentity, UserEmailContact { } ); $this->mTouched = $newTouched; - MediaWikiServices::getInstance()->getUserOptionsManager()->saveOptions( $this ); + MediaWikiServices::getInstance()->getUserOptionsManager()->saveOptionsInternal( $this, $dbw ); $this->getHookRunner()->onUserSaveSettings( $this ); $this->clearSharedCache( 'changed' ); diff --git a/includes/user/UserOptionsManager.php b/includes/user/UserOptionsManager.php index 02d31c62fa5..f4129dc409d 100644 --- a/includes/user/UserOptionsManager.php +++ b/includes/user/UserOptionsManager.php @@ -33,7 +33,7 @@ use MediaWiki\HookContainer\HookRunner; use MediaWiki\Languages\LanguageConverterFactory; use MediaWiki\MediaWikiServices; use Psr\Log\LoggerInterface; -use User; +use Wikimedia\Rdbms\IDatabase; use Wikimedia\Rdbms\ILoadBalancer; /** @@ -62,6 +62,9 @@ class UserOptionsManager extends UserOptionsLookup { /** @var ILoadBalancer */ private $loadBalancer; + /** @var UserFactory */ + private $userFactory; + /** @var LoggerInterface */ private $logger; @@ -94,6 +97,7 @@ class UserOptionsManager extends UserOptionsLookup { * @param ILoadBalancer $loadBalancer * @param LoggerInterface $logger * @param HookContainer $hookContainer + * @param UserFactory $userFactory */ public function __construct( ServiceOptions $options, @@ -101,7 +105,8 @@ class UserOptionsManager extends UserOptionsLookup { LanguageConverterFactory $languageConverterFactory, ILoadBalancer $loadBalancer, LoggerInterface $logger, - HookContainer $hookContainer + HookContainer $hookContainer, + UserFactory $userFactory ) { $options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS ); $this->serviceOptions = $options; @@ -110,6 +115,7 @@ class UserOptionsManager extends UserOptionsLookup { $this->loadBalancer = $loadBalancer; $this->logger = $logger; $this->hookRunner = new HookRunner( $hookContainer ); + $this->userFactory = $userFactory; } /** @@ -246,7 +252,10 @@ class UserOptionsManager extends UserOptionsLookup { // TODO: Deprecate passing full user to the hook $this->hookRunner->onUserResetAllOptions( - User::newFromIdentity( $user ), $newOptions, $oldOptions, $resetKinds + $this->userFactory->newFromUserIdentity( $user ), + $newOptions, + $oldOptions, + $resetKinds ); $this->modifiedOptions[$this->getCacheKey( $user )] = $newOptions; @@ -311,7 +320,7 @@ class UserOptionsManager extends UserOptionsLookup { // TODO: injecting the preferences factory creates a cyclic dependency between // PreferencesFactory and UserOptionsManager. See T250822 $preferencesFactory = MediaWikiServices::getInstance()->getPreferencesFactory(); - $user = User::newFromIdentity( $userIdentity ); + $user = $this->userFactory->newFromUserIdentity( $userIdentity ); $prefs = $preferencesFactory->getFormDescriptor( $user, $context ); $mapping = []; @@ -381,11 +390,37 @@ class UserOptionsManager extends UserOptionsLookup { /** * Saves the non-default options for this user, as previously set e.g. via * setOption(), in the database's "user_properties" (preferences) table. - * Usually used via saveSettings(). + * + * @since 1.38, this method was internal before that. * @param UserIdentity $user - * @internal */ public function saveOptions( UserIdentity $user ) { + $dbw = $this->loadBalancer->getConnectionRef( DB_PRIMARY ); + $changed = $this->saveOptionsInternal( $user, $dbw ); + $legacyUser = $this->userFactory->newFromUserIdentity( $user ); + // Before UserOptionsManager, User::saveSettings was used for user options + // saving. Some extensions might depend on UserSaveSettings hook being run + // when options are saved, so run this hook for legacy reasons. + // Once UserSaveSettings hook is deprecated and replaced with a different hook + // with more modern interface, extensions should use 'SaveUserOptions' hook. + $this->hookRunner->onUserSaveSettings( $legacyUser ); + if ( $changed ) { + $dbw->onTransactionCommitOrIdle( static function () use ( $legacyUser ) { + $legacyUser->checkAndSetTouched(); + }, __METHOD__ ); + } + } + + /** + * Saves the non-default options for this user, as previously set e.g. via + * setOption(), in the database's "user_properties" (preferences) table. + * + * @param UserIdentity $user + * @param IDatabase $dbw + * @return bool true if options were changed and new options successfully saved. + * @internal only public for use in User::saveSettings + */ + public function saveOptionsInternal( UserIdentity $user, IDatabase $dbw ): bool { if ( !$user->isRegistered() ) { throw new InvalidArgumentException( __METHOD__ . ' was called on anon user' ); } @@ -394,7 +429,7 @@ class UserOptionsManager extends UserOptionsLookup { $modifiedOptions = $this->modifiedOptions[$userKey] ?? []; $originalOptions = $this->loadOriginalOptions( $user, self::READ_LATEST ); if ( !$this->hookRunner->onSaveUserOptions( $user, $modifiedOptions, $originalOptions ) ) { - return; + return false; } // TODO: only needed for old hook. @@ -402,9 +437,9 @@ class UserOptionsManager extends UserOptionsLookup { // Allow hooks to abort, for instance to save to a global profile. // Reset options to default state before saving. if ( !$this->hookRunner->onUserSaveOptions( - User::newFromIdentity( $user ), $optionsToSave, $originalOptions ) + $this->userFactory->newFromUserIdentity( $user ), $optionsToSave, $originalOptions ) ) { - return; + return false; } // Delete any rows that were in the DB but are not in $optionsToSave @@ -444,11 +479,10 @@ class UserOptionsManager extends UserOptionsLookup { if ( !count( $keysToDelete ) && !count( $rowsToInsert ) ) { // Nothing to do - return; + return false; } // Do the DELETE - $dbw = $this->loadBalancer->getConnectionRef( DB_PRIMARY ); if ( $keysToDelete ) { $dbw->delete( 'user_properties', @@ -476,6 +510,7 @@ class UserOptionsManager extends UserOptionsLookup { unset( $this->originalOptionsCache[$userKey] ); // And nothing is modified anymore unset( $this->modifiedOptions[$userKey] ); + return true; } /** @@ -636,7 +671,7 @@ class UserOptionsManager extends UserOptionsLookup { $this->queryFlagsUsedForCaching[$userKey] = $queryFlags; // TODO: Remove deprecated hook. $this->hookRunner->onUserLoadOptions( - User::newFromIdentity( $user ), $options + $this->userFactory->newFromUserIdentity( $user ), $options ); $this->hookRunner->onLoadUserOptions( $user, $options ); $this->originalOptionsCache[$userKey] = $options; diff --git a/tests/phpunit/includes/user/UserOptionsManagerTest.php b/tests/phpunit/includes/user/UserOptionsManagerTest.php index b14f9f3ca34..46cc4079ab6 100644 --- a/tests/phpunit/includes/user/UserOptionsManagerTest.php +++ b/tests/phpunit/includes/user/UserOptionsManagerTest.php @@ -9,6 +9,7 @@ use MediaWiki\User\UserOptionsManager; use Psr\Log\NullLogger; use Wikimedia\Rdbms\DBConnRef; use Wikimedia\Rdbms\ILoadBalancer; +use Wikimedia\Timestamp\ConvertibleTimestamp; /** * @group Database @@ -16,6 +17,12 @@ use Wikimedia\Rdbms\ILoadBalancer; */ class UserOptionsManagerTest extends UserOptionsLookupTest { + protected function setUp(): void { + parent::setUp(); + $this->tablesUsed[] = 'user'; + $this->tablesUsed[] = 'user_properties'; + } + /** * @param array $overrides supported keys: * - 'language' - string language code @@ -41,7 +48,8 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { $services->getLanguageConverterFactory(), $overrides['lb'] ?? $services->getDBLoadBalancer(), new NullLogger(), - $overrides['hookContainer'] ?? $services->getHookContainer() + $overrides['hookContainer'] ?? $services->getHookContainer(), + $services->getUserFactory() ); } @@ -433,4 +441,24 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { $manager->setOption( $user, 'test_option2', 'test_value2' ); $manager->saveOptions( $user ); } + + /** + * @covers \MediaWiki\User\UserOptionsManager::saveOptions + */ + public function testUpdatesUserTouched() { + $user = $this->getTestUser()->getUser(); + $userTouched = $user->getDBTouched(); + $newTouched = ConvertibleTimestamp::convert( + TS_MW, + intval( ConvertibleTimestamp::convert( TS_UNIX, $userTouched ) ) + 100 + ); + ConvertibleTimestamp::setFakeTime( $newTouched ); + + $manager = $this->getManager(); + $manager->setOption( $user, 'test_option', 'test_value' ); + $manager->saveOptions( $user ); + $this->assertSame( $newTouched, $user->getDBTouched() ); + $user->clearInstanceCache(); + $this->assertSame( $newTouched, $user->getDBTouched() ); + } }