From 8ec24b607a23f8a51a3b770a562ec8381de6e685 Mon Sep 17 00:00:00 2001 From: Aryeh Gregor Date: Wed, 1 May 2019 14:39:45 +0300 Subject: [PATCH] Introduce MovePageFactory 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 --- RELEASE-NOTES-1.34 | 1 + autoload.php | 1 + includes/MediaWikiServices.php | 11 +- includes/MovePage.php | 91 ++++++++--- includes/ServiceWiring.php | 11 ++ includes/Title.php | 4 +- includes/api/ApiMove.php | 2 +- includes/page/MovePageFactory.php | 85 ++++++++++ includes/parser/Parser.php | 19 +-- maintenance/cleanupCaps.php | 3 +- maintenance/moveBatch.php | 5 +- tests/common/TestsAutoLoader.php | 1 + tests/phpunit/includes/MovePageTest.php | 79 +++++++++- .../parser/ParserFactoryIntegrationTest.php | 56 +++++++ .../includes/parser/ParserFactoryTest.php | 107 ------------- .../unit/includes/FactoryArgTestTrait.php | 148 ++++++++++++++++++ .../includes/page/MovePageFactoryTest.php | 23 +++ .../includes/parser/ParserFactoryTest.php | 32 ++++ 18 files changed, 533 insertions(+), 146 deletions(-) create mode 100644 includes/page/MovePageFactory.php create mode 100644 tests/phpunit/includes/parser/ParserFactoryIntegrationTest.php delete mode 100644 tests/phpunit/includes/parser/ParserFactoryTest.php create mode 100644 tests/phpunit/unit/includes/FactoryArgTestTrait.php create mode 100644 tests/phpunit/unit/includes/page/MovePageFactoryTest.php create mode 100644 tests/phpunit/unit/includes/parser/ParserFactoryTest.php diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index fd7e4d9f57..9a4aeb38f9 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -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 === * … diff --git a/autoload.php b/autoload.php index 52a5edaef1..acdf8dd3d2 100644 --- a/autoload.php +++ b/autoload.php @@ -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', diff --git a/includes/MediaWikiServices.php b/includes/MediaWikiServices.php index e5efd7d99c..bb9c05f424 100644 --- a/includes/MediaWikiServices.php +++ b/includes/MediaWikiServices.php @@ -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 diff --git a/includes/MovePage.php b/includes/MovePage.php index 832e24af81..f48d92e0a2 100644 --- a/includes/MovePage.php +++ b/includes/MovePage.php @@ -19,9 +19,13 @@ * @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(); diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 7221381e90..fb44722c11 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -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() ) ); diff --git a/includes/Title.php b/includes/Title.php index 281f75bac1..f6818525d8 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -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() ) { diff --git a/includes/api/ApiMove.php b/includes/api/ApiMove.php index 540860b3a9..0a788d4e26 100644 --- a/includes/api/ApiMove.php +++ b/includes/api/ApiMove.php @@ -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 index 0000000000..144e75b560 --- /dev/null +++ b/includes/page/MovePageFactory.php @@ -0,0 +1,85 @@ +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 ); + } +} diff --git a/includes/parser/Parser.php b/includes/parser/Parser.php index d14db039a1..b643c3f09c 100644 --- a/includes/parser/Parser.php +++ b/includes/parser/Parser.php @@ -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(); } diff --git a/maintenance/cleanupCaps.php b/maintenance/cleanupCaps.php index 20be9fd1d7..da241e5337 100644 --- a/maintenance/cleanupCaps.php +++ b/maintenance/cleanupCaps.php @@ -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" ); diff --git a/maintenance/moveBatch.php b/maintenance/moveBatch.php index 47828e690f..09f3120a97 100644 --- a/maintenance/moveBatch.php +++ b/maintenance/moveBatch.php @@ -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' ) ); diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index c35e80fada..65ba4957af 100644 --- a/tests/common/TestsAutoLoader.php +++ b/tests/common/TestsAutoLoader.php @@ -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", diff --git a/tests/phpunit/includes/MovePageTest.php b/tests/phpunit/includes/MovePageTest.php index 31a0e79aa7..25cb8073a0 100644 --- a/tests/phpunit/includes/MovePageTest.php +++ b/tests/phpunit/includes/MovePageTest.php @@ -1,9 +1,55 @@ 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 index 0000000000..5bf4f3f5fb --- /dev/null +++ b/tests/phpunit/includes/parser/ParserFactoryIntegrationTest.php @@ -0,0 +1,56 @@ +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 index 048256d255..0000000000 --- a/tests/phpunit/includes/parser/ParserFactoryTest.php +++ /dev/null @@ -1,107 +0,0 @@ -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 index 0000000000..f7035b4d23 --- /dev/null +++ b/tests/phpunit/unit/includes/FactoryArgTestTrait.php @@ -0,0 +1,148 @@ +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 index 0000000000..99fc631c78 --- /dev/null +++ b/tests/phpunit/unit/includes/page/MovePageFactoryTest.php @@ -0,0 +1,23 @@ +getPosition() === 0 ) { + return [ $this->createMock( MediaWiki\Config\ServiceOptions::class ) ]; + } + return []; + } +} -- 2.20.1