Merge "Pass $key into CommentStore methods and use MediawikiServices"
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Tue, 6 Feb 2018 15:31:59 +0000 (15:31 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Tue, 6 Feb 2018 15:31:59 +0000 (15:31 +0000)
1  2 
RELEASE-NOTES-1.31
includes/ServiceWiring.php
includes/Storage/RevisionStore.php
includes/Title.php
includes/api/ApiQueryBase.php
tests/phpunit/includes/RevisionTest.php
tests/phpunit/includes/Storage/RevisionStoreTest.php

diff --combined RELEASE-NOTES-1.31
@@@ -211,11 -211,18 +211,20 @@@ changes to languages because of Phabric
    * OutputPage::setSquidMaxage(); deprecated in 1.27
    * OutputPage::readOnlyPage(); deprecated in 1.25
    * OutputPage::rateLimited(); deprecated in 1.25
 +  * Additionally, the protected OutputPage::$mExtStyles array, only accessed through
 +    the above and with no known uses, was removed.
  * The no-op method Skin::showIPinHeader(), deprecated in 1.27, was removed.
  * \ObjectFactory (no namespace) is deprecated, the namespaced \Wikimedia\ObjectFactory
    from the wikimedia/object-factory library should be used instead.
+ * CommentStore::newKey is deprecated. Get an instance from MediaWikiServices instead.
+ * The following CommentStore methods have had their signatures changed to introduce a $key parameter,
+   usage of the methods on instances retrieved from CommentStore::newKey will remain unchanged but deprecated:
+   * CommentStore::getFields
+   * CommentStore::getJoin
+   * CommentStore::getComment
+   * CommentStore::getCommentLegacy
+   * CommentStore::insert
+   * CommentStore::insertWithTemplate
  
  == Compatibility ==
  MediaWiki 1.31 requires PHP 5.5.9 or later. Although HHVM 3.18.5 or later is supported,
@@@ -477,8 -477,6 +477,8 @@@ return 
                        $services->getMainWANObjectCache()
                );
  
 +              $store->setLogger( LoggerFactory::getInstance( 'RevisionStore' ) );
 +
                $config = $services->getMainConfig();
                $store->setContentHandlerUseDB( $config->get( 'ContentHandlerUseDB' ) );
  
                return new \MediaWiki\Http\HttpRequestFactory();
        },
  
+       'CommentStore' => function ( MediaWikiServices $services ) {
+               global $wgContLang;
+               return new CommentStore(
+                       $wgContLang,
+                       $services->getMainConfig()->get( 'CommentTableSchemaMigrationStage' )
+               );
+       }
        ///////////////////////////////////////////////////////////////////////////
        // NOTE: When adding a service here, don't forget to add a getter function
        // in the MediaWikiServices class. The convenience getter should just call
@@@ -42,9 -42,6 +42,9 @@@ use MediaWiki\User\UserIdentityValue
  use Message;
  use MWException;
  use MWUnknownContentModelException;
 +use Psr\Log\LoggerAwareInterface;
 +use Psr\Log\LoggerInterface;
 +use Psr\Log\NullLogger;
  use RecentChange;
  use stdClass;
  use Title;
@@@ -64,8 -61,7 +64,8 @@@ use Wikimedia\Rdbms\LoadBalancer
   * @note This was written to act as a drop-in replacement for the corresponding
   *       static methods in Revision.
   */
 -class RevisionStore implements IDBAccessObject, RevisionFactory, RevisionLookup {
 +class RevisionStore
 +      implements IDBAccessObject, RevisionFactory, RevisionLookup, LoggerAwareInterface {
  
        /**
         * @var SqlBlobStore
         */
        private $cache;
  
 +      /**
 +       * @var LoggerInterface
 +       */
 +      private $logger;
 +
        /**
         * @todo $blobStore should be allowed to be any BlobStore!
         *
                $this->blobStore = $blobStore;
                $this->cache = $cache;
                $this->wikiId = $wikiId;
 +              $this->logger = new NullLogger();
 +      }
 +
 +      public function setLogger( LoggerInterface $logger ) {
 +              $this->logger = $logger;
        }
  
        /**
         * @return Title
         * @throws RevisionAccessException
         */
 -      public function getTitle( $pageId, $revId, $queryFlags = 0 ) {
 +      public function getTitle( $pageId, $revId, $queryFlags = self::READ_NORMAL ) {
                if ( !$pageId && !$revId ) {
                        throw new InvalidArgumentException( '$pageId and $revId cannot both be 0 or null' );
                }
  
 -              list( $dbMode, $dbOptions, , ) = DBAccessObjectUtils::getDBOptions( $queryFlags );
 -              $titleFlags = $dbMode == DB_MASTER ? Title::GAID_FOR_UPDATE : 0;
 -              $title = null;
 +              // This method recalls itself with READ_LATEST if READ_NORMAL doesn't get us a Title
 +              // So ignore READ_LATEST_IMMUTABLE flags and handle the fallback logic in this method
 +              if ( DBAccessObjectUtils::hasFlags( $queryFlags, self::READ_LATEST_IMMUTABLE ) ) {
 +                      $queryFlags = self::READ_NORMAL;
 +              }
 +
 +              $canUseTitleNewFromId = ( $pageId !== null && $pageId > 0 && $this->wikiId === false );
 +              list( $dbMode, $dbOptions ) = DBAccessObjectUtils::getDBOptions( $queryFlags );
 +              $titleFlags = ( $dbMode == DB_MASTER ? Title::GAID_FOR_UPDATE : 0 );
  
                // Loading by ID is best, but Title::newFromID does not support that for foreign IDs.
 -              if ( $pageId !== null && $pageId > 0 && $this->wikiId === false ) {
 +              if ( $canUseTitleNewFromId ) {
                        // TODO: better foreign title handling (introduce TitleFactory)
                        $title = Title::newFromID( $pageId, $titleFlags );
 +                      if ( $title ) {
 +                              return $title;
 +                      }
                }
  
                // rev_id is defined as NOT NULL, but this revision may not yet have been inserted.
 -              if ( !$title && $revId !== null && $revId > 0 ) {
 +              $canUseRevId = ( $revId !== null && $revId > 0 );
 +
 +              if ( $canUseRevId ) {
                        $dbr = $this->getDBConnectionRef( $dbMode );
                        // @todo: Title::getSelectFields(), or Title::getQueryInfo(), or something like that
                        $row = $dbr->selectRow(
                        );
                        if ( $row ) {
                                // TODO: better foreign title handling (introduce TitleFactory)
 -                              $title = Title::newFromRow( $row );
 +                              return Title::newFromRow( $row );
                        }
                }
  
 -              if ( !$title ) {
 -                      throw new RevisionAccessException(
 -                              "Could not determine title for page ID $pageId and revision ID $revId"
 -                      );
 +              // If we still don't have a title, fallback to master if that wasn't already happening.
 +              if ( $dbMode !== DB_MASTER ) {
 +                      $title = $this->getTitle( $pageId, $revId, self::READ_LATEST );
 +                      if ( $title ) {
 +                              $this->logger->info(
 +                                      __METHOD__ . ' fell back to READ_LATEST and got a Title.',
 +                                      [ 'trace' => wfDebugBacktrace() ]
 +                              );
 +                              return $title;
 +                      }
                }
  
 -              return $title;
 +              throw new RevisionAccessException(
 +                      "Could not determine title for page ID $pageId and revision ID $revId"
 +              );
        }
  
        /**
                }
  
                list( $commentFields, $commentCallback ) =
-                       CommentStore::newKey( 'rev_comment' )->insertWithTempTable( $dbw, $comment );
+                       CommentStore::getStore()->insertWithTempTable( $dbw, 'rev_comment', $comment );
                $row += $commentFields;
  
                if ( $this->contentHandlerUseDB ) {
  
                $user = $this->getUserIdentityFromRowObject( $row, 'ar_' );
  
-               $comment = CommentStore::newKey( 'ar_comment' )
+               $comment = CommentStore::getStore()
                        // Legacy because $row may have come from self::selectFields()
-                       ->getCommentLegacy( $this->getDBConnection( DB_REPLICA ), $row, true );
+                       ->getCommentLegacy( $this->getDBConnection( DB_REPLICA ), 'ar_comment', $row, true );
  
                $mainSlot = $this->emulateMainSlot_1_29( $row, $queryFlags, $title );
                $slots = new RevisionSlots( [ 'main' => $mainSlot ] );
  
                $user = $this->getUserIdentityFromRowObject( $row );
  
-               $comment = CommentStore::newKey( 'rev_comment' )
+               $comment = CommentStore::getStore()
                        // Legacy because $row may have come from self::selectFields()
-                       ->getCommentLegacy( $this->getDBConnection( DB_REPLICA ), $row, true );
+                       ->getCommentLegacy( $this->getDBConnection( DB_REPLICA ), 'rev_comment', $row, true );
  
                $mainSlot = $this->emulateMainSlot_1_29( $row, $queryFlags, $title );
                $slots = new RevisionSlots( [ 'main' => $mainSlot ] );
                        'rev_sha1',
                ] );
  
-               $commentQuery = CommentStore::newKey( 'rev_comment' )->getJoin();
+               $commentQuery = CommentStore::getStore()->getJoin( 'rev_comment' );
                $ret['tables'] = array_merge( $ret['tables'], $commentQuery['tables'] );
                $ret['fields'] = array_merge( $ret['fields'], $commentQuery['fields'] );
                $ret['joins'] = array_merge( $ret['joins'], $commentQuery['joins'] );
         *   - joins: (array) to include in the `$join_conds` to `IDatabase->select()`
         */
        public function getArchiveQueryInfo() {
-               $commentQuery = CommentStore::newKey( 'ar_comment' )->getJoin();
+               $commentQuery = CommentStore::getStore()->getJoin( 'ar_comment' );
                $ret = [
                        'tables' => [ 'archive' ] + $commentQuery['tables'],
                        'fields' => [
diff --combined includes/Title.php
@@@ -2735,8 -2735,8 +2735,8 @@@ class Title implements LinkTarget 
  
                if ( $this->mTitleProtection === null ) {
                        $dbr = wfGetDB( DB_REPLICA );
-                       $commentStore = new CommentStore( 'pt_reason' );
-                       $commentQuery = $commentStore->getJoin();
+                       $commentStore = CommentStore::getStore();
+                       $commentQuery = $commentStore->getJoin( 'pt_reason' );
                        $res = $dbr->select(
                                [ 'protected_titles' ] + $commentQuery['tables'],
                                [
                                        'user' => $row['user'],
                                        'expiry' => $dbr->decodeExpiry( $row['expiry'] ),
                                        'permission' => $row['permission'],
-                                       'reason' => $commentStore->getComment( $row )->text,
+                                       'reason' => $commentStore->getComment( 'pt_reason', $row )->text,
                                ];
                        } else {
                                $this->mTitleProtection = false;
                }
  
                if ( $this->isExternal() ) {
 -                      return true;  // any interwiki link might be viewable, for all we know
 +                      return true; // any interwiki link might be viewable, for all we know
                }
  
                switch ( $this->mNamespace ) {
@@@ -255,7 -255,7 +255,7 @@@ abstract class ApiQueryBase extends Api
        /**
         * Equivalent to addWhere(array($field => $value))
         * @param string $field Field name
 -       * @param string|string[] $value Value; ignored if null or empty array;
 +       * @param string|string[] $value Value; ignored if null or empty array
         */
        protected function addWhereFld( $field, $value ) {
                if ( $value !== null && !( is_array( $value ) && !$value ) ) {
                                'ipb_expiry',
                                'ipb_timestamp'
                        ] );
-                       $commentQuery = CommentStore::newKey( 'ipb_reason' )->getJoin();
+                       $commentQuery = CommentStore::getStore()->getJoin( 'ipb_reason' );
                        $this->addTables( $commentQuery['tables'] );
                        $this->addFields( $commentQuery['fields'] );
                        $this->addJoinConds( $commentQuery['joins'] );
@@@ -317,8 -317,6 +317,8 @@@ class RevisionTest extends MediaWikiTes
         * @covers \MediaWiki\Storage\RevisionStore::newMutableRevisionFromArray
         */
        public function testConstructFromRowWithBadPageId() {
 +              $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD );
 +              $this->overrideMwServices();
                MediaWiki\suppressWarnings();
                $rev = new Revision( (object)[ 'rev_page' => 77777777 ] );
                $this->assertSame( 77777777, $rev->getPage() );
         * @covers Revision::loadFromTitle
         */
        public function testLoadFromTitle() {
 +              $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD );
 +              $this->overrideMwServices();
                $title = $this->getMockTitle();
  
                $conditions = [
                $this->hideDeprecated( 'Revision::selectFields' );
                $this->setMwGlobals( 'wgContentHandlerUseDB', $contentHandlerUseDB );
                $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD );
+               $this->overrideMwServices();
                $this->assertEquals( $expected, Revision::selectFields() );
        }
  
                $this->hideDeprecated( 'Revision::selectArchiveFields' );
                $this->setMwGlobals( 'wgContentHandlerUseDB', $contentHandlerUseDB );
                $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD );
+               $this->overrideMwServices();
                $this->assertEquals( $expected, Revision::selectArchiveFields() );
        }
  
@@@ -224,9 -224,10 +224,10 @@@ class RevisionStoreTest extends MediaWi
         * @covers \MediaWiki\Storage\RevisionStore::getQueryInfo
         */
        public function testGetQueryInfo( $contentHandlerUseDb, $options, $expected ) {
+               $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD );
+               $this->overrideMwServices();
                $store = $this->getRevisionStore();
                $store->setContentHandlerUseDB( $contentHandlerUseDb );
-               $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD );
                $this->assertEquals( $expected, $store->getQueryInfo( $options ) );
        }
  
         * @covers \MediaWiki\Storage\RevisionStore::getArchiveQueryInfo
         */
        public function testGetArchiveQueryInfo_contentHandlerDb() {
+               $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD );
+               $this->overrideMwServices();
                $store = $this->getRevisionStore();
                $store->setContentHandlerUseDB( true );
-               $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD );
                $this->assertEquals(
                        [
                                'tables' => [
         * @covers \MediaWiki\Storage\RevisionStore::getArchiveQueryInfo
         */
        public function testGetArchiveQueryInfo_noContentHandlerDb() {
+               $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD );
+               $this->overrideMwServices();
                $store = $this->getRevisionStore();
                $store->setContentHandlerUseDB( false );
-               $this->setMwGlobals( 'wgCommentTableSchemaMigrationStage', MIGRATION_OLD );
                $this->assertEquals(
                        [
                                'tables' => [
                $this->assertSame( 'Food', $title->getDBkey() );
        }
  
 +      public function testGetTitle_successFromPageIdOnFallback() {
 +              $mockLoadBalancer = $this->getMockLoadBalancer();
 +              // Title calls wfGetDB() so we have to set the main service
 +              $this->setService( 'DBLoadBalancer', $mockLoadBalancer );
 +
 +              $db = $this->getMockDatabase();
 +              // Title calls wfGetDB() which uses a regular Connection
 +              // Assert that the first call uses a REPLICA and the second falls back to master
 +              $mockLoadBalancer->expects( $this->exactly( 2 ) )
 +                      ->method( 'getConnection' )
 +                      ->willReturn( $db );
 +              // RevisionStore getTitle uses a ConnectionRef
 +              $mockLoadBalancer->expects( $this->atLeastOnce() )
 +                      ->method( 'getConnectionRef' )
 +                      ->willReturn( $db );
 +
 +              // First call to Title::newFromID, faking no result (db lag?)
 +              $db->expects( $this->at( 0 ) )
 +                      ->method( 'selectRow' )
 +                      ->with(
 +                              'page',
 +                              $this->anything(),
 +                              [ 'page_id' => 1 ]
 +                      )
 +                      ->willReturn( false );
 +
 +              // First select using rev_id, faking no result (db lag?)
 +              $db->expects( $this->at( 1 ) )
 +                      ->method( 'selectRow' )
 +                      ->with(
 +                              [ 'revision', 'page' ],
 +                              $this->anything(),
 +                              [ 'rev_id' => 2 ]
 +                      )
 +                      ->willReturn( false );
 +
 +              // Second call to Title::newFromID, no result
 +              $db->expects( $this->at( 2 ) )
 +                      ->method( 'selectRow' )
 +                      ->with(
 +                              'page',
 +                              $this->anything(),
 +                              [ 'page_id' => 1 ]
 +                      )
 +                      ->willReturn( (object)[
 +                              'page_namespace' => '2',
 +                              'page_title' => 'Foodey',
 +                      ] );
 +
 +              $store = $this->getRevisionStore( $mockLoadBalancer );
 +              $title = $store->getTitle( 1, 2, RevisionStore::READ_NORMAL );
 +
 +              $this->assertSame( 2, $title->getNamespace() );
 +              $this->assertSame( 'Foodey', $title->getDBkey() );
 +      }
 +
        public function testGetTitle_successFromRevId() {
                $mockLoadBalancer = $this->getMockLoadBalancer();
                // Title calls wfGetDB() so we have to set the main service
                $this->assertSame( 'Food2', $title->getDBkey() );
        }
  
 -      /**
 -       * @covers \MediaWiki\Storage\RevisionStore::getTitle
 -       */
 -      public function testGetTitle_throwsExceptionAfterFallbacks() {
 +      public function testGetTitle_successFromRevIdOnFallback() {
                $mockLoadBalancer = $this->getMockLoadBalancer();
                // Title calls wfGetDB() so we have to set the main service
                $this->setService( 'DBLoadBalancer', $mockLoadBalancer );
  
                $db = $this->getMockDatabase();
                // Title calls wfGetDB() which uses a regular Connection
 -              $mockLoadBalancer->expects( $this->atLeastOnce() )
 +              // Assert that the first call uses a REPLICA and the second falls back to master
 +              $mockLoadBalancer->expects( $this->exactly( 2 ) )
                        ->method( 'getConnection' )
                        ->willReturn( $db );
                // RevisionStore getTitle uses a ConnectionRef
                        )
                        ->willReturn( false );
  
 +              // Second call to Title::newFromID, no result
 +              $db->expects( $this->at( 2 ) )
 +                      ->method( 'selectRow' )
 +                      ->with(
 +                              'page',
 +                              $this->anything(),
 +                              [ 'page_id' => 1 ]
 +                      )
 +                      ->willReturn( false );
 +
 +              // Second select using rev_id, result
 +              $db->expects( $this->at( 3 ) )
 +                      ->method( 'selectRow' )
 +                      ->with(
 +                              [ 'revision', 'page' ],
 +                              $this->anything(),
 +                              [ 'rev_id' => 2 ]
 +                      )
 +                      ->willReturn( (object)[
 +                              'page_namespace' => '2',
 +                              'page_title' => 'Foodey',
 +                      ] );
 +
 +              $store = $this->getRevisionStore( $mockLoadBalancer );
 +              $title = $store->getTitle( 1, 2, RevisionStore::READ_NORMAL );
 +
 +              $this->assertSame( 2, $title->getNamespace() );
 +              $this->assertSame( 'Foodey', $title->getDBkey() );
 +      }
 +
 +      /**
 +       * @covers \MediaWiki\Storage\RevisionStore::getTitle
 +       */
 +      public function testGetTitle_correctFallbackAndthrowsExceptionAfterFallbacks() {
 +              $mockLoadBalancer = $this->getMockLoadBalancer();
 +              // Title calls wfGetDB() so we have to set the main service
 +              $this->setService( 'DBLoadBalancer', $mockLoadBalancer );
 +
 +              $db = $this->getMockDatabase();
 +              // Title calls wfGetDB() which uses a regular Connection
 +              // Assert that the first call uses a REPLICA and the second falls back to master
 +
 +              // RevisionStore getTitle uses getConnectionRef
 +              // Title::newFromID uses getConnection
 +              foreach ( [ 'getConnection', 'getConnectionRef' ] as $method ) {
 +                      $mockLoadBalancer->expects( $this->exactly( 2 ) )
 +                              ->method( $method )
 +                              ->willReturnCallback( function ( $masterOrReplica ) use ( $db ) {
 +                                      static $callCounter = 0;
 +                                      $callCounter++;
 +                                      // The first call should be to a REPLICA, and the second a MASTER.
 +                                      if ( $callCounter === 1 ) {
 +                                              $this->assertSame( DB_REPLICA, $masterOrReplica );
 +                                      } elseif ( $callCounter === 2 ) {
 +                                              $this->assertSame( DB_MASTER, $masterOrReplica );
 +                                      }
 +                                      return $db;
 +                              } );
 +              }
 +              // First and third call to Title::newFromID, faking no result
 +              foreach ( [ 0, 2 ] as $counter ) {
 +                      $db->expects( $this->at( $counter ) )
 +                              ->method( 'selectRow' )
 +                              ->with(
 +                                      'page',
 +                                      $this->anything(),
 +                                      [ 'page_id' => 1 ]
 +                              )
 +                              ->willReturn( false );
 +              }
 +
 +              foreach ( [ 1, 3 ] as $counter ) {
 +                      $db->expects( $this->at( $counter ) )
 +                              ->method( 'selectRow' )
 +                              ->with(
 +                                      [ 'revision', 'page' ],
 +                                      $this->anything(),
 +                                      [ 'rev_id' => 2 ]
 +                              )
 +                              ->willReturn( false );
 +              }
 +
                $store = $this->getRevisionStore( $mockLoadBalancer );
  
                $this->setExpectedException( RevisionAccessException::class );