From: Brad Jorsch <bjorsch@wikimedia.org>
Date: Fri, 21 Apr 2017 18:14:11 +0000 (-0400)
Subject: Have Title::get(Next|Previous)RevisionID sort by timestamp
X-Git-Tag: 1.31.0-rc.0~3310^2
X-Git-Url: http://git.cyclocoop.org/%22%20.%20generer_url_aide%28?a=commitdiff_plain;h=6d5217180609cbe36b0a591f497212be3e8df22d;p=lhc%2Fweb%2Fwiklou.git

Have Title::get(Next|Previous)RevisionID sort by timestamp

Revision IDs are usually increasing as timestamp increases, but not
always. Callers almost certainly want next/previous timestamp when the
two differ.

This also takes care of a minor bug in the nearby getFirstRevision()
where it'll choose an arbitrary tied revision ID if there were multiple
revisions made in the same second.

Bug: T4930
Bug: T163532
Bug: T159319
Change-Id: Iab2060a0ad5e45edbaa0ff36e863cb014b8e876f
---

diff --git a/includes/Title.php b/includes/Title.php
index e460cdaaa5..a8cfad8748 100644
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -3994,29 +3994,52 @@ class Title implements LinkTarget {
 	}
 
 	/**
-	 * Get the revision ID of the previous revision
-	 *
+	 * Get next/previous revision ID relative to another revision ID
 	 * @param int $revId Revision ID. Get the revision that was before this one.
 	 * @param int $flags Title::GAID_FOR_UPDATE
-	 * @return int|bool Old revision ID, or false if none exists
-	 */
-	public function getPreviousRevisionID( $revId, $flags = 0 ) {
-		/* This function and getNextRevisionID have bad performance when
-		   used on a page with many revisions on mysql. An explicit extended
-		   primary key may help in some cases, if the PRIMARY KEY is banned:
-		   T159319 */
+	 * @param string $dir 'next' or 'prev'
+	 * @return int|bool New revision ID, or false if none exists
+	 */
+	private function getRelativeRevisionID( $revId, $flags, $dir ) {
+		$revId = (int)$revId;
+		if ( $dir === 'next' ) {
+			$op = '>';
+			$sort = 'ASC';
+		} elseif ( $dir === 'prev' ) {
+			$op = '<';
+			$sort = 'DESC';
+		} else {
+			throw new InvalidArgumentException( '$dir must be "next" or "prev"' );
+		}
+
 		if ( $flags & self::GAID_FOR_UPDATE ) {
 			$db = wfGetDB( DB_MASTER );
 		} else {
 			$db = wfGetDB( DB_REPLICA, 'contributions' );
 		}
+
+		// Intentionally not caring if the specified revision belongs to this
+		// page. We only care about the timestamp.
+		$ts = $db->selectField( 'revision', 'rev_timestamp', [ 'rev_id' => $revId ], __METHOD__ );
+		if ( $ts === false ) {
+			$ts = $db->selectField( 'archive', 'ar_timestamp', [ 'ar_rev_id' => $revId ], __METHOD__ );
+			if ( $ts === false ) {
+				// Or should this throw an InvalidArgumentException or something?
+				return false;
+			}
+		}
+		$ts = $db->addQuotes( $ts );
+
 		$revId = $db->selectField( 'revision', 'rev_id',
 			[
 				'rev_page' => $this->getArticleID( $flags ),
-				'rev_id < ' . intval( $revId )
+				"rev_timestamp $op $ts OR (rev_timestamp = $ts AND rev_id $op $revId)"
 			],
 			__METHOD__,
-			[ 'ORDER BY' => 'rev_id DESC', 'IGNORE INDEX' => 'PRIMARY' ]
+			[
+				'ORDER BY' => "rev_timestamp $sort, rev_id $sort",
+				'IGNORE INDEX' => 'rev_timestamp', // Probably needed for T159319
+			]
 		);
 
 		if ( $revId === false ) {
@@ -4026,6 +4049,17 @@ class Title implements LinkTarget {
 		}
 	}
 
+	/**
+	 * Get the revision ID of the previous revision
+	 *
+	 * @param int $revId Revision ID. Get the revision that was before this one.
+	 * @param int $flags Title::GAID_FOR_UPDATE
+	 * @return int|bool Old revision ID, or false if none exists
+	 */
+	public function getPreviousRevisionID( $revId, $flags = 0 ) {
+		return $this->getRelativeRevisionID( $revId, $flags, 'prev' );
+	}
+
 	/**
 	 * Get the revision ID of the next revision
 	 *
@@ -4034,25 +4068,7 @@ class Title implements LinkTarget {
 	 * @return int|bool Next revision ID, or false if none exists
 	 */
 	public function getNextRevisionID( $revId, $flags = 0 ) {
-		if ( $flags & self::GAID_FOR_UPDATE ) {
-			$db = wfGetDB( DB_MASTER );
-		} else {
-			$db = wfGetDB( DB_REPLICA, 'contributions' );
-		}
-		$revId = $db->selectField( 'revision', 'rev_id',
-			[
-				'rev_page' => $this->getArticleID( $flags ),
-				'rev_id > ' . intval( $revId )
-			],
-			__METHOD__,
-			[ 'ORDER BY' => 'rev_id', 'IGNORE INDEX' => 'PRIMARY' ]
-		);
-
-		if ( $revId === false ) {
-			return false;
-		} else {
-			return intval( $revId );
-		}
+		return $this->getRelativeRevisionID( $revId, $flags, 'next' );
 	}
 
 	/**
@@ -4069,8 +4085,8 @@ class Title implements LinkTarget {
 				[ 'rev_page' => $pageId ],
 				__METHOD__,
 				[
-					'ORDER BY' => 'rev_timestamp ASC',
-					'IGNORE INDEX' => 'rev_timestamp'
+					'ORDER BY' => 'rev_timestamp ASC, rev_id ASC',
+					'IGNORE INDEX' => 'rev_timestamp', // See T159319
 				]
 			);
 			if ( $row ) {