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

@@ -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 ===
 * …

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',

includes/BadFileLookup.php

@@ -0,0 +1,127 @@
+<?php
+
+namespace MediaWiki;
+
+use BagOStuff;
+use Hooks;
+use MalformedTitleException;
+use MediaWiki\Linker\LinkTarget;
+use RepoGroup;
+use TitleParser;
+
+class BadFileLookup {
+	/** @var callable Returns contents of blacklist (see comment for isBadFile()) */
+	private $blacklistCallback;
+
+	/** @var BagOStuff Cache of parsed bad image list */
+	private $cache;
+
+	/** @var RepoGroup */
+	private $repoGroup;
+
+	/** @var TitleParser */
+	private $titleParser;
+
+	/** @var array|null Parsed blacklist */
+	private $badFiles;
+
+	/**
+	 * Do not call directly. Use MediaWikiServices.
+	 *
+	 * @param callable $blacklistCallback Callback that returns wikitext of a file blacklist
+	 * @param BagOStuff $cache For caching parsed versions of the blacklist
+	 * @param RepoGroup $repoGroup
+	 * @param TitleParser $titleParser
+	 */
+	public function __construct(
+		callable $blacklistCallback,
+		BagOStuff $cache,
+		RepoGroup $repoGroup,
+		TitleParser $titleParser
+	) {
+		$this->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()] ) );
+	}
+}

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;
+	$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 );
 	}
-
-	$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;
+	return $services->getBadFileLookup()->isBadFile( $name, $contextTitle ?: null );
 }
 
 /**

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

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' );
 	},

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 );

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" . '<div class="thumb" style="height: ' .

includes/parser/Parser.php

@@ -20,6 +20,7 @@
  * @file
  * @ingroup Parser
  */
+use MediaWiki\BadFileLookup;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Linker\LinkRenderer;
 use MediaWiki\Linker\LinkRendererFactory;
@@ -299,6 +300,9 @@ class Parser {
 	/** @var LoggerInterface */
 	private $logger;
 
+	/** @var BadFileLookup */
+	private $badFileLookup;
+
 	/**
 	 * TODO Make this a const when HHVM support is dropped (T192166)
 	 *
@@ -339,6 +343,7 @@ class Parser {
 	 * @param LinkRendererFactory|null $linkRendererFactory
 	 * @param NamespaceInfo|null $nsInfo
 	 * @param LoggerInterface|null $logger
+	 * @param BadFileLookup|null $badFileLookup
 	 */
 	public function __construct(
 		$svcOptions = null,
@@ -349,7 +354,8 @@ class Parser {
 		SpecialPageFactory $spFactory = null,
 		$linkRendererFactory = null,
 		$nsInfo = null,
-		$logger = null
+		$logger = null,
+		BadFileLookup $badFileLookup = null
 	) {
 		if ( !$svcOptions || is_array( $svcOptions ) ) {
 			// Pre-1.34 calling convention is the first parameter is just ParserConf, the seventh is
@@ -396,6 +402,8 @@ class Parser {
 			MediaWikiServices::getInstance()->getLinkRendererFactory();
 		$this->nsInfo = $nsInfo ?? MediaWikiServices::getInstance()->getNamespaceInfo();
 		$this->logger = $logger ?: new NullLogger();
+		$this->badFileLookup = $badFileLookup ??
+			MediaWikiServices::getInstance()->getBadFileLookup();
 	}
 
 	/**
@@ -2498,7 +2506,7 @@ class Parser {
 				}
 
 				if ( $ns == NS_FILE ) {
-					if ( !wfIsBadImage( $nt->getDBkey(), $this->mTitle ) ) {
+					if ( !$this->badFileLookup->isBadFile( $nt->getDBkey(), $this->mTitle ) ) {
 						if ( $wasblank ) {
 							# if no parameters were passed, $text
 							# becomes something like "File:Foo.png",

includes/parser/ParserFactory.php

@@ -19,6 +19,7 @@
  * @ingroup Parser
  */
 
+use MediaWiki\BadFileLookup;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Linker\LinkRendererFactory;
 use MediaWiki\MediaWikiServices;
@@ -54,6 +55,9 @@ class ParserFactory {
 	/** @var LoggerInterface */
 	private $logger;
 
+	/** @var BadFileLookup */
+	private $badFileLookup;
+
 	/**
 	 * Old parameter list, which we support for backwards compatibility, were:
 	 *   array $parserConf See $wgParserConf documentation
@@ -77,6 +81,7 @@ class ParserFactory {
 	 * @param LinkRendererFactory $linkRendererFactory
 	 * @param NamespaceInfo|LinkRendererFactory|null $nsInfo
 	 * @param LoggerInterface|null $logger
+	 * @param BadFileLookup|null $badFileLookup
 	 * @since 1.32
 	 */
 	public function __construct(
@@ -87,7 +92,8 @@ class ParserFactory {
 		SpecialPageFactory $spFactory,
 		$linkRendererFactory,
 		$nsInfo = null,
-		$logger = null
+		$logger = null,
+		BadFileLookup $badFileLookup = null
 	) {
 		// @todo Do we need to retain compat for constructing this class directly?
 		if ( !$nsInfo ) {
@@ -119,6 +125,8 @@ class ParserFactory {
 		$this->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
 		);
 	}
 }

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",
 

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' );

tests/phpunit/MediaWikiTestCaseTrait.php

@@ -0,0 +1,20 @@
+<?php
+
+/**
+ * For code common to both MediaWikiUnitTestCase and MediaWikiIntegrationTestCase.
+ */
+trait MediaWikiTestCaseTrait {
+	/**
+	 * 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 )
+		) );
+	}
+}

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 ];
+	}
 }

tests/phpunit/includes/GlobalFunctions/GlobalWithDBTest.php

@@ -5,36 +5,6 @@
  * @group Database
  */
 class GlobalWithDBTest extends MediaWikiTestCase {
-	const FILE_BLACKLIST = <<<WIKITEXT
-Comment line, no effect [[File:Good.jpg]]
- * Indented list is also a comment [[File:Good.jpg]]
-* [[File:Bad.jpg]] except [[Nasty page]]
-*[[Image:Bad2.jpg]] also works
-* So does [[Bad3.jpg]]
-* [[User:Bad4.jpg]] works although it is silly
-* [[File:Redirect to good.jpg]] doesn't do anything if RepoGroup is working, because we only look at
-  the final name, but will work if RepoGroup returns null
-* List line with no link
-* [[Malformed title<>]] 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 ) );
 	}
 }

tests/phpunit/unit/includes/BadFileLookupTest.php

@@ -0,0 +1,185 @@
+<?php
+
+use MediaWiki\BadFileLookup;
+
+/**
+ * @coversDefaultClass MediaWiki\BadFileLookup
+ */
+class BadFileLookupTest extends MediaWikiUnitTestCase {
+	/** Shared with GlobalWithDBTest */
+	const BLACKLIST = <<<WIKITEXT
+Comment line, no effect [[File:Good.jpg]]
+ * Indented list is also a comment [[File:Good.jpg]]
+* [[File:Bad.jpg]] except [[Nasty page]]
+*[[Image:Bad2.jpg]] also works
+* So does [[Bad3.jpg]]
+* [[User:Bad4.jpg]] works although it is silly
+* [[File:Redirect to good.jpg]] doesn't do anything if RepoGroup is working, because we only look at
+  the final name, but will work if RepoGroup returns null
+* List line with no link
+* [[Malformed title<>]] 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 ],
+		];
+	}
+}