From dbe592df6d1914eb5d9a57c66792cbf4d59e3be7 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Wed, 14 Sep 2016 14:14:31 -0700 Subject: [PATCH] resourceloader: Fix WikiModule preload to support localised titles Follows-up 6f8dc27ca2 and dbd11e04aa. You'd expect a bug in preloading to fallback to fetching it on-demand but due to a title format mismatch the preload method did use the same title format for the cache key, but not the same title format for discovering the results. As such, it set the right cache key to an empty array. * Make relevant methods testable and mockable. * Add regression test. * Change call to array_interect_key to use the same format as fetchTitleInfo(): Normalised title keys from Title::getPrefixedText. Previously, the intersect used the declared titles from getPages() which are not localised - causing an empty array to be returned from the intersect on wikis where the namespace name is localised. Bug: T145673 Change-Id: Ibe788157724d73c727b9e2127b6828db32ca9420 --- .../ResourceLoaderWikiModule.php | 15 +++- tests/phpunit/ResourceLoaderTestCase.php | 4 +- .../ResourceLoaderWikiModuleTest.php | 79 +++++++++++++++++++ 3 files changed, 91 insertions(+), 7 deletions(-) diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php index 5580306a13..4fdd86eaca 100644 --- a/includes/resourceloader/ResourceLoaderWikiModule.php +++ b/includes/resourceloader/ResourceLoaderWikiModule.php @@ -296,12 +296,12 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule { sort( $pageNames ); $key = implode( '|', $pageNames ); if ( !isset( $this->titleInfo[$key] ) ) { - $this->titleInfo[$key] = self::fetchTitleInfo( $dbr, $pageNames, __METHOD__ ); + $this->titleInfo[$key] = static::fetchTitleInfo( $dbr, $pageNames, __METHOD__ ); } return $this->titleInfo[$key]; } - private static function fetchTitleInfo( IDatabase $db, array $pages, $fname = __METHOD__ ) { + protected static function fetchTitleInfo( IDatabase $db, array $pages, $fname = __METHOD__ ) { $titleInfo = []; $batch = new LinkBatch; foreach ( $pages as $titleText ) { @@ -353,10 +353,17 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule { } } } - $allInfo = self::fetchTitleInfo( $db, array_keys( $allPages ), __METHOD__ ); + $allInfo = static::fetchTitleInfo( $db, array_keys( $allPages ), __METHOD__ ); foreach ( $wikiModules as $module ) { $pages = $module->getPages( $context ); - $info = array_intersect_key( $allInfo, $pages ); + // Before we intersect, map the names to canonical form (T145673). + $intersect = []; + foreach ( $pages as $page => $unused ) { + $title = Title::newFromText( $page )->getPrefixedText(); + $intersect[$title] = 1; + } + $info = array_intersect_key( $allInfo, $intersect ); + $pageNames = array_keys( $pages ); sort( $pageNames ); $key = implode( '|', $pageNames ); diff --git a/tests/phpunit/ResourceLoaderTestCase.php b/tests/phpunit/ResourceLoaderTestCase.php index 18a49f60bd..f0eb12ecea 100644 --- a/tests/phpunit/ResourceLoaderTestCase.php +++ b/tests/phpunit/ResourceLoaderTestCase.php @@ -22,9 +22,7 @@ abstract class ResourceLoaderTestCase extends MediaWikiTestCase { ->setConstructorArgs( [ $resourceLoader, $request ] ) ->setMethods( [ 'getDirection' ] ) ->getMock(); - $ctx->expects( $this->any() )->method( 'getDirection' )->will( - $this->returnValue( $dir ) - ); + $ctx->method( 'getDirection' )->willReturn( $dir ); return $ctx; } diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php index 404fd970da..b12d235f26 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php @@ -138,4 +138,83 @@ class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase { ], ]; } + + /** + * @covers ResourceLoaderWikiModule::getTitleInfo + */ + public function testGetTitleInfo() { + $pages = [ + 'MediaWiki:Common.css' => [ 'type' => 'styles' ], + 'mediawiki: fallback.css' => [ 'type' => 'styles' ], + ]; + $titleInfo = [ + 'MediaWiki:Common.css' => [ 'page_len' => 1234 ], + 'MediaWiki:Fallback.css' => [ 'page_len' => 0 ], + ]; + $expected = $titleInfo; + + $module = $this->getMockBuilder( 'TestResourceLoaderWikiModule' ) + ->setMethods( [ 'getPages' ] ) + ->getMock(); + $module->method( 'getPages' )->willReturn( $pages ); + // Can't mock static methods + $module::$returnFetchTitleInfo = $titleInfo; + + $context = $this->getMockBuilder( 'ResourceLoaderContext' ) + ->disableOriginalConstructor() + ->getMock(); + + $module = TestingAccessWrapper::newFromObject( $module ); + $this->assertEquals( $expected, $module->getTitleInfo( $context ), 'Title info' ); + } + + /** + * @covers ResourceLoaderWikiModule::getTitleInfo + * @covers ResourceLoaderWikiModule::setTitleInfo + * @covers ResourceLoaderWikiModule::preloadTitleInfo + */ + public function testGetPreloadedTitleInfo() { + $pages = [ + 'MediaWiki:Common.css' => [ 'type' => 'styles' ], + // Regression against T145673. It's impossible to statically declare page names in + // a canonical way since the canonical prefix is localised. As such, the preload + // cache computed the right cache key, but failed to find the results when + // doing an intersect on the canonical result, producing an empty array. + 'mediawiki: fallback.css' => [ 'type' => 'styles' ], + ]; + $titleInfo = [ + 'MediaWiki:Common.css' => [ 'page_len' => 1234 ], + 'MediaWiki:Fallback.css' => [ 'page_len' => 0 ], + ]; + $expected = $titleInfo; + + $module = $this->getMockBuilder( 'TestResourceLoaderWikiModule' ) + ->setMethods( [ 'getPages' ] ) + ->getMock(); + $module->method( 'getPages' )->willReturn( $pages ); + // Can't mock static methods + $module::$returnFetchTitleInfo = $titleInfo; + + $rl = new EmptyResourceLoader(); + $rl->register( 'testmodule', $module ); + $context = new ResourceLoaderContext( $rl, new FauxRequest() ); + + TestResourceLoaderWikiModule::preloadTitleInfo( + $context, + wfGetDB( DB_REPLICA ), + [ 'testmodule' ] + ); + + $module = TestingAccessWrapper::newFromObject( $module ); + $this->assertEquals( $expected, $module->getTitleInfo( $context ), 'Title info' ); + } +} + +class TestResourceLoaderWikiModule extends ResourceLoaderWikiModule { + public static $returnFetchTitleInfo = null; + protected static function fetchTitleInfo( IDatabase $db, array $pages, $fname = null ) { + $ret = self::$returnFetchTitleInfo; + self::$returnFetchTitleInfo = null; + return $ret; + } } -- 2.20.1