From 5e2199c5b0c383337c78e0bc0160397b0f05b3b5 Mon Sep 17 00:00:00 2001 From: Aryeh Gregor Date: Sun, 18 Aug 2019 21:19:05 +0300 Subject: [PATCH] BadFileLookup to replace wfIsBadImage I think this probably shouldn't be directly in the MediaWiki namespace, but I don't know where is a better place to put it. In order to avoid gratuitous use of TitleFormatter, I changed the cache format -- the old implementation used getPrefixedDBkey() and I switched to an ns/dbkey pair. I also changed the cache keys to use SHA1 instead of MD5, by Daniel's request. The previous implementation cached the parsed blacklist for one minute without invalidation, so it could return slightly stale results, but it didn't retrieve the bad image list message on a cache hit. The new implementation unconditionally retrieves the bad image list message, but uses a hash of it in the cache key and caches for one day. The new behavior happens to be more cleanly implementable in a service. Bug: T200882 Bug: T139216 Change-Id: I69fed1b1f3cfc1aa149e0739780e67f6de01609d --- RELEASE-NOTES-1.34 | 1 + autoload.php | 1 + includes/BadFileLookup.php | 127 ++++++++++++ includes/GlobalFunctions.php | 78 ++------ includes/MediaWikiServices.php | 8 + includes/ServiceWiring.php | 12 ++ includes/api/ApiQueryImageInfo.php | 3 +- includes/gallery/TraditionalImageGallery.php | 4 +- includes/parser/Parser.php | 12 +- includes/parser/ParserFactory.php | 13 +- tests/common/TestsAutoLoader.php | 4 + .../phpunit/MediaWikiIntegrationTestCase.php | 17 +- tests/phpunit/MediaWikiTestCaseTrait.php | 20 ++ tests/phpunit/MediaWikiUnitTestCase.php | 20 +- .../GlobalFunctions/GlobalWithDBTest.php | 65 +----- .../unit/includes/BadFileLookupTest.php | 185 ++++++++++++++++++ 16 files changed, 427 insertions(+), 143 deletions(-) create mode 100644 includes/BadFileLookup.php create mode 100644 tests/phpunit/MediaWikiTestCaseTrait.php create mode 100644 tests/phpunit/unit/includes/BadFileLookupTest.php diff --git a/RELEASE-NOTES-1.34 b/RELEASE-NOTES-1.34 index a05f8d18e1..4e9a2e78dd 100644 --- a/RELEASE-NOTES-1.34 +++ b/RELEASE-NOTES-1.34 @@ -454,6 +454,7 @@ because of Phabricator reports. MediaWikiServices::getMessageCache(). * Constructing MovePage directly is deprecated. Use MovePageFactory. * TempFSFile::factory() has been deprecated. Use TempFSFileFactory instead. +* wfIsBadImage() is deprecated. Use the BadFileLookup service instead. === Other changes in 1.34 === * … diff --git a/autoload.php b/autoload.php index 20c19e50a9..c593b0c508 100644 --- a/autoload.php +++ b/autoload.php @@ -874,6 +874,7 @@ $wgAutoloadLocalClasses = [ 'MediaWikiSite' => __DIR__ . '/includes/site/MediaWikiSite.php', 'MediaWikiTitleCodec' => __DIR__ . '/includes/title/MediaWikiTitleCodec.php', 'MediaWikiVersionFetcher' => __DIR__ . '/includes/MediaWikiVersionFetcher.php', + 'MediaWiki\\BadFileLookup' => __DIR__ . '/includes/BadFileLookup.php', 'MediaWiki\\ChangeTags\\Taggable' => __DIR__ . '/includes/changetags/Taggable.php', 'MediaWiki\\Config\\ConfigRepository' => __DIR__ . '/includes/config/ConfigRepository.php', 'MediaWiki\\Config\\ServiceOptions' => __DIR__ . '/includes/config/ServiceOptions.php', diff --git a/includes/BadFileLookup.php b/includes/BadFileLookup.php new file mode 100644 index 0000000000..2f7c0eade3 --- /dev/null +++ b/includes/BadFileLookup.php @@ -0,0 +1,127 @@ +blacklistCallback = $blacklistCallback; + $this->cache = $cache; + $this->repoGroup = $repoGroup; + $this->titleParser = $titleParser; + } + + /** + * Determine if a file exists on the 'bad image list'. + * + * The format of MediaWiki:Bad_image_list is as follows: + * * Only list items (lines starting with "*") are considered + * * The first link on a line must be a link to a bad file + * * Any subsequent links on the same line are considered to be exceptions, + * i.e. articles where the file may occur inline. + * + * @param string $name The file name to check + * @param LinkTarget|null $contextTitle The page on which the file occurs, if known + * @return bool + */ + public function isBadFile( $name, LinkTarget $contextTitle = null ) { + // Handle redirects; callers almost always hit wfFindFile() anyway, so just use that method + // because it has a fast process cache. + $file = $this->repoGroup->findFile( $name ); + // XXX If we don't find the file we also don't replace spaces by underscores or otherwise + // validate or normalize the title, is this right? + if ( $file ) { + $name = $file->getTitle()->getDBkey(); + } + + // Run the extension hook + $bad = false; + if ( !Hooks::run( 'BadImage', [ $name, &$bad ] ) ) { + return (bool)$bad; + } + + if ( $this->badFiles === null ) { + // Not used before in this request, try the cache + $blacklist = ( $this->blacklistCallback )(); + $key = $this->cache->makeKey( 'bad-image-list', sha1( $blacklist ) ); + $this->badFiles = $this->cache->get( $key ) ?: null; + } + + if ( $this->badFiles === null ) { + // Cache miss, build the list now + $this->badFiles = []; + $lines = explode( "\n", $blacklist ); + foreach ( $lines as $line ) { + // List items only + if ( substr( $line, 0, 1 ) !== '*' ) { + continue; + } + + // Find all links + $m = []; + // XXX What is the ':?' doing in the regex? Why not let the TitleParser strip it? + if ( !preg_match_all( '/\[\[:?(.*?)\]\]/', $line, $m ) ) { + continue; + } + + $fileDBkey = null; + $exceptions = []; + foreach ( $m[1] as $i => $titleText ) { + try { + $title = $this->titleParser->parseTitle( $titleText ); + } catch ( MalformedTitleException $e ) { + continue; + } + if ( $i == 0 ) { + $fileDBkey = $title->getDBkey(); + } else { + $exceptions[$title->getNamespace()][$title->getDBkey()] = true; + } + } + + if ( $fileDBkey !== null ) { + $this->badFiles[$fileDBkey] = $exceptions; + } + } + $this->cache->set( $key, $this->badFiles, 24 * 60 * 60 ); + } + + return isset( $this->badFiles[$name] ) && ( !$contextTitle || + !isset( $this->badFiles[$name][$contextTitle->getNamespace()] + [$contextTitle->getDBkey()] ) ); + } +} diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index 1741958681..cc998c7223 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -24,14 +24,15 @@ if ( !defined( 'MEDIAWIKI' ) ) { die( "This file is part of MediaWiki, it is not a valid entry point" ); } +use MediaWiki\BadFileLookup; use MediaWiki\Linker\LinkTarget; use MediaWiki\Logger\LoggerFactory; use MediaWiki\MediaWikiServices; use MediaWiki\ProcOpenError; use MediaWiki\Session\SessionManager; use MediaWiki\Shell\Shell; -use Wikimedia\WrappedString; use Wikimedia\AtEase\AtEase; +use Wikimedia\WrappedString; /** * Load an extension @@ -2907,72 +2908,27 @@ function wfUnpack( $format, $data, $length = false ) { * * Any subsequent links on the same line are considered to be exceptions, * i.e. articles where the image may occur inline. * + * @deprecated since 1.34, use the BadFileLookup service directly instead + * * @param string $name The image name to check * @param Title|bool $contextTitle The page on which the image occurs, if known * @param string|null $blacklist Wikitext of a file blacklist * @return bool */ function wfIsBadImage( $name, $contextTitle = false, $blacklist = null ) { - # Handle redirects; callers almost always hit wfFindFile() anyway, - # so just use that method because it has a fast process cache. - $file = MediaWikiServices::getInstance()->getRepoGroup()->findFile( $name ); // get the final name - $name = $file ? $file->getTitle()->getDBkey() : $name; - - # Run the extension hook - $bad = false; - if ( !Hooks::run( 'BadImage', [ $name, &$bad ] ) ) { - return (bool)$bad; - } - - $cache = ObjectCache::getLocalServerInstance( 'hash' ); - $key = $cache->makeKey( - 'bad-image-list', ( $blacklist === null ) ? 'default' : md5( $blacklist ) - ); - $badImages = $cache->get( $key ); - - if ( $badImages === false ) { // cache miss - if ( $blacklist === null ) { - $blacklist = wfMessage( 'bad_image_list' )->inContentLanguage()->plain(); // site list - } - # Build the list now - $badImages = []; - $lines = explode( "\n", $blacklist ); - foreach ( $lines as $line ) { - # List items only - if ( substr( $line, 0, 1 ) !== '*' ) { - continue; - } - - # Find all links - $m = []; - if ( !preg_match_all( '/\[\[:?(.*?)\]\]/', $line, $m ) ) { - continue; - } - - $exceptions = []; - $imageDBkey = false; - foreach ( $m[1] as $i => $titleText ) { - $title = Title::newFromText( $titleText ); - if ( !is_null( $title ) ) { - if ( $i == 0 ) { - $imageDBkey = $title->getDBkey(); - } else { - $exceptions[$title->getPrefixedDBkey()] = true; - } - } - } - - if ( $imageDBkey !== false ) { - $badImages[$imageDBkey] = $exceptions; - } - } - $cache->set( $key, $badImages, 60 ); - } - - $contextKey = $contextTitle ? $contextTitle->getPrefixedDBkey() : false; - $bad = isset( $badImages[$name] ) && !isset( $badImages[$name][$contextKey] ); - - return $bad; + $services = MediaWikiServices::getInstance(); + if ( $blacklist !== null ) { + wfDeprecated( __METHOD__ . ' with $blacklist parameter', '1.34' ); + return ( new BadFileLookup( + function () use ( $blacklist ) { + return $blacklist; + }, + $services->getLocalServerObjectCache(), + $services->getRepoGroup(), + $services->getTitleParser() + ) )->isBadFile( $name, $contextTitle ?: null ); + } + return $services->getBadFileLookup()->isBadFile( $name, $contextTitle ?: null ); } /** diff --git a/includes/MediaWikiServices.php b/includes/MediaWikiServices.php index ec82f8ed28..21fa756bbc 100644 --- a/includes/MediaWikiServices.php +++ b/includes/MediaWikiServices.php @@ -429,6 +429,14 @@ class MediaWikiServices extends ServiceContainer { return $this->getService( 'ActorMigration' ); } + /** + * @since 1.34 + * @return BadFileLookup + */ + public function getBadFileLookup() : BadFileLookup { + return $this->getService( 'BadFileLookup' ); + } + /** * @since 1.31 * @return BlobStore diff --git a/includes/ServiceWiring.php b/includes/ServiceWiring.php index 407b2e31f8..fda9a94671 100644 --- a/includes/ServiceWiring.php +++ b/includes/ServiceWiring.php @@ -39,6 +39,7 @@ use Liuggio\StatsdClient\Factory\StatsdDataFactoryInterface; use MediaWiki\Auth\AuthManager; +use MediaWiki\BadFileLookup; use MediaWiki\Block\BlockManager; use MediaWiki\Block\BlockRestrictionStore; use MediaWiki\Config\ConfigRepository; @@ -77,6 +78,17 @@ return [ ); }, + 'BadFileLookup' => function ( MediaWikiServices $services ) : BadFileLookup { + return new BadFileLookup( + function () { + return wfMessage( 'bad_image_list' )->inContentLanguage()->plain(); + }, + $services->getLocalServerObjectCache(), + $services->getRepoGroup(), + $services->getTitleParser() + ); + }, + 'BlobStore' => function ( MediaWikiServices $services ) : BlobStore { return $services->getService( '_SqlBlobStore' ); }, diff --git a/includes/api/ApiQueryImageInfo.php b/includes/api/ApiQueryImageInfo.php index 0791426f77..b97ab3c362 100644 --- a/includes/api/ApiQueryImageInfo.php +++ b/includes/api/ApiQueryImageInfo.php @@ -144,7 +144,8 @@ class ApiQueryImageInfo extends ApiQueryBase { $info['imagerepository'] = $img->getRepoName(); } if ( isset( $prop['badfile'] ) ) { - $info['badfile'] = (bool)wfIsBadImage( $title, $badFileContextTitle ); + $info['badfile'] = (bool)MediaWikiServices::getInstance()->getBadFileLookup() + ->isBadFile( $title, $badFileContextTitle ); } $fit = $result->addValue( [ 'query', 'pages' ], (int)$pageId, $info ); diff --git a/includes/gallery/TraditionalImageGallery.php b/includes/gallery/TraditionalImageGallery.php index d25d9aa861..fadd5878ba 100644 --- a/includes/gallery/TraditionalImageGallery.php +++ b/includes/gallery/TraditionalImageGallery.php @@ -111,8 +111,8 @@ class TraditionalImageGallery extends ImageGalleryBase { if ( $this->mParser instanceof Parser ) { $this->mParser->addTrackingCategory( 'broken-file-category' ); } - } elseif ( $this->mHideBadImages - && wfIsBadImage( $nt->getDBkey(), $this->getContextTitle() ) + } elseif ( $this->mHideBadImages && MediaWikiServices::getInstance()->getBadFileLookup() + ->isBadFile( $nt->getDBkey(), $this->getContextTitle() ) ) { # The image is blacklisted, just show it as a text link. $thumbhtml = "\n\t\t\t" . '
linkRendererFactory = $linkRendererFactory; $this->nsInfo = $nsInfo; $this->logger = $logger ?: new NullLogger(); + $this->badFileLookup = $badFileLookup ?? + MediaWikiServices::getInstance()->getBadFileLookup(); } /** @@ -135,7 +143,8 @@ class ParserFactory { $this->specialPageFactory, $this->linkRendererFactory, $this->nsInfo, - $this->logger + $this->logger, + $this->badFileLookup ); } } diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index e71cc88b3e..7c8df1a6f5 100644 --- a/tests/common/TestsAutoLoader.php +++ b/tests/common/TestsAutoLoader.php @@ -61,6 +61,7 @@ $wgAutoloadClasses += [ 'MediaWikiPHPUnitResultPrinter' => "$testDir/phpunit/MediaWikiPHPUnitResultPrinter.php", 'MediaWikiPHPUnitTestListener' => "$testDir/phpunit/MediaWikiPHPUnitTestListener.php", 'MediaWikiTestCase' => "$testDir/phpunit/MediaWikiIntegrationTestCase.php", + 'MediaWikiTestCaseTrait' => "$testDir/phpunit/MediaWikiTestCaseTrait.php", 'MediaWikiUnitTestCase' => "$testDir/phpunit/MediaWikiUnitTestCase.php", 'MediaWikiIntegrationTestCase' => "$testDir/phpunit/MediaWikiIntegrationTestCase.php", 'MediaWikiTestResult' => "$testDir/phpunit/MediaWikiTestResult.php", @@ -217,6 +218,9 @@ $wgAutoloadClasses += [ 'MockSearchResultSet' => "$testDir/phpunit/mocks/search/MockSearchResultSet.php", 'MockSearchResult' => "$testDir/phpunit/mocks/search/MockSearchResult.php", + # tests/phpunit/unit/includes + 'BadFileLookupTest' => "$testDir/phpunit/unit/includes/BadFileLookupTest.php", + # tests/phpunit/unit/includes/libs/filebackend/fsfile 'TempFSFileTestTrait' => "$testDir/phpunit/unit/includes/libs/filebackend/fsfile/TempFSFileTestTrait.php", diff --git a/tests/phpunit/MediaWikiIntegrationTestCase.php b/tests/phpunit/MediaWikiIntegrationTestCase.php index 496f265b37..64d1c343c3 100644 --- a/tests/phpunit/MediaWikiIntegrationTestCase.php +++ b/tests/phpunit/MediaWikiIntegrationTestCase.php @@ -23,8 +23,9 @@ use Wikimedia\TestingAccessWrapper; abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase { use MediaWikiCoversValidator; - use PHPUnit4And6Compat; use MediaWikiGroupValidator; + use MediaWikiTestCaseTrait; + use PHPUnit4And6Compat; /** * The original service locator. This is overridden during setUp(). @@ -2510,20 +2511,6 @@ abstract class MediaWikiIntegrationTestCase extends PHPUnit\Framework\TestCase { 'comment' => $comment, ] ); } - - /** - * Returns a PHPUnit constraint that matches anything other than a fixed set of values. This can - * be used to whitelist values, e.g. - * $mock->expects( $this->never() )->method( $this->anythingBut( 'foo', 'bar' ) ); - * which will throw if any unexpected method is called. - * - * @param mixed ...$values Values that are not matched - */ - protected function anythingBut( ...$values ) { - return $this->logicalNot( $this->logicalOr( - ...array_map( [ $this, 'matches' ], $values ) - ) ); - } } class_alias( 'MediaWikiIntegrationTestCase', 'MediaWikiTestCase' ); diff --git a/tests/phpunit/MediaWikiTestCaseTrait.php b/tests/phpunit/MediaWikiTestCaseTrait.php new file mode 100644 index 0000000000..77d7c04482 --- /dev/null +++ b/tests/phpunit/MediaWikiTestCaseTrait.php @@ -0,0 +1,20 @@ +expects( $this->never() )->method( $this->anythingBut( 'foo', 'bar' ) ); + * which will throw if any unexpected method is called. + * + * @param mixed ...$values Values that are not matched + */ + protected function anythingBut( ...$values ) { + return $this->logicalNot( $this->logicalOr( + ...array_map( [ $this, 'matches' ], $values ) + ) ); + } +} diff --git a/tests/phpunit/MediaWikiUnitTestCase.php b/tests/phpunit/MediaWikiUnitTestCase.php index 5f7746b3e6..ccf3357f63 100644 --- a/tests/phpunit/MediaWikiUnitTestCase.php +++ b/tests/phpunit/MediaWikiUnitTestCase.php @@ -26,10 +26,13 @@ use PHPUnit\Framework\TestCase; * * Extend this class if you are testing classes which use dependency injection and do not access * global functions, variables, services or a storage backend. + * + * @since 1.34 */ abstract class MediaWikiUnitTestCase extends TestCase { use PHPUnit4And6Compat; use MediaWikiCoversValidator; + use MediaWikiTestCaseTrait; private $unitGlobals = []; @@ -38,7 +41,7 @@ abstract class MediaWikiUnitTestCase extends TestCase { $reflection = new ReflectionClass( $this ); $dirSeparator = DIRECTORY_SEPARATOR; if ( strpos( $reflection->getFilename(), "${dirSeparator}unit${dirSeparator}" ) === false ) { - $this->fail( 'This unit test needs to be in "tests/phpunit/unit" !' ); + $this->fail( 'This unit test needs to be in "tests/phpunit/unit"!' ); } $this->unitGlobals = $GLOBALS; unset( $GLOBALS ); @@ -54,4 +57,19 @@ abstract class MediaWikiUnitTestCase extends TestCase { $GLOBALS = $this->unitGlobals; parent::tearDown(); } + + /** + * Create a temporary hook handler which will be reset by tearDown. + * This replaces other handlers for the same hook. + * @param string $hookName Hook name + * @param mixed $handler Value suitable for a hook handler + * @since 1.34 + */ + protected function setTemporaryHook( $hookName, $handler ) { + // This will be reset by tearDown() when it restores globals. We don't want to use + // Hooks::register()/clear() because they won't replace other handlers for the same hook, + // which doesn't match behavior of MediaWikiIntegrationTestCase. + global $wgHooks; + $wgHooks[$hookName] = [ $handler ]; + } } diff --git a/tests/phpunit/includes/GlobalFunctions/GlobalWithDBTest.php b/tests/phpunit/includes/GlobalFunctions/GlobalWithDBTest.php index f0f449bd14..95571f23da 100644 --- a/tests/phpunit/includes/GlobalFunctions/GlobalWithDBTest.php +++ b/tests/phpunit/includes/GlobalFunctions/GlobalWithDBTest.php @@ -5,36 +5,6 @@ * @group Database */ class GlobalWithDBTest extends MediaWikiTestCase { - const FILE_BLACKLIST = <<]] doesn't break anything, the line is ignored [[File:Good.jpg]] -* [[File:Bad5.jpg]] before [[malformed title<>]] doesn't ignore the line -WIKITEXT; - - public static function badImageHook( $name, &$bad ) { - switch ( $name ) { - case 'Hook_bad.jpg': - case 'Redirect_to_hook_good.jpg': - $bad = true; - return false; - - case 'Hook_good.jpg': - case 'Redirect_to_hook_bad.jpg': - $bad = false; - return false; - } - - return true; - } - private function setUpBadImageTests( $name ) { if ( in_array( $name, [ 'Hook bad.jpg', @@ -58,17 +28,17 @@ WIKITEXT; $this->editPage( 'File:Redirect to hook bad.jpg', '#REDIRECT [[Hook bad.jpg]]' ); $this->editPage( 'File:Redirect to hook good.jpg', '#REDIRECT [[Hook good.jpg]]' ); - $this->setTemporaryHook( 'BadImage', __CLASS__ . '::badImageHook' ); + $this->setTemporaryHook( 'BadImage', 'BadFileLookupTest::badImageHook' ); } /** - * @dataProvider provideIsBadFile + * @dataProvider BadFileLookupTest::provideIsBadFile * @covers ::wfIsBadImage */ public function testWfIsBadImage( $name, $title, $expected ) { $this->setUpBadImageTests( $name ); - $this->editPage( 'MediaWiki:Bad image list', self::FILE_BLACKLIST ); + $this->editPage( 'MediaWiki:Bad image list', BadFileLookupTest::BLACKLIST ); $this->resetServices(); // Enable messages from MediaWiki namespace MessageCache::singleton()->enable(); @@ -77,36 +47,13 @@ WIKITEXT; } /** - * @dataProvider provideIsBadFile + * @dataProvider BadFileLookupTest::provideIsBadFile * @covers ::wfIsBadImage */ public function testWfIsBadImage_blacklistParam( $name, $title, $expected ) { $this->setUpBadImageTests( $name ); - $this->assertSame( $expected, wfIsBadImage( $name, $title, self::FILE_BLACKLIST ) ); - } - - public static function provideIsBadFile() { - return [ - 'No context page' => [ 'Bad.jpg', null, true ], - 'Context page not whitelisted' => - [ 'Bad.jpg', Title::makeTitleSafe( NS_MAIN, 'A page' ), true ], - 'Good image' => [ 'Good.jpg', null, false ], - 'Whitelisted context page' => - [ 'Bad.jpg', Title::makeTitleSafe( NS_MAIN, 'Nasty page' ), false ], - 'Bad image with Image:' => [ 'Image:Bad.jpg', null, false ], - 'Bad image with File:' => [ 'File:Bad.jpg', null, false ], - 'Bad image with Image: in blacklist' => [ 'Bad2.jpg', null, true ], - 'Bad image without prefix in blacklist' => [ 'Bad3.jpg', null, true ], - 'Bad image with different namespace in blacklist' => [ 'Bad4.jpg', null, true ], - 'Redirect to bad image' => [ 'Redirect to bad.jpg', null, true ], - 'Redirect to good image' => [ 'Redirect_to_good.jpg', null, false ], - 'Hook says bad (with space)' => [ 'Hook bad.jpg', null, true ], - 'Hook says bad (with underscore)' => [ 'Hook_bad.jpg', null, true ], - 'Hook says good' => [ 'Hook good.jpg', null, false ], - 'Redirect to hook bad image' => [ 'Redirect to hook bad.jpg', null, true ], - 'Redirect to hook good image' => [ 'Redirect to hook good.jpg', null, false ], - 'Malformed title doesn\'t break the line' => [ 'Bad5.jpg', null, true ], - ]; + $this->hideDeprecated( 'wfIsBadImage with $blacklist parameter' ); + $this->assertSame( $expected, wfIsBadImage( $name, $title, BadFileLookupTest::BLACKLIST ) ); } } diff --git a/tests/phpunit/unit/includes/BadFileLookupTest.php b/tests/phpunit/unit/includes/BadFileLookupTest.php new file mode 100644 index 0000000000..6ecfe37d99 --- /dev/null +++ b/tests/phpunit/unit/includes/BadFileLookupTest.php @@ -0,0 +1,185 @@ +]] doesn't break anything, the line is ignored [[File:Good.jpg]] +* [[File:Bad5.jpg]] before [[malformed title<>]] doesn't ignore the line +WIKITEXT; + + /** Shared with GlobalWithDBTest */ + public static function badImageHook( $name, &$bad ) { + switch ( $name ) { + case 'Hook_bad.jpg': + case 'Redirect_to_hook_good.jpg': + $bad = true; + return false; + + case 'Hook_good.jpg': + case 'Redirect_to_hook_bad.jpg': + $bad = false; + return false; + } + + return true; + } + + private function getMockRepoGroup() { + $mock = $this->createMock( RepoGroup::class ); + $mock->expects( $this->once() )->method( 'findFile' ) + ->will( $this->returnCallback( function ( $name ) { + $mockFile = $this->createMock( File::class ); + $mockFile->expects( $this->once() )->method( 'getTitle' ) + ->will( $this->returnCallback( function () use ( $name ) { + switch ( $name ) { + case 'Redirect to bad.jpg': + return new TitleValue( NS_FILE, 'Bad.jpg' ); + case 'Redirect_to_good.jpg': + return new TitleValue( NS_FILE, 'Good.jpg' ); + case 'Redirect to hook bad.jpg': + return new TitleValue( NS_FILE, 'Hook_bad.jpg' ); + case 'Redirect to hook good.jpg': + return new TitleValue( NS_FILE, 'Hook_good.jpg' ); + default: + return new TitleValue( NS_FILE, $name ); + } + } ) ); + $mockFile->expects( $this->never() )->method( $this->anythingBut( 'getTitle' ) ); + return $mockFile; + } ) ); + $mock->expects( $this->never() )->method( $this->anythingBut( 'findFile' ) ); + + return $mock; + } + + /** + * Just returns null for every findFile(). + */ + private function getMockRepoGroupNull() { + $mock = $this->createMock( RepoGroup::class ); + $mock->expects( $this->once() )->method( 'findFile' )->willReturn( null ); + $mock->expects( $this->never() )->method( $this->anythingBut( 'findFile' ) ); + + return $mock; + } + + private function getMockTitleParser() { + $mock = $this->createMock( TitleParser::class ); + $mock->method( 'parseTitle' )->will( $this->returnCallback( function ( $text ) { + if ( strpos( $text, '<' ) !== false ) { + throw $this->createMock( MalformedTitleException::class ); + } + if ( strpos( $text, ':' ) === false ) { + return new TitleValue( NS_MAIN, $text ); + } + list( $ns, $text ) = explode( ':', $text ); + switch ( $ns ) { + case 'Image': + case 'File': + $ns = NS_FILE; + break; + + case 'User': + $ns = NS_USER; + break; + } + return new TitleValue( $ns, $text ); + } ) ); + $mock->expects( $this->never() )->method( $this->anythingBut( 'parseTitle' ) ); + + return $mock; + } + + public function setUp() { + parent::setUp(); + + $this->setTemporaryHook( 'BadImage', __CLASS__ . '::badImageHook' ); + } + + /** + * @dataProvider provideIsBadFile + * @covers ::__construct + * @covers ::isBadFile + */ + public function testIsBadFile( $name, $title, $expected ) { + $bfl = new BadFileLookup( + function () { + return self::BLACKLIST; + }, + new EmptyBagOStuff, + $this->getMockRepoGroup(), + $this->getMockTitleParser() + ); + + $this->assertSame( $expected, $bfl->isBadFile( $name, $title ) ); + } + + /** + * @dataProvider provideIsBadFile + * @covers ::__construct + * @covers ::isBadFile + */ + public function testIsBadFile_nullRepoGroup( $name, $title, $expected ) { + $bfl = new BadFileLookup( + function () { + return self::BLACKLIST; + }, + new EmptyBagOStuff, + $this->getMockRepoGroupNull(), + $this->getMockTitleParser() + ); + + // Hack -- these expectations are reversed if the repo group returns null. In that case 1) + // we don't honor redirects, and 2) we don't replace spaces by underscores (which makes the + // hook not see 'Hook bad.jpg'). + if ( in_array( $name, [ + 'Redirect to bad.jpg', + 'Redirect_to_good.jpg', + 'Hook bad.jpg', + 'Redirect to hook bad.jpg', + ] ) ) { + $expected = !$expected; + } + + $this->assertSame( $expected, $bfl->isBadFile( $name, $title ) ); + } + + /** Shared with GlobalWithDBTest */ + public static function provideIsBadFile() { + return [ + 'No context page' => [ 'Bad.jpg', null, true ], + 'Context page not whitelisted' => + [ 'Bad.jpg', new TitleValue( NS_MAIN, 'A page' ), true ], + 'Good image' => [ 'Good.jpg', null, false ], + 'Whitelisted context page' => + [ 'Bad.jpg', new TitleValue( NS_MAIN, 'Nasty page' ), false ], + 'Bad image with Image:' => [ 'Image:Bad.jpg', null, false ], + 'Bad image with File:' => [ 'File:Bad.jpg', null, false ], + 'Bad image with Image: in blacklist' => [ 'Bad2.jpg', null, true ], + 'Bad image without prefix in blacklist' => [ 'Bad3.jpg', null, true ], + 'Bad image with different namespace in blacklist' => [ 'Bad4.jpg', null, true ], + 'Redirect to bad image' => [ 'Redirect to bad.jpg', null, true ], + 'Redirect to good image' => [ 'Redirect_to_good.jpg', null, false ], + 'Hook says bad (with space)' => [ 'Hook bad.jpg', null, true ], + 'Hook says bad (with underscore)' => [ 'Hook_bad.jpg', null, true ], + 'Hook says good' => [ 'Hook good.jpg', null, false ], + 'Redirect to hook bad image' => [ 'Redirect to hook bad.jpg', null, true ], + 'Redirect to hook good image' => [ 'Redirect to hook good.jpg', null, false ], + 'Malformed title doesn\'t break the line' => [ 'Bad5.jpg', null, true ], + ]; + } +} -- 2.20.1