Move getPrevious/NextRevision logic out of Title

They belong in RevisionStore. This change removes the dependency on
Title for these methods and will assist in porting more code to
LinkTarget.

At the same time, deprecate the Title parameter to
RevisionLookup/RevisionStore getPreviousRevision/getNextRevision, and
add a $flags parameter to match the functionality of the Title versions.
Since code search turned up no callers that passed a Title outside core,
this variant is immediately hard-deprecated. The Title methods
themselves are only soft-deprecated.

Change-Id: I76bc6fd6ee1a9f35b5f29fa640824fb5da3bb78e

diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34
index 46e85fa..1050c4d 100644 (file)
--- a/RELEASE-NOTES-1.34
+++ b/RELEASE-NOTES-1.34
@@ -131,6 +131,10 @@ because of Phabricator reports.
 * WatchedItem::getUser is deprecated. Use getUserIdentity.
 * Passing a Title as the first parameter to the getTimestampById method of
   RevisionStore is deprecated. Omit it, passing only the remaining parameters.
+* Title::getPreviousRevisionId and Title::getNextRevisionId are deprecated. Use
+  RevisionLookup::getPreviousRevision and RevisionLookup::getNextRevision.
+* The Title parameter to RevisionLookup::getPreviousRevision and
+  RevisionLookup::getNextRevision is deprecated and should be omitted.
 
 === Other changes in 1.34 ===
 * …

diff --git a/includes/Revision.php b/includes/Revision.php
index 2b14a21..de3c299 100644 (file)
--- a/includes/Revision.php
+++ b/includes/Revision.php
@@ -1008,9 +1008,8 @@ class Revision implements IDBAccessObject {
         * @return Revision|null
         */
        public function getPrevious() {
-               $title = $this->getTitle();
-               $rec = self::getRevisionLookup()->getPreviousRevision( $this->mRecord, $title );
-               return $rec ? new Revision( $rec, self::READ_NORMAL, $title ) : null;
+               $rec = self::getRevisionLookup()->getPreviousRevision( $this->mRecord );
+               return $rec ? new Revision( $rec, self::READ_NORMAL, $this->getTitle() ) : null;
        }
 
        /**
@@ -1019,9 +1018,8 @@ class Revision implements IDBAccessObject {
         * @return Revision|null
         */
        public function getNext() {
-               $title = $this->getTitle();
-               $rec = self::getRevisionLookup()->getNextRevision( $this->mRecord, $title );
-               return $rec ? new Revision( $rec, self::READ_NORMAL, $title ) : null;
+               $rec = self::getRevisionLookup()->getNextRevision( $this->mRecord );
+               return $rec ? new Revision( $rec, self::READ_NORMAL, $this->getTitle() ) : null;
        }
 
        /**

diff --git a/includes/Revision/RevisionLookup.php b/includes/Revision/RevisionLookup.php
index 4feb9f5..17cafc6 100644 (file)
--- a/includes/Revision/RevisionLookup.php
+++ b/includes/Revision/RevisionLookup.php
@@ -85,11 +85,12 @@ interface RevisionLookup extends IDBAccessObject {
         * MCR migration note: this replaces Revision::getPrevious
         *
         * @param RevisionRecord $rev
-        * @param Title|null $title if known (optional)
+        * @param int $flags (optional) $flags include:
+        *      IDBAccessObject::READ_LATEST: Select the data from the master
         *
         * @return RevisionRecord|null
         */
-       public function getPreviousRevision( RevisionRecord $rev, Title $title = null );
+       public function getPreviousRevision( RevisionRecord $rev, $flags = 0 );
 
        /**
         * Get next revision for this title
@@ -97,11 +98,12 @@ interface RevisionLookup extends IDBAccessObject {
         * MCR migration note: this replaces Revision::getNext
         *
         * @param RevisionRecord $rev
-        * @param Title|null $title if known (optional)
+        * @param int $flags (optional) $flags include:
+        *      IDBAccessObject::READ_LATEST: Select the data from the master
         *
         * @return RevisionRecord|null
         */
-       public function getNextRevision( RevisionRecord $rev, Title $title = null );
+       public function getNextRevision( RevisionRecord $rev, $flags = 0 );
 
        /**
         * Get rev_timestamp from rev_id, without loading the rest of the row.

diff --git a/includes/Revision/RevisionStore.php b/includes/Revision/RevisionStore.php
index 515f07d..bdaac7f 100644 (file)
--- a/includes/Revision/RevisionStore.php
+++ b/includes/Revision/RevisionStore.php
@@ -278,12 +278,13 @@ class RevisionStore
 
        /**
         * @param int $mode DB_MASTER or DB_REPLICA
+        * @param array $groups
         *
         * @return IDatabase
         */
-       private function getDBConnection( $mode ) {
+       private function getDBConnection( $mode, $groups = [] ) {
                $lb = $this->getDBLoadBalancer();
-               return $lb->getConnection( $mode, [], $this->wikiId );
+               return $lb->getConnection( $mode, $groups, $this->wikiId );
        }
 
        /**
@@ -2548,20 +2549,17 @@ class RevisionStore
        }
 
        /**
-        * Get the revision before $rev in the page's history, if any.
-        * Will return null for the first revision but also for deleted or unsaved revisions.
-        *
-        * MCR migration note: this replaces Revision::getPrevious
-        *
-        * @see Title::getPreviousRevisionID
-        * @see PageArchive::getPreviousRevision
+        * Implementation of getPreviousRevision and getNextRevision.
         *
         * @param RevisionRecord $rev
-        * @param Title|null $title if known (optional)
-        *
+        * @param int $flags
+        * @param string $dir 'next' or 'prev'
         * @return RevisionRecord|null
         */
-       public function getPreviousRevision( RevisionRecord $rev, Title $title = null ) {
+       private function getRelativeRevision( RevisionRecord $rev, $flags, $dir ) {
+               $op = $dir === 'next' ? '>' : '<';
+               $sort = $dir === 'next' ? 'ASC' : 'DESC';
+
                if ( !$rev->getId() || !$rev->getPageId() ) {
                        // revision is unsaved or otherwise incomplete
                        return null;
@@ -2572,54 +2570,86 @@ class RevisionStore
                        return null;
                }
 
-               if ( $title === null ) {
-                       // this would fail for deleted revisions
-                       $title = $this->getTitle( $rev->getPageId(), $rev->getId() );
+               list( $dbType, ) = DBAccessObjectUtils::getDBOptions( $flags );
+               $db = $this->getDBConnection( $dbType, [ 'contributions' ] );
+
+               $ts = $this->getTimestampFromId( $rev->getId(), $flags );
+               if ( $ts === false ) {
+                       // XXX Should this be moved into getTimestampFromId?
+                       $ts = $db->selectField( 'archive', 'ar_timestamp',
+                               [ 'ar_rev_id' => $rev->getId() ], __METHOD__ );
+                       if ( $ts === false ) {
+                               // XXX Is this reachable? How can we have a page id but no timestamp?
+                               return null;
+                       }
                }
+               $ts = $db->addQuotes( $db->timestamp( $ts ) );
 
-               $prev = $title->getPreviousRevisionID( $rev->getId() );
-               if ( !$prev ) {
+               $revId = $db->selectField( 'revision', 'rev_id',
+                       [
+                               'rev_page' => $rev->getPageId(),
+                               "rev_timestamp $op $ts OR (rev_timestamp = $ts AND rev_id $op {$rev->getId()})"
+                       ],
+                       __METHOD__,
+                       [
+                               'ORDER BY' => "rev_timestamp $sort, rev_id $sort",
+                               'IGNORE INDEX' => 'rev_timestamp', // Probably needed for T159319
+                       ]
+               );
+
+               if ( $revId === false ) {
                        return null;
                }
 
-               return $this->getRevisionByTitle( $title, $prev );
+               return $this->getRevisionById( intval( $revId ) );
        }
 
        /**
-        * Get the revision after $rev in the page's history, if any.
-        * Will return null for the latest revision but also for deleted or unsaved revisions.
+        * Get the revision before $rev in the page's history, if any.
+        * Will return null for the first revision but also for deleted or unsaved revisions.
         *
-        * MCR migration note: this replaces Revision::getNext
+        * MCR migration note: this replaces Revision::getPrevious
         *
-        * @see Title::getNextRevisionID
+        * @see Title::getPreviousRevisionID
+        * @see PageArchive::getPreviousRevision
         *
         * @param RevisionRecord $rev
-        * @param Title|null $title if known (optional)
+        * @param int $flags (optional) $flags include:
+        *      IDBAccessObject::READ_LATEST: Select the data from the master
         *
         * @return RevisionRecord|null
         */
-       public function getNextRevision( RevisionRecord $rev, Title $title = null ) {
-               if ( !$rev->getId() || !$rev->getPageId() ) {
-                       // revision is unsaved or otherwise incomplete
-                       return null;
+       public function getPreviousRevision( RevisionRecord $rev, $flags = 0 ) {
+               if ( $flags instanceof Title ) {
+                       // Old calling convention, we don't use Title here anymore
+                       wfDeprecated( __METHOD__ . ' with Title', '1.34' );
+                       $flags = 0;
                }
 
-               if ( $rev instanceof RevisionArchiveRecord ) {
-                       // revision is deleted, so it's not part of the page history
-                       return null;
-               }
-
-               if ( $title === null ) {
-                       // this would fail for deleted revisions
-                       $title = $this->getTitle( $rev->getPageId(), $rev->getId() );
-               }
+               return $this->getRelativeRevision( $rev, $flags, 'prev' );
+       }
 
-               $next = $title->getNextRevisionID( $rev->getId() );
-               if ( !$next ) {
-                       return null;
+       /**
+        * Get the revision after $rev in the page's history, if any.
+        * Will return null for the latest revision but also for deleted or unsaved revisions.
+        *
+        * MCR migration note: this replaces Revision::getNext
+        *
+        * @see Title::getNextRevisionID
+        *
+        * @param RevisionRecord $rev
+        * @param int $flags (optional) $flags include:
+        *      IDBAccessObject::READ_LATEST: Select the data from the master
+        * @return RevisionRecord|null
+        */
+       public function getNextRevision( RevisionRecord $rev, $flags = 0 ) {
+               if ( $flags instanceof Title ) {
+                       // Old calling convention, we don't use Title here anymore
+                       wfDeprecated( __METHOD__ . ' with Title', '1.34' );
+                       $flags = 0;
                }
 
-               return $this->getRevisionByTitle( $title, $next );
+               return $this->getRelativeRevision( $rev, $flags, 'next' );
        }
 
        /**

diff --git a/includes/Title.php b/includes/Title.php
index 27baeb2..2203ccb 100644 (file)
--- a/includes/Title.php
+++ b/includes/Title.php
@@ -3730,57 +3730,25 @@ class Title implements LinkTarget, IDBAccessObject {
         * @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;
-                       }
+               $rl = MediaWikiServices::getInstance()->getRevisionLookup();
+               $rlFlags = $flags === self::GAID_FOR_UPDATE ? IDBAccessObject::READ_LATEST : 0;
+               $rev = $rl->getRevisionById( $revId, $rlFlags );
+               if ( !$rev ) {
+                       return false;
                }
-               $ts = $db->addQuotes( $ts );
-
-               $revId = $db->selectField( 'revision', 'rev_id',
-                       [
-                               'rev_page' => $this->getArticleID( $flags ),
-                               "rev_timestamp $op $ts OR (rev_timestamp = $ts AND rev_id $op $revId)"
-                       ],
-                       __METHOD__,
-                       [
-                               'ORDER BY' => "rev_timestamp $sort, rev_id $sort",
-                               'IGNORE INDEX' => 'rev_timestamp', // Probably needed for T159319
-                       ]
-               );
-
-               if ( $revId === false ) {
+               $oldRev = $dir === 'next'
+                       ? $rl->getNextRevision( $rev, $rlFlags )
+                       : $rl->getPreviousRevision( $rev, $rlFlags );
+               if ( !$oldRev ) {
                        return false;
-               } else {
-                       return intval( $revId );
                }
+               return $oldRev->getId();
        }
 
        /**
         * Get the revision ID of the previous revision
         *
+        * @deprecated since 1.34, use RevisionLookup::getPreviousRevision
         * @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
@@ -3792,6 +3760,7 @@ class Title implements LinkTarget, IDBAccessObject {
        /**
         * Get the revision ID of the next revision
         *
+        * @deprecated since 1.34, use RevisionLookup::getNextRevision
         * @param int $revId Revision ID. Get the revision that was after this one.
         * @param int $flags Title::GAID_FOR_UPDATE
         * @return int|bool Next revision ID, or false if none exists

diff --git a/tests/phpunit/includes/RevisionDbTestBase.php b/tests/phpunit/includes/RevisionDbTestBase.php
index 13ddffa..96e2766 100644 (file)
--- a/tests/phpunit/includes/RevisionDbTestBase.php
+++ b/tests/phpunit/includes/RevisionDbTestBase.php
@@ -625,6 +625,34 @@ abstract class RevisionDbTestBase extends MediaWikiTestCase {
                $this->assertEquals( $latestRevision, $newRevision->getPrevious()->getId() );
        }
 
+       /**
+        * @covers Title::getPreviousRevisionID
+        * @covers Title::getRelativeRevisionID
+        * @covers MediaWiki\Revision\RevisionStore::getPreviousRevision
+        * @covers MediaWiki\Revision\RevisionStore::getRelativeRevision
+        */
+       public function testTitleGetPreviousRevisionID() {
+               $oldestId = $this->testPage->getOldestRevision()->getId();
+               $latestId = $this->testPage->getLatest();
+
+               $title = $this->testPage->getTitle();
+
+               $this->assertFalse( $title->getPreviousRevisionID( $oldestId ) );
+
+               $this->testPage->doEditContent( new WikitextContent( __METHOD__ ), __METHOD__ );
+               $newId = $this->testPage->getRevision()->getId();
+
+               $this->assertEquals( $latestId, $title->getPreviousRevisionID( $newId ) );
+       }
+
+       /**
+        * @covers Title::getPreviousRevisionID
+        * @covers Title::getRelativeRevisionID
+        */
+       public function testTitleGetPreviousRevisionID_invalid() {
+               $this->assertFalse( $this->testPage->getTitle()->getPreviousRevisionID( 123456789 ) );
+       }
+
        /**
         * @covers Revision::getNext
         */
@@ -640,6 +668,33 @@ abstract class RevisionDbTestBase extends MediaWikiTestCase {
                $this->assertEquals( $rev2->getId(), $rev1->getNext()->getId() );
        }
 
+       /**
+        * @covers Title::getNextRevisionID
+        * @covers Title::getRelativeRevisionID
+        * @covers MediaWiki\Revision\RevisionStore::getNextRevision
+        * @covers MediaWiki\Revision\RevisionStore::getRelativeRevision
+        */
+       public function testTitleGetNextRevisionID() {
+               $title = $this->testPage->getTitle();
+
+               $origId = $this->testPage->getLatest();
+
+               $this->assertFalse( $title->getNextRevisionID( $origId ) );
+
+               $this->testPage->doEditContent( new WikitextContent( __METHOD__ ), __METHOD__ );
+               $newId = $this->testPage->getLatest();
+
+               $this->assertSame( $this->testPage->getLatest(), $title->getNextRevisionID( $origId ) );
+       }
+
+       /**
+        * @covers Title::getNextRevisionID
+        * @covers Title::getRelativeRevisionID
+        */
+       public function testTitleGetNextRevisionID_invalid() {
+               $this->assertFalse( $this->testPage->getTitle()->getNextRevisionID( 123456789 ) );
+       }
+
        /**
         * @covers Revision::newNullRevision
         */