From: Aryeh Gregor Date: Mon, 29 Apr 2019 14:24:58 +0000 (+0300) Subject: Move getPrevious/NextRevision logic out of Title X-Git-Tag: 1.34.0-rc.0~1811 X-Git-Url: http://git.cyclocoop.org/?a=commitdiff_plain;h=090f6daa1b388b772a724984debf7ba287a8bebe;p=lhc%2Fweb%2Fwiklou.git 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 46e85faebb..1050c4d582 100644 --- 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 2b14a21b09..de3c2998b5 100644 --- 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 4feb9f5430..17cafc6a5d 100644 --- 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 515f07d4c5..bdaac7f4a7 100644 --- 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 27baeb2a7e..2203ccbc0b 100644 --- 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 13ddffac14..96e27668ec 100644 --- 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 */