Introduce MovePageFactory
authorAryeh Gregor <ayg@aryeh.name>
Wed, 1 May 2019 11:39:45 +0000 (14:39 +0300)
committerAryeh Gregor <ayg@aryeh.name>
Mon, 19 Aug 2019 17:25:31 +0000 (20:25 +0300)
This will help make MovePage more testable.

In the course of abstracting the logic out of ParserFactoryTest to
FactoryArgTestTrait so it could be used in MovePageFactoryTest, I made
them all unit tests instead of integration. This required some
modification to the Parser constructor so that it didn't access
MediaWikiServices unnecessarily.

Change-Id: Idaa1633f32dfedfa37516bb9180cfcfbe7ca31aa

18 files changed:
RELEASE-NOTES-1.34
autoload.php
includes/MediaWikiServices.php
includes/MovePage.php
includes/ServiceWiring.php
includes/Title.php
includes/api/ApiMove.php
includes/page/MovePageFactory.php [new file with mode: 0644]
includes/parser/Parser.php
maintenance/cleanupCaps.php
maintenance/moveBatch.php
tests/common/TestsAutoLoader.php
tests/phpunit/includes/MovePageTest.php
tests/phpunit/includes/parser/ParserFactoryIntegrationTest.php [new file with mode: 0644]
tests/phpunit/includes/parser/ParserFactoryTest.php [deleted file]
tests/phpunit/unit/includes/FactoryArgTestTrait.php [new file with mode: 0644]
tests/phpunit/unit/includes/page/MovePageFactoryTest.php [new file with mode: 0644]
tests/phpunit/unit/includes/parser/ParserFactoryTest.php [new file with mode: 0644]

index fd7e4d9..9a4aeb3 100644 (file)
@@ -449,6 +449,7 @@ because of Phabricator reports.
 * IDatabase::bufferResults() has been deprecated. Use query batching instead.
 * MessageCache::singleton() is deprecated. Use
   MediaWikiServices::getMessageCache().
+* Constructing MovePage directly is deprecated. Use MovePageFactory.
 
 === Other changes in 1.34 ===
 * …
index 52a5eda..acdf8dd 100644 (file)
@@ -913,6 +913,7 @@ $wgAutoloadLocalClasses = [
        'MediaWiki\\MediaWikiServices' => __DIR__ . '/includes/MediaWikiServices.php',
        'MediaWiki\\Navigation\\PrevNextNavigationRenderer' => __DIR__ . '/includes/Navigation/PrevNextNavigationRenderer.php',
        'MediaWiki\\OutputHandler' => __DIR__ . '/includes/OutputHandler.php',
+       'MediaWiki\\Page\\MovePageFactory' => __DIR__ . '/includes/page/MovePageFactory.php',
        'MediaWiki\\ProcOpenError' => __DIR__ . '/includes/exception/ProcOpenError.php',
        'MediaWiki\\Search\\ParserOutputSearchDataExtractor' => __DIR__ . '/includes/search/ParserOutputSearchDataExtractor.php',
        'MediaWiki\\Services\\CannotReplaceActiveServiceException' => __DIR__ . '/includes/libs/services/CannotReplaceActiveServiceException.php',
index e5efd7d..bb9c05f 100644 (file)
@@ -17,11 +17,12 @@ use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface;
 use MediaWiki\Block\BlockManager;
 use MediaWiki\Block\BlockRestrictionStore;
 use MediaWiki\Http\HttpRequestFactory;
+use MediaWiki\Page\MovePageFactory;
 use MediaWiki\Permissions\PermissionManager;
 use MediaWiki\Preferences\PreferencesFactory;
-use MediaWiki\Shell\CommandFactory;
 use MediaWiki\Revision\RevisionRenderer;
 use MediaWiki\Revision\SlotRoleRegistry;
+use MediaWiki\Shell\CommandFactory;
 use MediaWiki\Special\SpecialPageFactory;
 use MediaWiki\Storage\BlobStore;
 use MediaWiki\Storage\BlobStoreFactory;
@@ -706,6 +707,14 @@ class MediaWikiServices extends ServiceContainer {
                return $this->getService( 'MimeAnalyzer' );
        }
 
+       /**
+        * @since 1.34
+        * @return MovePageFactory
+        */
+       public function getMovePageFactory() : MovePageFactory {
+               return $this->getService( 'MovePageFactory' );
+       }
+
        /**
         * @since 1.34
         * @return NamespaceInfo
index 832e24a..f48d92e 100644 (file)
  * @file
  */
 
+use MediaWiki\Config\ServiceOptions;
 use MediaWiki\MediaWikiServices;
+use MediaWiki\Page\MovePageFactory;
+use MediaWiki\Permissions\PermissionManager;
 use MediaWiki\Revision\SlotRecord;
 use Wikimedia\Rdbms\IDatabase;
+use Wikimedia\Rdbms\LoadBalancer;
 
 /**
  * Handles the backend logic of moving a page from one title
@@ -41,9 +45,62 @@ class MovePage {
         */
        protected $newTitle;
 
-       public function __construct( Title $oldTitle, Title $newTitle ) {
+       /**
+        * @var ServiceOptions
+        */
+       protected $options;
+
+       /**
+        * @var LoadBalancer
+        */
+       protected $loadBalancer;
+
+       /**
+        * @var NamespaceInfo
+        */
+       protected $nsInfo;
+
+       /**
+        * @var WatchedItemStore
+        */
+       protected $watchedItems;
+
+       /**
+        * @var PermissionManager
+        */
+       protected $permMgr;
+
+       /**
+        * Calling this directly is deprecated in 1.34. Use MovePageFactory instead.
+        *
+        * @param Title $oldTitle
+        * @param Title $newTitle
+        * @param ServiceOptions|null $options
+        * @param LoadBalancer|null $loadBalancer
+        * @param NamespaceInfo|null $nsInfo
+        * @param WatchedItemStore|null $watchedItems
+        * @param PermissionManager|null $permMgr
+        */
+       public function __construct(
+               Title $oldTitle,
+               Title $newTitle,
+               ServiceOptions $options = null,
+               LoadBalancer $loadBalancer = null,
+               NamespaceInfo $nsInfo = null,
+               WatchedItemStore $watchedItems = null,
+               PermissionManager $permMgr = null
+       ) {
                $this->oldTitle = $oldTitle;
                $this->newTitle = $newTitle;
+               $this->options = $options ??
+                       new ServiceOptions( MovePageFactory::$constructorOptions,
+                               MediaWikiServices::getInstance()->getMainConfig() );
+               $this->loadBalancer =
+                       $loadBalancer ?? MediaWikiServices::getInstance()->getDBLoadBalancer();
+               $this->nsInfo = $nsInfo ?? MediaWikiServices::getInstance()->getNamespaceInfo();
+               $this->watchedItems =
+                       $watchedItems ?? MediaWikiServices::getInstance()->getWatchedItemStore();
+               $this->permMgr = $permMgr ?? MediaWikiServices::getInstance()->getPermissionManager();
        }
 
        /**
@@ -58,10 +115,10 @@ class MovePage {
                $status = new Status();
 
                $errors = wfMergeErrorArrays(
-                       $this->oldTitle->getUserPermissionsErrors( 'move', $user ),
-                       $this->oldTitle->getUserPermissionsErrors( 'edit', $user ),
-                       $this->newTitle->getUserPermissionsErrors( 'move-target', $user ),
-                       $this->newTitle->getUserPermissionsErrors( 'edit', $user )
+                       $this->permMgr->getPermissionErrors( 'move', $user, $this->oldTitle ),
+                       $this->permMgr->getPermissionErrors( 'edit', $user, $this->oldTitle ),
+                       $this->permMgr->getPermissionErrors( 'move-target', $user, $this->newTitle ),
+                       $this->permMgr->getPermissionErrors( 'edit', $user, $this->newTitle )
                );
 
                // Convert into a Status object
@@ -96,7 +153,6 @@ class MovePage {
         * @return Status
         */
        public function isValidMove() {
-               global $wgContentHandlerUseDB;
                $status = new Status();
 
                if ( $this->oldTitle->equals( $this->newTitle ) ) {
@@ -133,7 +189,7 @@ class MovePage {
                }
 
                // Content model checks
-               if ( !$wgContentHandlerUseDB &&
+               if ( !$this->options->get( 'ContentHandlerUseDB' ) &&
                        $this->oldTitle->getContentModel() !== $this->newTitle->getContentModel() ) {
                        // can't move a page if that would change the page's content model
                        $status->fatal(
@@ -430,8 +486,6 @@ class MovePage {
         * @return Status
         */
        private function moveUnsafe( User $user, $reason, $createRedirect, array $changeTags ) {
-               global $wgCategoryCollation;
-
                $status = Status::newGood();
                Hooks::run( 'TitleMove', [ $this->oldTitle, $this->newTitle, $user, $reason, &$status ] );
                if ( !$status->isOK() ) {
@@ -439,7 +493,7 @@ class MovePage {
                        return $status;
                }
 
-               $dbw = wfGetDB( DB_MASTER );
+               $dbw = $this->loadBalancer->getConnection( DB_MASTER );
                $dbw->startAtomic( __METHOD__, IDatabase::ATOMIC_CANCELABLE );
 
                Hooks::run( 'TitleMoveStarting', [ $this->oldTitle, $this->newTitle, $user ] );
@@ -461,9 +515,7 @@ class MovePage {
                        [ 'cl_from' => $pageid ],
                        __METHOD__
                );
-               $services = MediaWikiServices::getInstance();
-               $type = $services->getNamespaceInfo()->
-                       getCategoryLinkType( $this->newTitle->getNamespace() );
+               $type = $this->nsInfo->getCategoryLinkType( $this->newTitle->getNamespace() );
                foreach ( $prefixes as $prefixRow ) {
                        $prefix = $prefixRow->cl_sortkey_prefix;
                        $catTo = $prefixRow->cl_to;
@@ -471,7 +523,7 @@ class MovePage {
                                [
                                        'cl_sortkey' => Collation::singleton()->getSortKey(
                                                        $this->newTitle->getCategorySortkey( $prefix ) ),
-                                       'cl_collation' => $wgCategoryCollation,
+                                       'cl_collation' => $this->options->get( 'CategoryCollation' ),
                                        'cl_type' => $type,
                                        'cl_timestamp=cl_timestamp' ],
                                [
@@ -563,13 +615,10 @@ class MovePage {
                # Update watchlists
                $oldtitle = $this->oldTitle->getDBkey();
                $newtitle = $this->newTitle->getDBkey();
-               $oldsnamespace = $services->getNamespaceInfo()->
-                       getSubject( $this->oldTitle->getNamespace() );
-               $newsnamespace = $services->getNamespaceInfo()->
-                       getSubject( $this->newTitle->getNamespace() );
+               $oldsnamespace = $this->nsInfo->getSubject( $this->oldTitle->getNamespace() );
+               $newsnamespace = $this->nsInfo->getSubject( $this->newTitle->getNamespace() );
                if ( $oldsnamespace != $newsnamespace || $oldtitle != $newtitle ) {
-                       $services->getWatchedItemStore()->duplicateAllAssociatedEntries(
-                               $this->oldTitle, $this->newTitle );
+                       $this->watchedItems->duplicateAllAssociatedEntries( $this->oldTitle, $this->newTitle );
                }
 
                // If it is a file then move it last.
@@ -739,7 +788,7 @@ class MovePage {
                        $comment .= wfMessage( 'colon-separator' )->inContentLanguage()->text() . $reason;
                }
 
-               $dbw = wfGetDB( DB_MASTER );
+               $dbw = $this->loadBalancer->getConnection( DB_MASTER );
 
                $oldpage = WikiPage::factory( $this->oldTitle );
                $oldcountable = $oldpage->isCountable();
index 7221381..fb44722 100644 (file)
@@ -50,6 +50,7 @@ use MediaWiki\Linker\LinkRenderer;
 use MediaWiki\Linker\LinkRendererFactory;
 use MediaWiki\Logger\LoggerFactory;
 use MediaWiki\MediaWikiServices;
+use MediaWiki\Page\MovePageFactory;
 use MediaWiki\Permissions\PermissionManager;
 use MediaWiki\Preferences\PreferencesFactory;
 use MediaWiki\Preferences\DefaultPreferencesFactory;
@@ -391,6 +392,16 @@ return [
                return new MimeAnalyzer( $params );
        },
 
+       'MovePageFactory' => function ( MediaWikiServices $services ) : MovePageFactory {
+               return new MovePageFactory(
+                       new ServiceOptions( MovePageFactory::$constructorOptions, $services->getMainConfig() ),
+                       $services->getDBLoadBalancer(),
+                       $services->getNamespaceInfo(),
+                       $services->getWatchedItemStore(),
+                       $services->getPermissionManager()
+               );
+       },
+
        'NamespaceInfo' => function ( MediaWikiServices $services ) : NamespaceInfo {
                return new NamespaceInfo( new ServiceOptions( NamespaceInfo::$constructorOptions,
                        $services->getMainConfig() ) );
index 281f75b..f681852 100644 (file)
@@ -3461,7 +3461,7 @@ class Title implements LinkTarget, IDBAccessObject {
                        return [ [ 'badtitletext' ] ];
                }
 
-               $mp = new MovePage( $this, $nt );
+               $mp = MediaWikiServices::getInstance()->getMovePageFactory()->newMovePage( $this, $nt );
                $errors = $mp->isValidMove()->getErrorsArray();
                if ( $auth ) {
                        $errors = wfMergeErrorArrays(
@@ -3493,7 +3493,7 @@ class Title implements LinkTarget, IDBAccessObject {
 
                global $wgUser;
 
-               $mp = new MovePage( $this, $nt );
+               $mp = MediaWikiServices::getInstance()->getMovePageFactory()->newMovePage( $this, $nt );
                $method = $auth ? 'moveIfAllowed' : 'move';
                $status = $mp->$method( $wgUser, $reason, $createRedirect, $changeTags );
                if ( $status->isOK() ) {
index 540860b..0a788d4 100644 (file)
@@ -172,7 +172,7 @@ class ApiMove extends ApiBase {
         * @return Status
         */
        protected function movePage( Title $from, Title $to, $reason, $createRedirect, $changeTags ) {
-               $mp = new MovePage( $from, $to );
+               $mp = MediaWikiServices::getInstance()->getMovePageFactory()->newMovePage( $from, $to );
                $valid = $mp->isValidMove();
                if ( !$valid->isOK() ) {
                        return $valid;
diff --git a/includes/page/MovePageFactory.php b/includes/page/MovePageFactory.php
new file mode 100644 (file)
index 0000000..144e75b
--- /dev/null
@@ -0,0 +1,85 @@
+<?php
+
+/**
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ */
+
+namespace MediaWiki\Page;
+
+use MediaWiki\Config\ServiceOptions;
+use MediaWiki\Permissions\PermissionManager;
+use MovePage;
+use NamespaceInfo;
+use Title;
+use WatchedItemStore;
+use Wikimedia\Rdbms\LoadBalancer;
+
+/**
+ * @since 1.34
+ */
+class MovePageFactory {
+       /** @var ServiceOptions */
+       private $options;
+
+       /** @var LoadBalancer */
+       private $loadBalancer;
+
+       /** @var NamespaceInfo */
+       private $nsInfo;
+
+       /** @var WatchedItemStore */
+       private $watchedItems;
+
+       /** @var PermissionManager */
+       private $permMgr;
+
+       /**
+        * @todo Make this a const when we drop HHVM support (T192166)
+        * @var array
+        */
+       public static $constructorOptions = [
+               'CategoryCollation',
+               'ContentHandlerUseDB',
+       ];
+
+       public function __construct(
+               ServiceOptions $options,
+               LoadBalancer $loadBalancer,
+               NamespaceInfo $nsInfo,
+               WatchedItemStore $watchedItems,
+               PermissionManager $permMgr
+       ) {
+               $options->assertRequiredOptions( self::$constructorOptions );
+
+               $this->options = $options;
+               $this->loadBalancer = $loadBalancer;
+               $this->nsInfo = $nsInfo;
+               $this->watchedItems = $watchedItems;
+               $this->permMgr = $permMgr;
+       }
+
+       /**
+        * @param Title $from
+        * @param Title $to
+        * @return MovePage
+        */
+       public function newMovePage( Title $from, Title $to ) : MovePage {
+               return new MovePage( $from, $to, $this->options, $this->loadBalancer, $this->nsInfo,
+                       $this->watchedItems, $this->permMgr );
+       }
+}
index d14db03..b643c3f 100644 (file)
@@ -351,7 +351,6 @@ class Parser {
                $nsInfo = null,
                $logger = null
        ) {
-               $services = MediaWikiServices::getInstance();
                if ( !$svcOptions || is_array( $svcOptions ) ) {
                        // Pre-1.34 calling convention is the first parameter is just ParserConf, the seventh is
                        // Config, and the eighth is LinkRendererFactory.
@@ -363,8 +362,8 @@ class Parser {
                                $this->mConf['preprocessorClass'] = self::getDefaultPreprocessorClass();
                        }
                        $this->svcOptions = new ServiceOptions( self::$constructorOptions,
-                               $this->mConf,
-                               func_num_args() > 6 ? func_get_arg( 6 ) : $services->getMainConfig()
+                               $this->mConf, func_num_args() > 6
+                                       ? func_get_arg( 6 ) : MediaWikiServices::getInstance()->getMainConfig()
                        );
                        $linkRendererFactory = func_num_args() > 7 ? func_get_arg( 7 ) : null;
                        $nsInfo = func_num_args() > 8 ? func_get_arg( 8 ) : null;
@@ -386,14 +385,16 @@ class Parser {
                        self::EXT_LINK_URL_CLASS . '*)\p{Zs}*([^\]\\x00-\\x08\\x0a-\\x1F\\x{FFFD}]*?)\]/Su';
 
                $this->magicWordFactory = $magicWordFactory ??
-                       $services->getMagicWordFactory();
+                       MediaWikiServices::getInstance()->getMagicWordFactory();
 
-               $this->contLang = $contLang ?? $services->getContentLanguage();
+               $this->contLang = $contLang ?? MediaWikiServices::getInstance()->getContentLanguage();
 
-               $this->factory = $factory ?? $services->getParserFactory();
-               $this->specialPageFactory = $spFactory ?? $services->getSpecialPageFactory();
-               $this->linkRendererFactory = $linkRendererFactory ?? $services->getLinkRendererFactory();
-               $this->nsInfo = $nsInfo ?? $services->getNamespaceInfo();
+               $this->factory = $factory ?? MediaWikiServices::getInstance()->getParserFactory();
+               $this->specialPageFactory = $spFactory ??
+                       MediaWikiServices::getInstance()->getSpecialPageFactory();
+               $this->linkRendererFactory = $linkRendererFactory ??
+                       MediaWikiServices::getInstance()->getLinkRendererFactory();
+               $this->nsInfo = $nsInfo ?? MediaWikiServices::getInstance()->getNamespaceInfo();
                $this->logger = $logger ?: new NullLogger();
        }
 
index 20be9fd..da241e5 100644 (file)
@@ -160,7 +160,8 @@ class CleanupCaps extends TableCleanup {
                        $this->output( "\"$display\" -> \"$targetDisplay\": DRY RUN, NOT MOVED\n" );
                        $ok = 'OK';
                } else {
-                       $mp = new MovePage( $current, $target );
+                       $mp = MediaWikiServices::getInstance()->getMovePageFactory()
+                               ->newMovePage( $current, $target );
                        $status = $mp->move( $this->user, $reason, $createRedirect );
                        $ok = $status->isOK() ? 'OK' : $status->getWikiText( false, false, 'en' );
                        $this->output( "\"$display\" -> \"$targetDisplay\": $ok\n" );
index 47828e6..09f3120 100644 (file)
@@ -35,6 +35,8 @@
  * e.g. immobile_namespace for namespaces which can't be moved
  */
 
+use MediaWiki\MediaWikiServices;
+
 require_once __DIR__ . '/Maintenance.php';
 
 /**
@@ -105,7 +107,8 @@ class MoveBatch extends Maintenance {
 
                        $this->output( $source->getPrefixedText() . ' --> ' . $dest->getPrefixedText() );
                        $this->beginTransaction( $dbw, __METHOD__ );
-                       $mp = new MovePage( $source, $dest );
+                       $mp = MediaWikiServices::getInstance()->getMovePageFactory()
+                               ->newMovePage( $source, $dest );
                        $status = $mp->move( $wgUser, $reason, !$noredirects );
                        if ( !$status->isOK() ) {
                                $this->output( "\nFAILED: " . $status->getWikiText( false, false, 'en' ) );
index c35e80f..65ba495 100644 (file)
@@ -74,6 +74,7 @@ $wgAutoloadClasses += [
        'TestUserRegistry' => "$testDir/phpunit/includes/TestUserRegistry.php",
 
        # tests/phpunit/includes
+       'FactoryArgTestTrait' => "$testDir/phpunit/unit/includes/FactoryArgTestTrait.php",
        'PageArchiveTestBase' => "$testDir/phpunit/includes/page/PageArchiveTestBase.php",
        'RevisionDbTestBase' => "$testDir/phpunit/includes/RevisionDbTestBase.php",
        'RevisionTestModifyableContent' => "$testDir/phpunit/includes/RevisionTestModifyableContent.php",
index 31a0e79..25cb807 100644 (file)
@@ -1,9 +1,55 @@
 <?php
 
+use MediaWiki\Config\ServiceOptions;
+use MediaWiki\Page\MovePageFactory;
+use Wikimedia\Rdbms\IDatabase;
+use Wikimedia\Rdbms\LoadBalancer;
+
 /**
  * @group Database
  */
 class MovePageTest extends MediaWikiTestCase {
+       /**
+        * @param string $class
+        * @return object A mock that throws on any method call
+        */
+       private function getNoOpMock( $class ) {
+               $mock = $this->createMock( $class );
+               $mock->expects( $this->never() )->method( $this->anythingBut( '__destruct' ) );
+               return $mock;
+       }
+
+       /**
+        * @param LinkTarget $old
+        * @param LinkTarget $new
+        * @param array $params Valid keys are: db, options, nsInfo, wiStore. options is an indexed
+        *   array that will overwrite our defaults, not a ServiceOptions, so it need not contain all
+        *   keys.
+        * @return MovePage
+        */
+       private function newMovePage( $old, $new, array $params = [] ) : MovePage {
+               $mockLB = $this->createMock( LoadBalancer::class );
+               $mockLB->method( 'getConnection' )
+                       ->willReturn( $params['db'] ?? $this->getNoOpMock( IDatabase::class ) );
+               $mockLB->expects( $this->never() )
+                       ->method( $this->anythingBut( 'getConnection', '__destruct' ) );
+
+               return new MovePage(
+                       $old,
+                       $new,
+                       new ServiceOptions(
+                               MovePageFactory::$constructorOptions,
+                               $params['options'] ?? [],
+                               [
+                                       'CategoryCollation' => 'uppercase',
+                                       'ContentHandlerUseDB' => true,
+                               ]
+                       ),
+                       $mockLB,
+                       $params['nsInfo'] ?? $this->getNoOpMock( NamespaceInfo::class ),
+                       $params['wiStore'] ?? $this->getNoOpMock( WatchedItemStore::class )
+               );
+       }
 
        public function setUp() {
                parent::setUp();
@@ -23,9 +69,10 @@ class MovePageTest extends MediaWikiTestCase {
                        // We can only set this to false with the old schema
                        $this->setMwGlobals( 'wgContentHandlerUseDB', false );
                }
-               $mp = new MovePage(
+               $mp = $this->newMovePage(
                        Title::newFromText( $old ),
-                       Title::newFromText( $new )
+                       Title::newFromText( $new ),
+                       [ 'options' => [ 'ContentHandlerUseDB' => false ] ]
                );
                $status = $mp->isValidMove();
                if ( $error === true ) {
@@ -58,6 +105,32 @@ class MovePageTest extends MediaWikiTestCase {
                return $ret;
        }
 
+       /**
+        * Integration test to catch regressions like T74870. Taken and modified
+        * from SemanticMediaWiki
+        *
+        * @covers Title::moveTo
+        * @covers MovePage::move
+        */
+       public function testTitleMoveCompleteIntegrationTest() {
+               $this->hideDeprecated( 'Title::moveTo' );
+
+               $oldTitle = Title::newFromText( 'Help:Some title' );
+               WikiPage::factory( $oldTitle )->doEditContent( new WikitextContent( 'foo' ), 'bar' );
+               $newTitle = Title::newFromText( 'Help:Some other title' );
+               $this->assertNull(
+                       WikiPage::factory( $newTitle )->getRevision()
+               );
+
+               $this->assertTrue( $oldTitle->moveTo( $newTitle, false, 'test1', true ) );
+               $this->assertNotNull(
+                       WikiPage::factory( $oldTitle )->getRevision()
+               );
+               $this->assertNotNull(
+                       WikiPage::factory( $newTitle )->getRevision()
+               );
+       }
+
        /**
         * Test for the move operation being aborted via the TitleMove hook
         * @covers MovePage::move
@@ -73,7 +146,7 @@ class MovePageTest extends MediaWikiTestCase {
                $oldTitle = Title::newFromText( 'Some old title' );
                WikiPage::factory( $oldTitle )->doEditContent( new WikitextContent( 'foo' ), 'bar' );
                $newTitle = Title::newFromText( 'A brand new title' );
-               $mp = new MovePage( $oldTitle, $newTitle );
+               $mp = $this->newMovePage( $oldTitle, $newTitle );
                $user = User::newFromName( 'TitleMove tester' );
                $status = $mp->move( $user, 'Reason', true );
                $this->assertTrue( $status->hasMessage( $error ) );
diff --git a/tests/phpunit/includes/parser/ParserFactoryIntegrationTest.php b/tests/phpunit/includes/parser/ParserFactoryIntegrationTest.php
new file mode 100644 (file)
index 0000000..5bf4f3f
--- /dev/null
@@ -0,0 +1,56 @@
+<?php
+
+/**
+ * @covers ParserFactory
+ */
+class ParserFactoryIntegrationTest extends MediaWikiIntegrationTestCase {
+       public function provideConstructorArguments() {
+               // Create a mock Config object that will satisfy ServiceOptions::__construct
+               $mockConfig = $this->createMock( 'Config' );
+               $mockConfig->method( 'has' )->willReturn( true );
+               $mockConfig->method( 'get' )->willReturn( 'I like otters.' );
+
+               $mocks = [
+                       [ 'the plural of platypus...' ],
+                       $this->createMock( 'MagicWordFactory' ),
+                       $this->createMock( 'Language' ),
+                       '...is platypodes',
+                       $this->createMock( 'MediaWiki\Special\SpecialPageFactory' ),
+                       $mockConfig,
+                       $this->createMock( 'MediaWiki\Linker\LinkRendererFactory' ),
+               ];
+
+               yield 'args_without_namespace_info' => [
+                       $mocks,
+               ];
+               yield 'args_with_namespace_info' => [
+                       array_merge( $mocks, [ $this->createMock( 'NamespaceInfo' ) ] ),
+               ];
+       }
+
+       /**
+        * @dataProvider provideConstructorArguments
+        * @covers ParserFactory::__construct
+        */
+       public function testBackwardsCompatibleConstructorArguments( $args ) {
+               $this->hideDeprecated( 'ParserFactory::__construct with Config parameter' );
+               $factory = new ParserFactory( ...$args );
+               $parser = $factory->create();
+
+               // It is expected that these are not present on the parser.
+               unset( $args[5] );
+               unset( $args[0] );
+
+               foreach ( ( new ReflectionObject( $parser ) )->getProperties() as $prop ) {
+                       $prop->setAccessible( true );
+                       foreach ( $args as $idx => $mockTest ) {
+                               if ( $prop->getValue( $parser ) === $mockTest ) {
+                                       unset( $args[$idx] );
+                               }
+                       }
+               }
+
+               $this->assertCount( 0, $args, 'Not all arguments to the ParserFactory constructor were ' .
+                       'found in Parser member variables' );
+       }
+}
diff --git a/tests/phpunit/includes/parser/ParserFactoryTest.php b/tests/phpunit/includes/parser/ParserFactoryTest.php
deleted file mode 100644 (file)
index 048256d..0000000
+++ /dev/null
@@ -1,107 +0,0 @@
-<?php
-
-/**
- * @covers ParserFactory
- */
-class ParserFactoryTest extends MediaWikiTestCase {
-       /**
-        * For backwards compatibility, all parameters to the parser constructor are optional and
-        * default to the appropriate global service, so it's easy to forget to update ParserFactory to
-        * actually pass the parameters it's supposed to.
-        */
-       public function testConstructorArgNum() {
-               $factoryConstructor = new ReflectionMethod( 'ParserFactory', '__construct' );
-               $instanceConstructor = new ReflectionMethod( 'Parser', '__construct' );
-               // Subtract one for the ParserFactory itself
-               $this->assertSame( $instanceConstructor->getNumberOfParameters() - 1,
-                       $factoryConstructor->getNumberOfParameters(),
-                       'Parser and ParserFactory constructors have an inconsistent number of parameters. ' .
-                       'Did you add a parameter to one and not the other?' );
-       }
-
-       public function testAllArgumentsWerePassed() {
-               $factoryConstructor = new ReflectionMethod( 'ParserFactory', '__construct' );
-               $mocks = [];
-               foreach ( $factoryConstructor->getParameters() as $index => $param ) {
-                       $type = (string)$param->getType();
-                       if ( $index === 0 ) {
-                               $val = $this->createMock( 'MediaWiki\Config\ServiceOptions' );
-                       } elseif ( $type === 'array' ) {
-                               $val = [ 'porcupines will tell me your secrets' . count( $mocks ) ];
-                       } elseif ( class_exists( $type ) || interface_exists( $type ) ) {
-                               $val = $this->createMock( $type );
-                       } elseif ( $type === '' ) {
-                               // Optimistically assume a string is okay
-                               $val = 'I will de-quill them first' . count( $mocks );
-                       } else {
-                               $this->fail( "Unrecognized parameter type $type in ParserFactory constructor" );
-                       }
-                       $mocks[] = $val;
-               }
-
-               $factory = new ParserFactory( ...$mocks );
-               $parser = $factory->create();
-
-               foreach ( ( new ReflectionObject( $parser ) )->getProperties() as $prop ) {
-                       $prop->setAccessible( true );
-                       foreach ( $mocks as $idx => $mock ) {
-                               if ( $prop->getValue( $parser ) === $mock ) {
-                                       unset( $mocks[$idx] );
-                               }
-                       }
-               }
-
-               $this->assertCount( 0, $mocks, 'Not all arguments to the ParserFactory constructor were ' .
-                       'found in Parser member variables' );
-       }
-
-       public function provideConstructorArguments() {
-               // Create a mock Config object that will satisfy ServiceOptions::__construct
-               $mockConfig = $this->createMock( 'Config' );
-               $mockConfig->method( 'has' )->willReturn( true );
-               $mockConfig->method( 'get' )->willReturn( 'I like otters.' );
-
-               $mocks = [
-                       [ 'the plural of platypus...' ],
-                       $this->createMock( 'MagicWordFactory' ),
-                       $this->createMock( 'Language' ),
-                       '...is platypodes',
-                       $this->createMock( 'MediaWiki\Special\SpecialPageFactory' ),
-                       $mockConfig,
-                       $this->createMock( 'MediaWiki\Linker\LinkRendererFactory' ),
-               ];
-
-               yield 'args_without_namespace_info' => [
-                       $mocks,
-               ];
-               yield 'args_with_namespace_info' => [
-                       array_merge( $mocks, [ $this->createMock( 'NamespaceInfo' ) ] ),
-               ];
-       }
-
-       /**
-        * @dataProvider provideConstructorArguments
-        * @covers ParserFactory::__construct
-        */
-       public function testBackwardsCompatibleConstructorArguments( $args ) {
-               $this->hideDeprecated( 'ParserFactory::__construct with Config parameter' );
-               $factory = new ParserFactory( ...$args );
-               $parser = $factory->create();
-
-               // It is expected that these are not present on the parser.
-               unset( $args[5] );
-               unset( $args[0] );
-
-               foreach ( ( new ReflectionObject( $parser ) )->getProperties() as $prop ) {
-                       $prop->setAccessible( true );
-                       foreach ( $args as $idx => $mockTest ) {
-                               if ( $prop->getValue( $parser ) === $mockTest ) {
-                                       unset( $args[$idx] );
-                               }
-                       }
-               }
-
-               $this->assertCount( 0, $args, 'Not all arguments to the ParserFactory constructor were ' .
-                       'found in Parser member variables' );
-       }
-}
diff --git a/tests/phpunit/unit/includes/FactoryArgTestTrait.php b/tests/phpunit/unit/includes/FactoryArgTestTrait.php
new file mode 100644 (file)
index 0000000..f7035b4
--- /dev/null
@@ -0,0 +1,148 @@
+<?php
+
+/**
+ * Test that a factory class correctly forwards all arguments to the class it constructs. This is
+ * useful because sometimes a class' constructor will have more arguments added, and it's easy to
+ * accidentally have the factory's constructor fall out of sync.
+ */
+trait FactoryArgTestTrait {
+       /**
+        * @return string Name of factory class
+        */
+       abstract protected static function getFactoryClass();
+
+       /**
+        * @return string Name of instance class
+        */
+       abstract protected static function getInstanceClass();
+
+       /**
+        * @return int The number of arguments that the instance constructor receives but the factory
+        * constructor doesn't. Used for a simple argument count check. Override if this isn't zero.
+        */
+       protected static function getExtraClassArgCount() {
+               return 0;
+       }
+
+       /**
+        * Override if your factory method name is different from newInstanceClassName.
+        *
+        * @return string
+        */
+       protected function getFactoryMethodName() {
+               return 'new' . $this->getInstanceClass();
+       }
+
+       /**
+        * Override if $factory->$method( ...$args ) isn't the right way to create an instance, where
+        * $method is returned from getFactoryMethodName(), and $args is constructed by applying
+        * getMockValueForParam() to the factory method's parameters.
+        *
+        * @param object $factory Factory object
+        * @return object Object created by factory
+        */
+       protected function createInstanceFromFactory( $factory ) {
+               $methodName = $this->getFactoryMethodName();
+               $methodObj = new ReflectionMethod( $factory, $methodName );
+               $mocks = [];
+               foreach ( $methodObj->getParameters() as $param ) {
+                       $mocks[] = $this->getMockValueForParam( $param );
+               }
+
+               return $factory->$methodName( ...$mocks );
+       }
+
+       public function testConstructorArgNum() {
+               $factoryClass = static::getFactoryClass();
+               $instanceClass = static::getInstanceClass();
+               $factoryConstructor = new ReflectionMethod( $factoryClass, '__construct' );
+               $instanceConstructor = new ReflectionMethod( $instanceClass, '__construct' );
+               $this->assertSame(
+                       $instanceConstructor->getNumberOfParameters() - static::getExtraClassArgCount(),
+                       $factoryConstructor->getNumberOfParameters(),
+                       "$instanceClass and $factoryClass constructors have an inconsistent number of " .
+                       ' parameters. Did you add a parameter to one and not the other?' );
+       }
+
+       /**
+        * Override if getMockValueForParam doesn't produce suitable values for one or more of the
+        * parameters to your factory constructor or create method.
+        *
+        * @param ReflectionParameter $param One of the factory constructor's arguments
+        * @return array Empty to not override, or an array of one element which is the value to pass
+        *   that will allow the object to be constructed successfully
+        */
+       protected function getOverriddenMockValueForParam( ReflectionParameter $param ) {
+               return [];
+       }
+
+       /**
+        * Override if this doesn't produce suitable values for one or more of the parameters to your
+        * factory constructor or create method.
+        *
+        * @param ReflectionParameter $param One of the factory constructor's arguments
+        * @return mixed A value to pass that will allow the object to be constructed successfully
+        */
+       protected function getMockValueForParam( ReflectionParameter $param ) {
+               $overridden = $this->getOverriddenMockValueForParam( $param );
+               if ( $overridden ) {
+                       return $overridden[0];
+               }
+
+               $pos = $param->getPosition();
+
+               $type = (string)$param->getType();
+
+               if ( $type === 'array' ) {
+                       return [ "some unlikely string $pos" ];
+               }
+
+               if ( class_exists( $type ) || interface_exists( $type ) ) {
+                       return $this->createMock( $type );
+               }
+
+               if ( $type === '' ) {
+                       // Optimistically assume a string is okay
+                       return "some unlikely string $pos";
+               }
+
+               $this->fail( "Unrecognized parameter type $type" );
+       }
+
+       /**
+        * Assert that the given $instance correctly received $val as the value for parameter $name. By
+        * default, checks that the instance has some member whose value is the same as $val.
+        *
+        * @param object $instance
+        * @param string $name Name of parameter to the factory object's constructor
+        * @param mixed $val
+        */
+       protected function assertInstanceReceivedParam( $instance, $name, $val ) {
+               foreach ( ( new ReflectionObject( $instance ) )->getProperties() as $prop ) {
+                       $prop->setAccessible( true );
+                       if ( $prop->getValue( $instance ) === $val ) {
+                               $this->assertTrue( true );
+                               return;
+                       }
+               }
+
+               $this->assertFalse( true, "Param $name not received by " . static::getInstanceClass() );
+       }
+
+       public function testAllArgumentsWerePassed() {
+               $factoryClass = static::getFactoryClass();
+
+               $factoryConstructor = new ReflectionMethod( $factoryClass, '__construct' );
+               $mocks = [];
+               foreach ( $factoryConstructor->getParameters() as $param ) {
+                       $mocks[$param->getName()] = $this->getMockValueForParam( $param );
+               }
+
+               $instance =
+                       $this->createInstanceFromFactory( new $factoryClass( ...array_values( $mocks ) ) );
+
+               foreach ( $mocks as $name => $mock ) {
+                       $this->assertInstanceReceivedParam( $instance, $name, $mock );
+               }
+       }
+}
diff --git a/tests/phpunit/unit/includes/page/MovePageFactoryTest.php b/tests/phpunit/unit/includes/page/MovePageFactoryTest.php
new file mode 100644 (file)
index 0000000..99fc631
--- /dev/null
@@ -0,0 +1,23 @@
+<?php
+
+use MediaWiki\Page\MovePageFactory;
+
+/**
+ * @covers MediaWiki\Page\MovePageFactory
+ */
+class MovePageFactoryTest extends MediaWikiUnitTestCase {
+       use FactoryArgTestTrait;
+
+       protected function getFactoryClass() {
+               return MovePageFactory::class;
+       }
+
+       protected function getInstanceClass() {
+               return MovePage::class;
+       }
+
+       protected static function getExtraClassArgCount() {
+               // $to and $from
+               return 2;
+       }
+}
diff --git a/tests/phpunit/unit/includes/parser/ParserFactoryTest.php b/tests/phpunit/unit/includes/parser/ParserFactoryTest.php
new file mode 100644 (file)
index 0000000..f1e48c7
--- /dev/null
@@ -0,0 +1,32 @@
+<?php
+
+/**
+ * @covers ParserFactory
+ */
+class ParserFactoryTest extends MediaWikiUnitTestCase {
+       use FactoryArgTestTrait;
+
+       protected static function getFactoryClass() {
+               return ParserFactory::class;
+       }
+
+       protected static function getInstanceClass() {
+               return Parser::class;
+       }
+
+       protected static function getFactoryMethodName() {
+               return 'create';
+       }
+
+       protected static function getExtraClassArgCount() {
+               // The parser factory itself is passed to the parser
+               return 1;
+       }
+
+       protected function getOverriddenMockValueForParam( ReflectionParameter $param ) {
+               if ( $param->getPosition() === 0 ) {
+                       return [ $this->createMock( MediaWiki\Config\ServiceOptions::class ) ];
+               }
+               return [];
+       }
+}