Don't require Title for getTimestampFromId

3e36ba655e3a added an option for passing a page ID to this method of
Revision, and e03787afd91c switched it to a Title and made it mandatory.
This behavior propagated to the method in RevisionStore.  As far as I
can tell, the parameter does not help anything, but it can add a
database query to get the page ID if it's not cached, and impedes
conversion to LinkTarget. I can't figure out any reason to not
completely drop it. I've noted it as deprecated but still supported it
for now for compatibility -- I found one extension that does pass it.
(It's ignored, though, which theoretically would be a behavior change if
someone was passing a Title that didn't match the revision ID.)

While I was at it, I added the method to RevisionLookup, although it's
only used in later patches. Properly I should move that piece to a later
patch, but it didn't seem worth the effort.

I didn't change the Revision method, because the whole Revision class is
deprecated anyway.

Change-Id: I26ef5f2bf828f0f450633b7237d26d888e2c8773

diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34
index 25888cc..46e85fa 100644 (file)
--- a/RELEASE-NOTES-1.34
+++ b/RELEASE-NOTES-1.34
@@ -25,6 +25,7 @@ Some specific notes for MediaWiki 1.34 upgrades are below:
 For notes on 1.33.x and older releases, see HISTORY.
 
 === Configuration changes for system administrators in 1.34 ===
+
 ==== New configuration ====
 * …
 
@@ -41,6 +42,7 @@ For notes on 1.33.x and older releases, see HISTORY.
 * …
 
 === External library changes in 1.34 ===
+
 ==== New external libraries ====
 * …
 
@@ -127,6 +129,8 @@ because of Phabricator reports.
 * The Config argument to ChangesListSpecialPage::checkStructuredFilterUiEnabled
   is deprecated. Pass only the User argument.
 * 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.
 
 === Other changes in 1.34 ===
 * …

diff --git a/includes/Revision.php b/includes/Revision.php
index cbaff90..2b14a21 100644 (file)
--- a/includes/Revision.php
+++ b/includes/Revision.php
@@ -1256,13 +1256,13 @@ class Revision implements IDBAccessObject {
        /**
         * Get rev_timestamp from rev_id, without loading the rest of the row
         *
-        * @param Title $title
+        * @param Title $title (ignored since 1.34)
         * @param int $id
         * @param int $flags
         * @return string|bool False if not found
         */
        static function getTimestampFromId( $title, $id, $flags = 0 ) {
-               return self::getRevisionStore()->getTimestampFromId( $title, $id, $flags );
+               return self::getRevisionStore()->getTimestampFromId( $id, $flags );
        }
 
        /**

diff --git a/includes/Revision/RevisionLookup.php b/includes/Revision/RevisionLookup.php
index db6c7c3..4feb9f5 100644 (file)
--- a/includes/Revision/RevisionLookup.php
+++ b/includes/Revision/RevisionLookup.php
@@ -103,6 +103,18 @@ interface RevisionLookup extends IDBAccessObject {
         */
        public function getNextRevision( RevisionRecord $rev, Title $title = null );
 
+       /**
+        * Get rev_timestamp from rev_id, without loading the rest of the row.
+        *
+        * MCR migration note: this replaces Revision::getTimestampFromId
+        *
+        * @param int $id
+        * @param int $flags
+        * @return string|bool False if not found
+        * @since 1.34 (present earlier in RevisionStore)
+        */
+       public function getTimestampFromId( $id, $flags = 0 );
+
        /**
         * Load a revision based on a known page ID and current revision ID from the DB
         *

diff --git a/includes/Revision/RevisionStore.php b/includes/Revision/RevisionStore.php
index 0329bd1..515f07d 100644 (file)
--- a/includes/Revision/RevisionStore.php
+++ b/includes/Revision/RevisionStore.php
@@ -2658,21 +2658,27 @@ class RevisionStore
        }
 
        /**
-        * Get rev_timestamp from rev_id, without loading the rest of the row
+        * Get rev_timestamp from rev_id, without loading the rest of the row.
+        *
+        * Historically, there was an extra Title parameter that was passed before $id. This is no
+        * longer needed and is deprecated in 1.34.
         *
         * MCR migration note: this replaces Revision::getTimestampFromId
         *
-        * @param Title $title
         * @param int $id
         * @param int $flags
         * @return string|bool False if not found
         */
-       public function getTimestampFromId( $title, $id, $flags = 0 ) {
+       public function getTimestampFromId( $id, $flags = 0 ) {
+               if ( $id instanceof Title ) {
+                       // Old deprecated calling convention supported for backwards compatibility
+                       $id = $flags;
+                       $flags = func_num_args() > 2 ? func_get_arg( 2 ) : 0;
+               }
                $db = $this->getDBConnectionRefForQueryFlags( $flags );
 
-               $conds = [ 'rev_id' => $id ];
-               $conds['rev_page'] = $title->getArticleID();
-               $timestamp = $db->selectField( 'revision', 'rev_timestamp', $conds, __METHOD__ );
+               $timestamp =
+                       $db->selectField( 'revision', 'rev_timestamp', [ 'rev_id' => $id ], __METHOD__ );
 
                return ( $timestamp !== false ) ? wfTimestamp( TS_MW, $timestamp ) : false;
        }

diff --git a/includes/api/ApiSetNotificationTimestamp.php b/includes/api/ApiSetNotificationTimestamp.php
index ba4c6e8..d2bbe7b 100644 (file)
--- a/includes/api/ApiSetNotificationTimestamp.php
+++ b/includes/api/ApiSetNotificationTimestamp.php
@@ -77,8 +77,9 @@ class ApiSetNotificationTimestamp extends ApiBase {
                        $titles = $pageSet->getGoodTitles();
                        $title = reset( $titles );
                        if ( $title ) {
+                               // XXX $title isn't actually used, can we just get rid of the previous six lines?
                                $timestamp = MediaWikiServices::getInstance()->getRevisionStore()
-                                       ->getTimestampFromId( $title, $params['torevid'], IDBAccessObject::READ_LATEST );
+                                       ->getTimestampFromId( $params['torevid'], IDBAccessObject::READ_LATEST );
                                if ( $timestamp ) {
                                        $timestamp = $dbw->timestamp( $timestamp );
                                } else {

diff --git a/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php b/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php
index b183fca..3467153 100644 (file)
--- a/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php
+++ b/tests/phpunit/includes/Revision/RevisionStoreDbTestBase.php
@@ -1416,10 +1416,7 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase {
                        ->value['revision'];
 
                $store = MediaWikiServices::getInstance()->getRevisionStore();
-               $result = $store->getTimestampFromId(
-                       $page->getTitle(),
-                       $rev->getId()
-               );
+               $result = $store->getTimestampFromId( $rev->getId() );
 
                $this->assertSame( $rev->getTimestamp(), $result );
        }
@@ -1434,10 +1431,7 @@ abstract class RevisionStoreDbTestBase extends MediaWikiTestCase {
                        ->value['revision'];
 
                $store = MediaWikiServices::getInstance()->getRevisionStore();
-               $result = $store->getTimestampFromId(
-                       $page->getTitle(),
-                       $rev->getId() + 1
-               );
+               $result = $store->getTimestampFromId( $rev->getId() + 1 );
 
                $this->assertFalse( $result );
        }