From: daniel <dkinzler@wikimedia.org>
Date: Sat, 24 Nov 2018 15:59:58 +0000 (+0100)
Subject: Fix the cache timestamp for forced updates.
X-Git-Tag: 1.34.0-rc.0~3210^2
X-Git-Url: http://git.cyclocoop.org/?a=commitdiff_plain;h=be2bde8705e73a881ba034f2185cfd6da25b34a3;p=lhc%2Fweb%2Fwiklou.git

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 <https://phabricator.wikimedia.org/T210307#4771586> for details.

Bug: T210307
Change-Id: I281d6d0ed112b35e160775e528d363ce4770990a
---

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 ) {