From 7d8a9595438f037420a5eb9ee0db9aea988e6189 Mon Sep 17 00:00:00 2001 From: daniel Date: Sun, 30 Sep 2018 23:34:59 +0200 Subject: [PATCH] Allow getRevisionText to function without the text table. 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 | 64 ++++++++++++++++--- tests/phpunit/includes/RevisionDbTestBase.php | 34 ++++++++++ tests/phpunit/includes/RevisionMcrDbTest.php | 6 ++ .../includes/RevisionMcrReadNewDbTest.php | 14 ++++ tests/phpunit/includes/RevisionTest.php | 45 ++++++------- 5 files changed, 129 insertions(+), 34 deletions(-) diff --git a/includes/Revision.php b/includes/Revision.php index e8fe8bd688..6d1812a98e 100644 --- a/includes/Revision.php +++ b/includes/Revision.php @@ -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 ) diff --git a/tests/phpunit/includes/RevisionDbTestBase.php b/tests/phpunit/includes/RevisionDbTestBase.php index cc166a39fb..e5e5551e9f 100644 --- a/tests/phpunit/includes/RevisionDbTestBase.php +++ b/tests/phpunit/includes/RevisionDbTestBase.php @@ -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 ) ); + } + } diff --git a/tests/phpunit/includes/RevisionMcrDbTest.php b/tests/phpunit/includes/RevisionMcrDbTest.php index d6ac35bfac..2da2275202 100644 --- a/tests/phpunit/includes/RevisionMcrDbTest.php +++ b/tests/phpunit/includes/RevisionMcrDbTest.php @@ -46,4 +46,10 @@ class RevisionMcrDbTest extends RevisionDbTestBase { yield [ $rec, 789 ]; } + public function provideGetRevisionText() { + yield 'no text table' => [ + [] + ]; + } + } diff --git a/tests/phpunit/includes/RevisionMcrReadNewDbTest.php b/tests/phpunit/includes/RevisionMcrReadNewDbTest.php index df54f56c31..64de85451f 100644 --- a/tests/phpunit/includes/RevisionMcrReadNewDbTest.php +++ b/tests/phpunit/includes/RevisionMcrReadNewDbTest.php @@ -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' ] ] + ] + ]; + } + } diff --git a/tests/phpunit/includes/RevisionTest.php b/tests/phpunit/includes/RevisionTest.php index 5868b8d38e..c053104d6f 100644 --- a/tests/phpunit/includes/RevisionTest.php +++ b/tests/phpunit/includes/RevisionTest.php @@ -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' ], -- 2.20.1