From dfb754cbc633b7b653ae94bf9d64a88d6adad7c7 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Tue, 10 Apr 2018 18:29:50 +0100 Subject: [PATCH] resourceloader: Improve titleInfo docs and simplify title key * Document the structure of the in-process $titleInfo cache. Specifically, specify that it is not the value from getTitleInfo(), but rather a container for zero or more versions of such values. The reason this is fragmented is because ResourceLoaderContext is a parameter to most methods and as such, makes everything variable. Tracked as T99107. * Make various bits easier to understand by consistently refering to the container keys as "batchKey", and referring to the internal keys as "titleKey". * Centralise title key logic by moving to private method. * Replace the internal creation of titleKey to be based on LinkTarget with plain namespace IDs and db keys, instead of invoking the expensive getPrefixedTitle function which involves quite a lot of overhead (TitleCodec, GenderCache, Database, Language, LocalisationCache, ..). Change-Id: I701e5156ef7815a0e36caefae5871524eff3f688 --- .../ResourceLoaderWikiModule.php | 48 +++++++++++++------ .../ResourceLoaderWikiModuleTest.php | 19 ++++++-- 2 files changed, 47 insertions(+), 20 deletions(-) diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php index 5b512af7b9..0926b60650 100644 --- a/includes/resourceloader/ResourceLoaderWikiModule.php +++ b/includes/resourceloader/ResourceLoaderWikiModule.php @@ -22,6 +22,7 @@ * @author Roan Kattouw */ +use MediaWiki\Linker\LinkTarget; use Wikimedia\Rdbms\Database; use Wikimedia\Rdbms\IDatabase; @@ -50,7 +51,19 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule { // Origin defaults to users with sitewide authority protected $origin = self::ORIGIN_USER_SITEWIDE; - // In-process cache for title info + // In-process cache for title info, structured as an array + // [ + // // Pipe-separated list of sorted keys from getPages + // => [ + // => [ // Normalised title key + // 'page_len' => .., + // 'page_latest' => .., + // 'page_touched' => .., + // ] + // ] + // ] + // @see self::fetchTitleInfo() + // @see self::makeTitleKey() protected $titleInfo = []; // List of page names that contain CSS @@ -295,8 +308,13 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule { return count( $revisions ) === 0; } - private function setTitleInfo( $key, array $titleInfo ) { - $this->titleInfo[$key] = $titleInfo; + private function setTitleInfo( $batchKey, array $titleInfo ) { + $this->titleInfo[$batchKey] = $titleInfo; + } + + private static function makeTitleKey( LinkTarget $title ) { + // Used for keys in titleInfo. + return "{$title->getNamespace()}:{$title->getDBkey()}"; } /** @@ -313,11 +331,11 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule { $pageNames = array_keys( $this->getPages( $context ) ); sort( $pageNames ); - $key = implode( '|', $pageNames ); - if ( !isset( $this->titleInfo[$key] ) ) { - $this->titleInfo[$key] = static::fetchTitleInfo( $dbr, $pageNames, __METHOD__ ); + $batchKey = implode( '|', $pageNames ); + if ( !isset( $this->titleInfo[$batchKey] ) ) { + $this->titleInfo[$batchKey] = static::fetchTitleInfo( $dbr, $pageNames, __METHOD__ ); } - return $this->titleInfo[$key]; + return $this->titleInfo[$batchKey]; } protected static function fetchTitleInfo( IDatabase $db, array $pages, $fname = __METHOD__ ) { @@ -340,8 +358,8 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule { foreach ( $res as $row ) { // Avoid including ids or timestamps of revision/page tables so // that versions are not wasted - $title = Title::makeTitle( $row->page_namespace, $row->page_title ); - $titleInfo[$title->getPrefixedText()] = [ + $title = new TitleValue( (int)$row->page_namespace, $row->page_title ); + $titleInfo[ self::makeTitleKey( $title ) ] = [ 'page_len' => $row->page_len, 'page_latest' => $row->page_latest, 'page_touched' => $row->page_touched, @@ -410,23 +428,23 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule { $pages = $wikiModule->getPages( $context ); // Before we intersect, map the names to canonical form (T145673). $intersect = []; - foreach ( $pages as $page => $unused ) { - $title = Title::newFromText( $page ); + foreach ( $pages as $pageName => $unused ) { + $title = Title::newFromText( $pageName ); if ( $title ) { - $intersect[ $title->getPrefixedText() ] = 1; + $intersect[ self::makeTitleKey( $title ) ] = 1; } else { // Page name may be invalid if user-provided (e.g. gadgets) $rl->getLogger()->info( 'Invalid wiki page title "{title}" in ' . __METHOD__, - [ 'title' => $page ] + [ 'title' => $pageName ] ); } } $info = array_intersect_key( $allInfo, $intersect ); $pageNames = array_keys( $pages ); sort( $pageNames ); - $key = implode( '|', $pageNames ); - $wikiModule->setTitleInfo( $key, $info ); + $batchKey = implode( '|', $pageNames ); + $wikiModule->setTitleInfo( $batchKey, $info ); } } diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php index 0aa37d23d3..d4b9c16512 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php @@ -15,6 +15,15 @@ class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase { $this->assertInstanceOf( ResourceLoaderWikiModule::class, $module ); } + private function prepareTitleInfo( array $mockInfo ) { + $module = TestingAccessWrapper::newFromClass( ResourceLoaderWikiModule::class ); + $info = []; + foreach ( $mockInfo as $key => $val ) { + $info[ $module->makeTitleKey( Title::newFromText( $key ) ) ] = $val; + } + return $info; + } + public static function provideConstructor() { return [ // Nothing @@ -102,7 +111,7 @@ class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase { ->getMock(); $module->expects( $this->any() ) ->method( 'getTitleInfo' ) - ->will( $this->returnValue( $titleInfo ) ); + ->will( $this->returnValue( $this->prepareTitleInfo( $titleInfo ) ) ); $module->expects( $this->any() ) ->method( 'getGroup' ) ->will( $this->returnValue( $group ) ); @@ -151,10 +160,10 @@ class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase { 'MediaWiki:Common.css' => [ 'type' => 'styles' ], 'mediawiki: fallback.css' => [ 'type' => 'styles' ], ]; - $titleInfo = [ + $titleInfo = $this->prepareTitleInfo( [ 'MediaWiki:Common.css' => [ 'page_len' => 1234 ], 'MediaWiki:Fallback.css' => [ 'page_len' => 0 ], - ]; + ] ); $expected = $titleInfo; $module = $this->getMockBuilder( TestResourceLoaderWikiModule::class ) @@ -186,10 +195,10 @@ class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase { // doing an intersect on the canonical result, producing an empty array. 'mediawiki: fallback.css' => [ 'type' => 'styles' ], ]; - $titleInfo = [ + $titleInfo = $this->prepareTitleInfo( [ 'MediaWiki:Common.css' => [ 'page_len' => 1234 ], 'MediaWiki:Fallback.css' => [ 'page_len' => 0 ], - ]; + ] ); $expected = $titleInfo; $module = $this->getMockBuilder( TestResourceLoaderWikiModule::class ) -- 2.20.1