diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 7c9b99b6617..86acac29d4b 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -943,6 +943,8 @@ return [ $services->getDBLoadBalancer(), LoggerFactory::getInstance( 'StashEdit' ), $services->getStatsdDataFactory(), + $services->getUserEditTracker(), + $services->getUserFactory(), $services->getHookContainer(), defined( 'MEDIAWIKI_JOB_RUNNER' ) || $config->get( 'CommandLineMode' ) ? PageEditStash::INITIATOR_JOB_OR_CLI diff --git a/includes/Storage/PageEditStash.php b/includes/Storage/PageEditStash.php index a8ae454b2cb..8df2deb12ec 100644 --- a/includes/Storage/PageEditStash.php +++ b/includes/Storage/PageEditStash.php @@ -28,11 +28,14 @@ use Content; use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; use MediaWiki\HookContainer\HookContainer; use MediaWiki\HookContainer\HookRunner; +use MediaWiki\Page\PageIdentity; use MediaWiki\Storage\Hook\ParserOutputStashForEditHook; +use MediaWiki\User\UserEditTracker; +use MediaWiki\User\UserFactory; +use MediaWiki\User\UserIdentity; use ParserOutput; use Psr\Log\LoggerInterface; use stdClass; -use Title; use User; use Wikimedia\Rdbms\ILoadBalancer; use Wikimedia\ScopedCallback; @@ -54,6 +57,10 @@ class PageEditStash { private $stats; /** @var ParserOutputStashForEditHook */ private $hookRunner; + /** @var UserEditTracker */ + private $userEditTracker; + /** @var UserFactory */ + private $userFactory; /** @var int */ private $initiator; @@ -77,6 +84,8 @@ class PageEditStash { * @param ILoadBalancer $lb * @param LoggerInterface $logger * @param StatsdDataFactoryInterface $stats + * @param UserEditTracker $userEditTracker + * @param UserFactory $userFactory * @param HookContainer $hookContainer * @param int $initiator Class INITIATOR__* constant */ @@ -85,6 +94,8 @@ class PageEditStash { ILoadBalancer $lb, LoggerInterface $logger, StatsdDataFactoryInterface $stats, + UserEditTracker $userEditTracker, + UserFactory $userFactory, HookContainer $hookContainer, $initiator ) { @@ -92,6 +103,8 @@ class PageEditStash { $this->lb = $lb; $this->logger = $logger; $this->stats = $stats; + $this->userEditTracker = $userEditTracker; + $this->userFactory = $userFactory; $this->hookRunner = new HookRunner( $hookContainer ); $this->initiator = $initiator; } @@ -99,15 +112,14 @@ class PageEditStash { /** * @param WikiPage $page * @param Content $content Edit content - * @param User $user + * @param UserIdentity $user * @param string $summary Edit summary * @return string Class ERROR_* constant */ - public function parseAndCache( WikiPage $page, Content $content, User $user, string $summary ) { + public function parseAndCache( WikiPage $page, Content $content, UserIdentity $user, string $summary ) { $logger = $this->logger; - $title = $page->getTitle(); - $key = $this->getStashKey( $title, $this->getContentHash( $content ), $user ); + $key = $this->getStashKey( $page, $this->getContentHash( $content ), $user ); $fname = __METHOD__; // Use the master DB to allow for fast blocking locks on the "save path" where this @@ -139,15 +151,16 @@ class PageEditStash { $alreadyCached = false; } - $context = [ 'cachekey' => $key, 'title' => $title->getPrefixedText() ]; + $logContext = [ 'cachekey' => $key, 'title' => (string)$page ]; if ( $editInfo && $editInfo->output ) { // Let extensions add ParserOutput metadata or warm other caches + $legacyUser = $this->userFactory->newFromUserIdentity( $user ); $this->hookRunner->onParserOutputStashForEdit( - $page, $content, $editInfo->output, $summary, $user ); + $page, $content, $editInfo->output, $summary, $legacyUser ); if ( $alreadyCached ) { - $logger->debug( "Parser output for key '{cachekey}' already cached.", $context ); + $logger->debug( "Parser output for key '{cachekey}' already cached.", $logContext ); return self::ERROR_NONE; } @@ -161,20 +174,20 @@ class PageEditStash { ); if ( $code === true ) { - $logger->debug( "Cached parser output for key '{cachekey}'.", $context ); + $logger->debug( "Cached parser output for key '{cachekey}'.", $logContext ); return self::ERROR_NONE; } elseif ( $code === 'uncacheable' ) { $logger->info( "Uncacheable parser output for key '{cachekey}' [{code}].", - $context + [ 'code' => $code ] + $logContext + [ 'code' => $code ] ); return self::ERROR_UNCACHEABLE; } else { $logger->error( "Failed to cache parser output for key '{cachekey}'.", - $context + [ 'code' => $code ] + $logContext + [ 'code' => $code ] ); return self::ERROR_CACHE; @@ -199,12 +212,12 @@ class PageEditStash { * - timestamp: the timestamp of the parse * - edits: author edit count if they are logged in or NULL otherwise * - * @param Title $title + * @param PageIdentity $page * @param Content $content - * @param User $user User to get parser options from + * @param User $user to get parser options from * @return stdClass|bool Returns edit stash object or false on cache miss */ - public function checkCache( Title $title, Content $content, User $user ) { + public function checkCache( PageIdentity $page, Content $content, User $user ) { if ( // The context is not an HTTP POST request !$user->getRequest()->wasPosted() || @@ -219,10 +232,10 @@ class PageEditStash { $logger = $this->logger; - $key = $this->getStashKey( $title, $this->getContentHash( $content ), $user ); - $context = [ + $key = $this->getStashKey( $page, $this->getContentHash( $content ), $user ); + $logContext = [ 'key' => $key, - 'title' => $title->getPrefixedText(), + 'title' => (string)$page, 'user' => $user->getName() ]; @@ -230,43 +243,43 @@ class PageEditStash { if ( !is_object( $editInfo ) || !$editInfo->output ) { $this->incrStatsByContent( 'cache_misses.no_stash', $content ); if ( $this->recentStashEntryCount( $user ) > 0 ) { - $logger->info( "Empty cache for key '{key}' but not for user.", $context ); + $logger->info( "Empty cache for key '{key}' but not for user.", $logContext ); } else { - $logger->debug( "Empty cache for key '{key}'.", $context ); + $logger->debug( "Empty cache for key '{key}'.", $logContext ); } return false; } $age = time() - (int)wfTimestamp( TS_UNIX, $editInfo->output->getCacheTime() ); - $context['age'] = $age; + $logContext['age'] = $age; $isCacheUsable = true; if ( $age <= self::PRESUME_FRESH_TTL_SEC ) { // Assume nothing changed in this time $this->incrStatsByContent( 'cache_hits.presumed_fresh', $content ); - $logger->debug( "Timestamp-based cache hit for key '{key}'.", $context ); + $logger->debug( "Timestamp-based cache hit for key '{key}'.", $logContext ); } elseif ( $user->isAnon() ) { $lastEdit = $this->lastEditTime( $user ); $cacheTime = $editInfo->output->getCacheTime(); if ( $lastEdit < $cacheTime ) { // Logged-out user made no local upload/template edits in the meantime $this->incrStatsByContent( 'cache_hits.presumed_fresh', $content ); - $logger->debug( "Edit check based cache hit for key '{key}'.", $context ); + $logger->debug( "Edit check based cache hit for key '{key}'.", $logContext ); } else { $isCacheUsable = false; $this->incrStatsByContent( 'cache_misses.proven_stale', $content ); - $logger->info( "Stale cache for key '{key}' due to outside edits.", $context ); + $logger->info( "Stale cache for key '{key}' due to outside edits.", $logContext ); } } else { - if ( $editInfo->edits === $user->getEditCount() ) { + if ( $editInfo->edits === $this->userEditTracker->getUserEditCount( $user ) ) { // Logged-in user made no local upload/template edits in the meantime $this->incrStatsByContent( 'cache_hits.presumed_fresh', $content ); - $logger->debug( "Edit count based cache hit for key '{key}'.", $context ); + $logger->debug( "Edit count based cache hit for key '{key}'.", $logContext ); } else { $isCacheUsable = false; $this->incrStatsByContent( 'cache_misses.proven_stale', $content ); - $logger->info( "Stale cache for key '{key}'due to outside edits.", $context ); + $logger->info( "Stale cache for key '{key}'due to outside edits.", $logContext ); } } @@ -279,7 +292,7 @@ class PageEditStash { // but a second parse will be triggered in doEditUpdates() no matter what $logger->info( "Cache for key '{key}' has vary-revision; post-insertion parse inevitable.", - $context + $logContext ); } else { static $flagsMaybeReparse = [ @@ -296,7 +309,7 @@ class PageEditStash { if ( $editInfo->output->getFlag( $flag ) ) { $logger->debug( "Cache for key '{key}' has $flag; post-insertion parse possible.", - $context + $logContext ); } } @@ -367,10 +380,10 @@ class PageEditStash { } /** - * @param User $user + * @param UserIdentity $user * @return string|null TS_MW timestamp or null */ - private function lastEditTime( User $user ) { + private function lastEditTime( UserIdentity $user ) { $db = $this->lb->getConnectionRef( DB_REPLICA ); $actorQuery = ActorMigration::newMigration()->getWhere( $db, 'rc_user', $user, false ); @@ -407,19 +420,19 @@ class PageEditStash { * - a) The $user was used for PST options * - b) The parser output was made from the PST using cannonical matching options * - * @param Title $title + * @param PageIdentity $page * @param string $contentHash Result of getContentHash() - * @param User $user User to get parser options from + * @param UserIdentity $user User to get parser options from * @return string */ - private function getStashKey( Title $title, $contentHash, User $user ) { + private function getStashKey( PageIdentity $page, $contentHash, UserIdentity $user ) { return $this->cache->makeKey( 'stashedit-info-v1', - md5( $title->getPrefixedDBkey() ), + md5( "{$page->getNamespace()}\n{$page->getDBkey()}" ), // Account for the edit model/text $contentHash, // Account for user name related variables like signatures - md5( $user->getId() . "\n" . $user->getName() ) + md5( "{$user->getId()}\n{$user->getName()}" ) ); } @@ -445,7 +458,7 @@ class PageEditStash { * @param Content $pstContent Pre-Save transformed content * @param ParserOutput $parserOutput * @param string $timestamp TS_MW - * @param User $user + * @param UserIdentity $user * @return string|bool True or an error code */ private function storeStashValue( @@ -453,7 +466,7 @@ class PageEditStash { Content $pstContent, ParserOutput $parserOutput, $timestamp, - User $user + UserIdentity $user ) { // If an item is renewed, mind the cache TTL determined by config and parser functions. // Put an upper limit on the TTL for sanity to avoid extreme template/file staleness. @@ -473,7 +486,7 @@ class PageEditStash { 'pstContent' => $pstContent, 'output' => $parserOutput, 'timestamp' => $timestamp, - 'edits' => $user->getEditCount() + 'edits' => $user->isRegistered() ? $this->userEditTracker->getUserEditCount( $user ) : null, ]; $ok = $this->cache->set( $key, $stashInfo, $ttl, BagOStuff::WRITE_ALLOW_SEGMENTS ); @@ -486,10 +499,10 @@ class PageEditStash { } /** - * @param User $user + * @param UserIdentity $user * @param string $newKey */ - private function pruneExcessStashedEntries( User $user, $newKey ) { + private function pruneExcessStashedEntries( UserIdentity $user, $newKey ) { $key = $this->cache->makeKey( 'stash-edit-recent', sha1( $user->getName() ) ); $keyList = $this->cache->get( $key ) ?: []; @@ -503,10 +516,10 @@ class PageEditStash { } /** - * @param User $user + * @param UserIdentity $user * @return int */ - private function recentStashEntryCount( User $user ) { + private function recentStashEntryCount( UserIdentity $user ) { $key = $this->cache->makeKey( 'stash-edit-recent', sha1( $user->getName() ) ); return count( $this->cache->get( $key ) ?: [] ); diff --git a/includes/api/ApiStashEdit.php b/includes/api/ApiStashEdit.php index 67af80e29a4..d5ca84b0d68 100644 --- a/includes/api/ApiStashEdit.php +++ b/includes/api/ApiStashEdit.php @@ -22,6 +22,7 @@ use MediaWiki\Content\IContentHandlerFactory; use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\SlotRecord; use MediaWiki\Storage\PageEditStash; +use MediaWiki\User\UserIdentity; /** * Prepare an edit in shared cache so that it can be reused on edit @@ -204,13 +205,13 @@ class ApiStashEdit extends ApiBase { /** * @param WikiPage $page * @param Content $content Edit content - * @param User $user + * @param UserIdentity $user * @param string $summary Edit summary * @return string ApiStashEdit::ERROR_* constant * @since 1.25 * @deprecated Since 1.34 */ - public function parseAndStash( WikiPage $page, Content $content, User $user, $summary ) { + public function parseAndStash( WikiPage $page, Content $content, UserIdentity $user, $summary ) { return $this->pageEditStash->parseAndCache( $page, $content, $user, $summary ?? '' ); } diff --git a/tests/phpunit/includes/api/ApiStashEditTest.php b/tests/phpunit/includes/api/ApiStashEditTest.php index 347d987740d..d7b6fd22f93 100644 --- a/tests/phpunit/includes/api/ApiStashEditTest.php +++ b/tests/phpunit/includes/api/ApiStashEditTest.php @@ -15,11 +15,19 @@ use Wikimedia\TestingAccessWrapper; class ApiStashEditTest extends ApiTestCase { protected function setUp() : void { parent::setUp(); + // Hack to make user edit tracker survive service reset. + // We want it's cache to persist within tests run, otherwise + // incorrect in-process cache is being reset, and we get outdated + // edit counts. + $this->setService( 'UserEditTracker', $this->getServiceContainer() + ->getUserEditTracker() ); $this->setService( 'PageEditStash', new PageEditStash( new HashBagOStuff( [] ), MediaWikiServices::getInstance()->getDBLoadBalancer(), new NullLogger(), new NullStatsdDataFactory(), + MediaWikiServices::getInstance()->getUserEditTracker(), + MediaWikiServices::getInstance()->getUserFactory(), MediaWikiServices::getInstance()->getHookContainer(), PageEditStash::INITIATOR_USER ) );