Allow getRevisionText to function without the text table.
authordaniel <dkinzler@wikimedia.org>
Sun, 30 Sep 2018 21:34:59 +0000 (23:34 +0200)
committerBrad Jorsch <bjorsch@wikimedia.org>
Wed, 17 Oct 2018 14:54:39 +0000 (10:54 -0400)
Without this patch, getRevisionText would fail silently (by
returning false) when the text table no longer gets joined,
due to the switch to the new MCR schema.

Bug: T205808
Change-Id: Iffc25c82a5d2b865c28070c76156d39d390cc675

includes/Revision.php
tests/phpunit/includes/RevisionDbTestBase.php
tests/phpunit/includes/RevisionMcrDbTest.php
tests/phpunit/includes/RevisionMcrReadNewDbTest.php
tests/phpunit/includes/RevisionTest.php

index e8fe8bd..6d1812a 100644 (file)
@@ -61,8 +61,13 @@ class Revision implements IDBAccessObject {
        /**
         * @return RevisionStore
         */
-       protected static function getRevisionStore() {
-               return MediaWikiServices::getInstance()->getRevisionStore();
+       protected static function getRevisionStore( $wiki = false ) {
+               if ( $wiki ) {
+                       return MediaWikiServices::getInstance()->getRevisionStoreFactory()
+                               ->getRevisionStore( $wiki );
+               } else {
+                       return MediaWikiServices::getInstance()->getRevisionStore();
+               }
        }
 
        /**
@@ -1036,10 +1041,17 @@ class Revision implements IDBAccessObject {
        /**
         * Get revision text associated with an old or archive row
         *
-        * Both the flags and the text field must be included. Including the old_id
+        * If the text field is not included, this uses RevisionStore to load the appropriate slot
+        * and return its serialized content. This is the default backwards-compatibility behavior
+        * when reading from the MCR aware database schema is enabled. For this to work, either
+        * the revision ID or the page ID must be included in the row.
+        *
+        * When using the old text field, the flags field must also be set. Including the old_id
         * field will activate cache usage as long as the $wiki parameter is not set.
         *
-        * @param stdClass $row The text data
+        * @deprecated since 1.32, use RevisionStore::newRevisionFromRow instead.
+        *
+        * @param stdClass $row The text data. If a falsy value is passed instead, false is returned.
         * @param string $prefix Table prefix (default 'old_')
         * @param string|bool $wiki The name of the wiki to load the revision text from
         *   (same as the wiki $row was loaded from) or false to indicate the local
@@ -1048,19 +1060,51 @@ class Revision implements IDBAccessObject {
         * @return string|false Text the text requested or false on failure
         */
        public static function getRevisionText( $row, $prefix = 'old_', $wiki = false ) {
+               global $wgMultiContentRevisionSchemaMigrationStage;
+
+               if ( !$row ) {
+                       return false;
+               }
+
                $textField = $prefix . 'text';
                $flagsField = $prefix . 'flags';
 
-               if ( isset( $row->$flagsField ) ) {
-                       $flags = explode( ',', $row->$flagsField );
+               if ( isset( $row->$textField ) ) {
+                       if ( !( $wgMultiContentRevisionSchemaMigrationStage & SCHEMA_COMPAT_WRITE_OLD ) ) {
+                               // The text field was read, but it's no longer being populated!
+                               // We could gloss over this by using the text when it's there and loading
+                               // if when it's not, but it seems preferable to complain loudly about a
+                               // query that is no longer guaranteed to work reliably.
+                               throw new LogicException(
+                                       'Cannot use ' . __METHOD__ . ' with the ' . $textField . ' field when'
+                                       . ' $wgMultiContentRevisionSchemaMigrationStage does not include'
+                                       . ' SCHEMA_COMPAT_WRITE_OLD. The field may not be populated for all revisions!'
+                               );
+                       }
+
+                       $text = $row->$textField;
                } else {
-                       $flags = [];
+                       // Missing text field, we are probably looking at the MCR-enabled DB schema.
+
+                       if ( !( $wgMultiContentRevisionSchemaMigrationStage & SCHEMA_COMPAT_WRITE_OLD ) ) {
+                               // This method should no longer be used with the new schema. Ideally, we
+                               // would already trigger a deprecation warning when SCHEMA_COMPAT_READ_NEW is set.
+                               wfDeprecated( __METHOD__ . ' (MCR without SCHEMA_COMPAT_WRITE_OLD)', '1.32' );
+                       }
+
+                       $store = self::getRevisionStore( $wiki );
+                       $rev = $prefix === 'ar_'
+                               ? $store->newRevisionFromArchiveRow( $row )
+                               : $store->newRevisionFromRow( $row );
+
+                       $content = $rev->getContent( SlotRecord::MAIN );
+                       return $content ? $content->serialize() : false;
                }
 
-               if ( isset( $row->$textField ) ) {
-                       $text = $row->$textField;
+               if ( isset( $row->$flagsField ) ) {
+                       $flags = explode( ',', $row->$flagsField );
                } else {
-                       return false;
+                       $flags = [];
                }
 
                $cacheKey = isset( $row->old_id )
index cc166a3..e5e5551 100644 (file)
@@ -1601,4 +1601,38 @@ abstract class RevisionDbTestBase extends MediaWikiTestCase {
                $this->assertSame( $expected, $rev->getTextId() );
        }
 
+       public function provideGetRevisionText() {
+               yield [
+                       [ 'text' ]
+               ];
+       }
+
+       /**
+        * @dataProvider provideGetRevisionText
+        * @covers Revision::getRevisionText
+        */
+       public function testGetRevisionText( array $queryInfoOptions, array $queryInfoExtra = [] ) {
+               $rev = $this->testPage->getRevisionRecord();
+
+               $queryInfo = Revision::getQueryInfo( $queryInfoOptions );
+               $queryInfo['tables'] = array_merge( $queryInfo['tables'], $queryInfoExtra['tables'] ?? [] );
+               $queryInfo['fields'] = array_merge( $queryInfo['fields'], $queryInfoExtra['fields'] ?? [] );
+               $queryInfo['joins'] = array_merge( $queryInfo['joins'], $queryInfoExtra['joins'] ?? [] );
+
+               $conds = [ 'rev_id' => $rev->getId() ];
+               $row = $this->db->selectRow(
+                       $queryInfo['tables'],
+                       $queryInfo['fields'],
+                       $conds,
+                       __METHOD__,
+                       [],
+                       $queryInfo['joins']
+               );
+
+               $expected = $rev->getContent( SlotRecord::MAIN )->serialize();
+
+               $this->hideDeprecated( 'Revision::getRevisionText (MCR without SCHEMA_COMPAT_WRITE_OLD)' );
+               $this->assertSame( $expected, Revision::getRevisionText( $row ) );
+       }
+
 }
index d6ac35b..2da2275 100644 (file)
@@ -46,4 +46,10 @@ class RevisionMcrDbTest extends RevisionDbTestBase {
                yield [ $rec, 789 ];
        }
 
+       public function provideGetRevisionText() {
+               yield 'no text table' => [
+                       []
+               ];
+       }
+
 }
index df54f56..64de854 100644 (file)
@@ -42,4 +42,18 @@ class RevisionMcrReadNewDbTest extends RevisionDbTestBase {
                yield [ $rec, 789 ];
        }
 
+       public function provideGetRevisionText() {
+               yield 'no text table' => [
+                       []
+               ];
+               yield 'force text table' => [
+                       [],
+                       [
+                               'tables' => [ 'text' ],
+                               'fields' => [ 'old_id', 'old_text', 'old_flags', 'rev_text_id' ],
+                               'joins' => [ 'text' => [ 'INNER JOIN', 'old_id=rev_text_id' ] ]
+                       ]
+               ];
+       }
+
 }
index 5868b8d..c053104 100644 (file)
@@ -289,16 +289,6 @@ class RevisionTest extends MediaWikiTestCase {
                Wikimedia\restoreWarnings();
        }
 
-       public function provideGetRevisionText() {
-               yield 'Generic test' => [
-                       'This is a goat of revision text.',
-                       [
-                               'old_flags' => '',
-                               'old_text' => 'This is a goat of revision text.',
-                       ],
-               ];
-       }
-
        public function provideGetId() {
                yield [
                        [],
@@ -365,6 +355,20 @@ class RevisionTest extends MediaWikiTestCase {
                $this->assertSame( $expected, $rev->getParentId() );
        }
 
+       public function provideGetRevisionText() {
+               yield 'Generic test' => [
+                       'This is a goat of revision text.',
+                       (object)[
+                               'old_flags' => '',
+                               'old_text' => 'This is a goat of revision text.',
+                       ],
+               ];
+               yield 'garbage in, garbage out' => [
+                       false,
+                       false,
+               ];
+       }
+
        /**
         * @covers Revision::getRevisionText
         * @dataProvider provideGetRevisionText
@@ -372,13 +376,13 @@ class RevisionTest extends MediaWikiTestCase {
        public function testGetRevisionText( $expected, $rowData, $prefix = 'old_', $wiki = false ) {
                $this->assertEquals(
                        $expected,
-                       Revision::getRevisionText( (object)$rowData, $prefix, $wiki ) );
+                       Revision::getRevisionText( $rowData, $prefix, $wiki ) );
        }
 
        public function provideGetRevisionTextWithZlibExtension() {
                yield 'Generic gzip test' => [
                        'This is a small goat of revision text.',
-                       [
+                       (object)[
                                'old_flags' => 'gzip',
                                'old_text' => gzdeflate( 'This is a small goat of revision text.' ),
                        ],
@@ -397,7 +401,7 @@ class RevisionTest extends MediaWikiTestCase {
        public function provideGetRevisionTextWithZlibExtension_badData() {
                yield 'Generic gzip test' => [
                        'This is a small goat of revision text.',
-                       [
+                       (object)[
                                'old_flags' => 'gzip',
                                'old_text' => 'DEAD BEEF',
                        ],
@@ -481,7 +485,7 @@ class RevisionTest extends MediaWikiTestCase {
                        "Wiki est l'\xc3\xa9cole superieur !",
                        'fr',
                        'iso-8859-1',
-                       [
+                       (object)[
                                'old_flags' => 'utf-8',
                                'old_text' => "Wiki est l'\xc3\xa9cole superieur !",
                        ]
@@ -490,7 +494,7 @@ class RevisionTest extends MediaWikiTestCase {
                        "Wiki est l'\xc3\xa9cole superieur !",
                        'fr',
                        'iso-8859-1',
-                       [
+                       (object)[
                                'old_flags' => '',
                                'old_text' => "Wiki est l'\xe9cole superieur !",
                        ]
@@ -519,7 +523,7 @@ class RevisionTest extends MediaWikiTestCase {
                        "Wiki est l'\xc3\xa9cole superieur !",
                        'fr',
                        'iso-8859-1',
-                       [
+                       (object)[
                                'old_flags' => 'gzip,utf-8',
                                'old_text' => gzdeflate( "Wiki est l'\xc3\xa9cole superieur !" ),
                        ]
@@ -528,7 +532,7 @@ class RevisionTest extends MediaWikiTestCase {
                        "Wiki est l'\xc3\xa9cole superieur !",
                        'fr',
                        'iso-8859-1',
-                       [
+                       (object)[
                                'old_flags' => 'gzip',
                                'old_text' => gzdeflate( "Wiki est l'\xe9cole superieur !" ),
                        ]
@@ -729,13 +733,6 @@ class RevisionTest extends MediaWikiTestCase {
                );
        }
 
-       /**
-        * @covers Revision::getRevisionText
-        */
-       public function testGetRevisionText_returnsFalseWhenNoTextField() {
-               $this->assertFalse( Revision::getRevisionText( new stdClass() ) );
-       }
-
        public function provideTestGetRevisionText_returnsDecompressedTextFieldWhenNotExternal() {
                yield 'Just text' => [
                        (object)[ 'old_text' => 'SomeText' ],