Merge "When encountering bad blobs, log the text row id."

diff --combined includes/Revision.php
index b9a03f5,c6889ab..89a58f3
--- 1/includes/Revision.php
--- 2/includes/Revision.php
+++ b/includes/Revision.php
@@@ -581,11 -581,11 +581,11 @@@ class Revision implements IDBAccessObje
                                return $row['title'];
                        }
  
 -                      $pageId = isset( $row['page'] ) ? $row['page'] : 0;
 -                      $revId = isset( $row['id'] ) ? $row['id'] : 0;
 +                      $pageId = $row['page'] ?? 0;
 +                      $revId = $row['id'] ?? 0;
                } else {
 -                      $pageId = isset( $row->rev_page ) ? $row->rev_page : 0;
 -                      $revId = isset( $row->rev_id ) ? $row->rev_id : 0;
 +                      $pageId = $row->rev_page ?? 0;
 +                      $revId = $row->rev_id ?? 0;
                }
  
                try {
@@@ -1038,7 -1038,18 +1038,18 @@@
                        ? SqlBlobStore::makeAddressFromTextId( $row->old_id )
                        : null;
  
-               return self::getBlobStore( $wiki )->expandBlob( $text, $flags, $cacheKey );
+               $revisionText = self::getBlobStore( $wiki )->expandBlob( $text, $flags, $cacheKey );
+ 
+               if ( $revisionText === false ) {
+                       if ( isset( $row->old_id ) ) {
+                               wfLogWarning( __METHOD__ . ": Bad data in text row {$row->old_id}! " );
+                       } else {
+                               wfLogWarning( __METHOD__ . ": Bad data in text row! " );
+                       }
+                       return false;
+               }
+ 
+               return $revisionText;
        }
  
        /**
@@@ -1094,6 -1105,8 +1105,6 @@@
  
                // Avoid PHP 7.1 warning of passing $this by reference
                $revision = $this;
 -              // TODO: hard-deprecate in 1.32 (or even 1.31?)
 -              Hooks::run( 'RevisionInsertComplete', [ &$revision, null, null ] );
  
                return $rec->getId();
        }

diff --combined tests/phpunit/includes/RevisionTest.php
index cf2c64a,f50dc80..6de37af
--- 1/tests/phpunit/includes/RevisionTest.php
--- 2/tests/phpunit/includes/RevisionTest.php
+++ b/tests/phpunit/includes/RevisionTest.php
@@@ -17,11 -17,6 +17,11 @@@ use Wikimedia\Rdbms\LoadBalancer
   */
  class RevisionTest extends MediaWikiTestCase {
  
 +      public function setUp() {
 +              parent::setUp();
 +              $this->setMwGlobals( 'wgMultiContentRevisionSchemaMigrationStage', MIGRATION_OLD );
 +      }
 +
        public function provideConstructFromArray() {
                yield 'with text' => [
                        [
@@@ -159,15 -154,16 +159,15 @@@
                                'content' => new WikitextContent( 'GOAT' ),
                                'text_id' => 'someid',
                        ],
 -                      new MWException( "Text already stored in external store (id someid), " .
 -                              "can't serialize content object" )
 +                      new MWException( 'Text already stored in external store (id someid),' )
                ];
                yield 'with bad content object (class)' => [
                        [ 'content' => new stdClass() ],
 -                      new MWException( 'content field must contain a Content object.' )
 +                      new MWException( 'content field must contain a Content object' )
                ];
                yield 'with bad content object (string)' => [
                        [ 'content' => 'ImAGoat' ],
 -                      new MWException( 'content field must contain a Content object.' )
 +                      new MWException( 'content field must contain a Content object' )
                ];
                yield 'bad row format' => [
                        'imastring, not a row',
@@@ -443,6 -439,31 +443,31 @@@
                $this->testGetRevisionText( $expected, $rowData );
        }
  
+       public function provideGetRevisionTextWithZlibExtension_badData() {
+               yield 'Generic gzip test' => [
+                       'This is a small goat of revision text.',
+                       [
+                               'old_flags' => 'gzip',
+                               'old_text' => 'DEAD BEEF',
+                       ],
+               ];
+       }
+ 
+       /**
+        * @covers Revision::getRevisionText
+        * @dataProvider provideGetRevisionTextWithZlibExtension_badData
+        */
+       public function testGetRevisionWithZlibExtension_badData( $expected, $rowData ) {
+               $this->checkPHPExtension( 'zlib' );
+               Wikimedia\suppressWarnings();
+               $this->assertFalse(
+                       Revision::getRevisionText(
+                               (object)$rowData
+                       )
+               );
+               Wikimedia\suppressWarnings( true );
+       }
+ 
        private function getWANObjectCache() {
                return new WANObjectCache( [ 'cache' => new HashBagOStuff() ] );
        }
@@@ -492,9 -513,6 +517,9 @@@
                        $this->getBlobStore(),
                        $cache,
                        MediaWikiServices::getInstance()->getCommentStore(),
 +                      MediaWikiServices::getInstance()->getContentModelStore(),
 +                      MediaWikiServices::getInstance()->getSlotRoleStore(),
 +                      MIGRATION_OLD,
                        MediaWikiServices::getInstance()->getActorMigration()
                );
                return $blobStore;
@@@ -809,6 -827,7 +834,7 @@@
        public function testGetRevisionText_external_returnsFalseWhenNotEnoughUrlParts(
                $text
        ) {
+               Wikimedia\suppressWarnings();
                $this->assertFalse(
                        Revision::getRevisionText(
                                (object)[
@@@ -817,6 -836,7 +843,7 @@@
                                ]
                        )
                );
+               Wikimedia\suppressWarnings( true );
        }
  
        /**
@@@ -1163,60 -1183,10 +1190,60 @@@
                $revisionStore = $this->getRevisionStore();
                $revisionStore->setContentHandlerUseDB( $globals['wgContentHandlerUseDB'] );
                $this->setService( 'RevisionStore', $revisionStore );
 -              $this->assertEquals(
 -                      $expected,
 -                      Revision::getArchiveQueryInfo()
 +
 +              $queryInfo = Revision::getArchiveQueryInfo();
 +
 +              $this->assertArrayEqualsIgnoringIntKeyOrder(
 +                      $expected['tables'],
 +                      $queryInfo['tables']
                );
 +              $this->assertArrayEqualsIgnoringIntKeyOrder(
 +                      $expected['fields'],
 +                      $queryInfo['fields']
 +              );
 +              $this->assertArrayEqualsIgnoringIntKeyOrder(
 +                      $expected['joins'],
 +                      $queryInfo['joins']
 +              );
 +      }
 +
 +      /**
 +       * Assert that the two arrays passed are equal, ignoring the order of the values that integer
 +       * keys.
 +       *
 +       * Note: Failures of this assertion can be slightly confusing as the arrays are actually
 +       * split into a string key array and an int key array before assertions occur.
 +       *
 +       * @param array $expected
 +       * @param array $actual
 +       */
 +      private function assertArrayEqualsIgnoringIntKeyOrder( array $expected, array $actual ) {
 +              $this->objectAssociativeSort( $expected );
 +              $this->objectAssociativeSort( $actual );
 +
 +              // Separate the int key values from the string key values so that assertion failures are
 +              // easier to understand.
 +              $expectedIntKeyValues = [];
 +              $actualIntKeyValues = [];
 +
 +              // Remove all int keys and re add them at the end after sorting by value
 +              // This will result in all int keys being in the same order with same ints at the end of
 +              // the array
 +              foreach ( $expected as $key => $value ) {
 +                      if ( is_int( $key ) ) {
 +                              unset( $expected[$key] );
 +                              $expectedIntKeyValues[] = $value;
 +                      }
 +              }
 +              foreach ( $actual as $key => $value ) {
 +                      if ( is_int( $key ) ) {
 +                              unset( $actual[$key] );
 +                              $actualIntKeyValues[] = $value;
 +                      }
 +              }
 +
 +              $this->assertArrayEquals( $expected, $actual, false, true );
 +              $this->assertArrayEquals( $expectedIntKeyValues, $actualIntKeyValues, false, true );
        }
  
        public function provideGetQueryInfo() {
@@@ -1448,19 -1418,9 +1475,19 @@@
                $revisionStore->setContentHandlerUseDB( $globals['wgContentHandlerUseDB'] );
                $this->setService( 'RevisionStore', $revisionStore );
  
 -              $this->assertEquals(
 -                      $expected,
 -                      Revision::getQueryInfo( $options )
 +              $queryInfo = Revision::getQueryInfo( $options );
 +
 +              $this->assertArrayEqualsIgnoringIntKeyOrder(
 +                      $expected['tables'],
 +                      $queryInfo['tables']
 +              );
 +              $this->assertArrayEqualsIgnoringIntKeyOrder(
 +                      $expected['fields'],
 +                      $queryInfo['fields']
 +              );
 +              $this->assertArrayEqualsIgnoringIntKeyOrder(
 +                      $expected['joins'],
 +                      $queryInfo['joins']
                );
        }