From be2bde8705e73a881ba034f2185cfd6da25b34a3 Mon Sep 17 00:00:00 2001 From: daniel Date: Sat, 24 Nov 2018 16:59:58 +0100 Subject: [PATCH] Fix the cache timestamp for forced updates. Without this patch, the forcelinksupdate parameter of ApiPurge was inoperational, caused by the fact that RefreshLinksJob got the original revision's timestamp in the rootJobTimestamp parameter, instead of the time at which the new ParserOutput was created. See for details. Bug: T210307 Change-Id: I281d6d0ed112b35e160775e528d363ce4770990a --- includes/Storage/DerivedPageDataUpdater.php | 13 ++- .../Storage/DerivedPageDataUpdaterTest.php | 80 ++++++++++++++++++- .../includes/Storage/PageUpdaterTest.php | 3 + 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/includes/Storage/DerivedPageDataUpdater.php b/includes/Storage/DerivedPageDataUpdater.php index c401d44eeb..9ce12b4b13 100644 --- a/includes/Storage/DerivedPageDataUpdater.php +++ b/includes/Storage/DerivedPageDataUpdater.php @@ -151,6 +151,9 @@ class DerivedPageDataUpdater implements IDBAccessObject { */ private $options = [ 'changed' => true, + // newrev is true if prepareUpdate is handling the creation of a new revision, + // as opposed to a null edit or a forced update. + 'newrev' => false, 'created' => false, 'moved' => false, 'restored' => false, @@ -1110,12 +1113,14 @@ class DerivedPageDataUpdater implements IDBAccessObject { // Override fields defined in $this->options with values from $options. $this->options = array_intersect_key( $options, $this->options ) + $this->options; - if ( isset( $this->pageState['oldId'] ) ) { - $oldId = $this->pageState['oldId']; + if ( $this->revision ) { + $oldId = $this->pageState['oldId'] ?? 0; + $this->options['newrev'] = ( $revision->getId() !== $oldId ); } elseif ( isset( $this->options['oldrevision'] ) ) { /** @var Revision|RevisionRecord $oldRev */ $oldRev = $this->options['oldrevision']; $oldId = $oldRev->getId(); + $this->options['newrev'] = ( $revision->getId() !== $oldId ); } else { $oldId = $revision->getParentId(); } @@ -1611,8 +1616,8 @@ class DerivedPageDataUpdater implements IDBAccessObject { // Save it to the parser cache. Use the revision timestamp in the case of a // freshly saved edit, as that matches page_touched and a mismatch would trigger an // unnecessary reparse. - $timestamp = $this->options['changed'] ? $this->revision->getTimestamp() - : $output->getTimestamp(); + $timestamp = $this->options['newrev'] ? $this->revision->getTimestamp() + : $output->getCacheTime(); $this->parserCache->save( $output, $wikiPage, $this->getCanonicalParserOptions(), $timestamp, $this->revision->getId() diff --git a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php index 5f3cba333b..3339749377 100644 --- a/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php +++ b/tests/phpunit/includes/Storage/DerivedPageDataUpdaterTest.php @@ -656,7 +656,7 @@ class DerivedPageDataUpdaterTest extends MediaWikiTestCase { RevisionSlotsUpdate $update, User $user, $comment, - $id, + $id = 0, $parentId = 0 ) { $rev = new MutableRevisionRecord( $title ); @@ -664,10 +664,13 @@ class DerivedPageDataUpdaterTest extends MediaWikiTestCase { $rev->applyUpdate( $update ); $rev->setUser( $user ); $rev->setComment( CommentStoreComment::newUnsavedComment( $comment ) ); - $rev->setId( $id ); $rev->setPageId( $title->getArticleID() ); $rev->setParentId( $parentId ); + if ( $id ) { + $rev->setId( $id ); + } + return $rev; } @@ -942,6 +945,79 @@ class DerivedPageDataUpdaterTest extends MediaWikiTestCase { // TODO: test category membership update (with setRcWatchCategoryMembership()) } + /** + * @covers \MediaWiki\Storage\DerivedPageDataUpdater::doParserCacheUpdate() + */ + public function testDoParserCacheUpdate() { + if ( $this->hasMultiSlotSupport() ) { + MediaWikiServices::getInstance()->getSlotRoleRegistry()->defineRoleWithModel( + 'aux', + CONTENT_MODEL_WIKITEXT + ); + } + + $page = $this->getPage( __METHOD__ ); + $this->createRevision( $page, 'Dummy' ); + + $user = $this->getTestUser()->getUser(); + + $update = new RevisionSlotsUpdate(); + $update->modifyContent( 'main', new WikitextContent( 'first [[Main]]' ) ); + + if ( $this->hasMultiSlotSupport() ) { + $update->modifyContent( 'aux', new WikitextContent( 'Aux [[Nix]]' ) ); + } + + // Emulate update after edit ---------- + $pcache = MediaWikiServices::getInstance()->getParserCache(); + $pcache->deleteOptionsKey( $page ); + + $rev = $this->makeRevision( $page->getTitle(), $update, $user, 'rev', null ); + $rev->setTimestamp( '20100101000000' ); + $rev->setParentId( $page->getLatest() ); + + $updater = $this->getDerivedPageDataUpdater( $page ); + $updater->prepareContent( $user, $update, false ); + + $rev->setId( 11 ); + $updater->prepareUpdate( $rev ); + + // Force the page timestamp, so we notice whether ParserOutput::getTimestamp + // or ParserOutput::getCacheTime are used. + $page->setTimestamp( $rev->getTimestamp() ); + $updater->doParserCacheUpdate(); + + // The cached ParserOutput should not use the revision timestamp + $cached = $pcache->get( $page, $updater->getCanonicalParserOptions(), true ); + $this->assertInternalType( 'object', $cached ); + $this->assertSame( $updater->getCanonicalParserOutput(), $cached ); + + $this->assertSame( $rev->getTimestamp(), $cached->getCacheTime() ); + $this->assertSame( $rev->getId(), $cached->getCacheRevisionId() ); + + // Emulate forced update of an old revision ---------- + $pcache->deleteOptionsKey( $page ); + + $updater = $this->getDerivedPageDataUpdater( $page ); + $updater->prepareUpdate( $rev ); + + // Force the page timestamp, so we notice whether ParserOutput::getTimestamp + // or ParserOutput::getCacheTime are used. + $page->setTimestamp( $rev->getTimestamp() ); + $updater->doParserCacheUpdate(); + + // The cached ParserOutput should not use the revision timestamp + $cached = $pcache->get( $page, $updater->getCanonicalParserOptions(), true ); + $this->assertInternalType( 'object', $cached ); + $this->assertSame( $updater->getCanonicalParserOutput(), $cached ); + + $this->assertGreaterThan( $rev->getTimestamp(), $cached->getCacheTime() ); + $this->assertSame( $rev->getId(), $cached->getCacheRevisionId() ); + } + + /** + * @return bool + */ private function hasMultiSlotSupport() { global $wgMultiContentRevisionSchemaMigrationStage; diff --git a/tests/phpunit/includes/Storage/PageUpdaterTest.php b/tests/phpunit/includes/Storage/PageUpdaterTest.php index 4e090775b5..89e1d4ee1f 100644 --- a/tests/phpunit/includes/Storage/PageUpdaterTest.php +++ b/tests/phpunit/includes/Storage/PageUpdaterTest.php @@ -29,6 +29,9 @@ class PageUpdaterTest extends MediaWikiTestCase { 'aux', CONTENT_MODEL_WIKITEXT ); + + $this->tablesUsed[] = 'logging'; + $this->tablesUsed[] = 'recentchanges'; } private function getDummyTitle( $method ) { -- 2.20.1