Check page_len in ResourceLoaderWikiModule::isKnownEmpty() for 'user' modules
In most cases, we just check whether the pages exist before saying the module is not empty to avoid generating cached HTML without the appropriate <script> or <link> tags. However, for modules in the 'user' group, normal users cannot delete their personal JavaScript/CSS pages, causing needless extra requests, even though we know the pages are empty. ResourceLoader::isKnownEmpty() now checks the page_len field for modules in the 'user' group to check that there is some actual content. Bug: 68488 Change-Id: I0570f62887fd4642fd60367ae0b51d7dc19488ca
This commit is contained in:
Kunal Mehta
Ori.livneh
parent
08e50bcecd
commit
8968d8787f
4 changed files with 113 additions and 14 deletions
|
|
@ -36,8 +36,8 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule {
|
|||
# Origin is user-supplied code
|
||||
protected $origin = self::ORIGIN_USER_SITEWIDE;
|
||||
|
||||
// In-object cache for title mtimes
|
||||
protected $titleMtimes = array();
|
||||
// In-object cache for title info
|
||||
protected $titleInfo = array();
|
||||
|
||||
/* Abstract Protected Methods */
|
||||
|
||||
|
|
@ -171,8 +171,11 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule {
|
|||
*/
|
||||
public function getModifiedTime( ResourceLoaderContext $context ) {
|
||||
$modifiedTime = 1; // wfTimestamp() interprets 0 as "now"
|
||||
$mtimes = $this->getTitleMtimes( $context );
|
||||
if ( count( $mtimes ) ) {
|
||||
$titleInfo = $this->getTitleInfo( $context );
|
||||
if ( count( $titleInfo ) ) {
|
||||
$mtimes = array_map( function( $value ) {
|
||||
return $value['timestamp'];
|
||||
}, $titleInfo );
|
||||
$modifiedTime = max( $modifiedTime, max( $mtimes ) );
|
||||
}
|
||||
$modifiedTime = max(
|
||||
|
|
@ -201,16 +204,35 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule {
|
|||
* @return bool
|
||||
*/
|
||||
public function isKnownEmpty( ResourceLoaderContext $context ) {
|
||||
return count( $this->getTitleMtimes( $context ) ) == 0;
|
||||
$titleInfo = $this->getTitleInfo( $context );
|
||||
// Bug 68488: For modules in the "user" group, we should actually
|
||||
// check that the pages are empty (page_len == 0), but for other
|
||||
// groups, just check the pages exist so that we don't end up
|
||||
// caching temporarily-blank pages without the appropriate
|
||||
// <script> or <link> tag.
|
||||
if ( $this->getGroup() !== 'user' ) {
|
||||
return count( $titleInfo ) === 0;
|
||||
}
|
||||
|
||||
foreach ( $titleInfo as $info ) {
|
||||
if ( $info['length'] !== 0 ) {
|
||||
// At least one non-0-lenth page, not empty
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
// All pages are 0-length, so it's empty
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Get the modification times of all titles that would be loaded for
|
||||
* a given context.
|
||||
* @param ResourceLoaderContext $context Context object
|
||||
* @return array( prefixed DB key => UNIX timestamp ), nonexistent titles are dropped
|
||||
* @return array keyed by page dbkey, with value is an array with 'length' and 'timestamp'
|
||||
* keys, where the timestamp is a unix one
|
||||
*/
|
||||
protected function getTitleMtimes( ResourceLoaderContext $context ) {
|
||||
protected function getTitleInfo( ResourceLoaderContext $context ) {
|
||||
$dbr = $this->getDB();
|
||||
if ( !$dbr ) {
|
||||
// We're dealing with a subclass that doesn't have a DB
|
||||
|
|
@ -218,11 +240,11 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule {
|
|||
}
|
||||
|
||||
$hash = $context->getHash();
|
||||
if ( isset( $this->titleMtimes[$hash] ) ) {
|
||||
return $this->titleMtimes[$hash];
|
||||
if ( isset( $this->titleInfo[$hash] ) ) {
|
||||
return $this->titleInfo[$hash];
|
||||
}
|
||||
|
||||
$this->titleMtimes[$hash] = array();
|
||||
$this->titleInfo[$hash] = array();
|
||||
$batch = new LinkBatch;
|
||||
foreach ( $this->getPages( $context ) as $titleText => $options ) {
|
||||
$batch->addObj( Title::newFromText( $titleText ) );
|
||||
|
|
@ -230,16 +252,18 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule {
|
|||
|
||||
if ( !$batch->isEmpty() ) {
|
||||
$res = $dbr->select( 'page',
|
||||
array( 'page_namespace', 'page_title', 'page_touched' ),
|
||||
array( 'page_namespace', 'page_title', 'page_touched', 'page_len' ),
|
||||
$batch->constructSet( 'page', $dbr ),
|
||||
__METHOD__
|
||||
);
|
||||
foreach ( $res as $row ) {
|
||||
$title = Title::makeTitle( $row->page_namespace, $row->page_title );
|
||||
$this->titleMtimes[$hash][$title->getPrefixedDBkey()] =
|
||||
wfTimestamp( TS_UNIX, $row->page_touched );
|
||||
$this->titleInfo[$hash][$title->getPrefixedDBkey()] = array(
|
||||
'timestamp' => wfTimestamp( TS_UNIX, $row->page_touched ),
|
||||
'length' => $row->page_len,
|
||||
);
|
||||
}
|
||||
}
|
||||
return $this->titleMtimes[$hash];
|
||||
return $this->titleInfo[$hash];
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -44,6 +44,7 @@ $wgAutoloadClasses += array(
|
|||
'ResourceLoaderTestCase' => "$testDir/phpunit/ResourceLoaderTestCase.php",
|
||||
'ResourceLoaderTestModule' => "$testDir/phpunit/ResourceLoaderTestCase.php",
|
||||
'ResourceLoaderFileModuleTestModule' => "$testDir/phpunit/ResourceLoaderTestCase.php",
|
||||
'ResourceLoaderWikiModuleTestModule' => "$testDir/phpunit/ResourceLoaderTestCase.php",
|
||||
'TestUser' => "$testDir/phpunit/includes/TestUser.php",
|
||||
'LessFileCompilationTest' => "$testDir/phpunit/LessFileCompilationTest.php",
|
||||
|
||||
|
|
|
|||
|
|
@ -81,3 +81,10 @@ class ResourceLoaderTestModule extends ResourceLoaderModule {
|
|||
|
||||
class ResourceLoaderFileModuleTestModule extends ResourceLoaderFileModule {
|
||||
}
|
||||
|
||||
class ResourceLoaderWikiModuleTestModule extends ResourceLoaderWikiModule {
|
||||
// Override expected via PHPUnit mocks and stubs
|
||||
protected function getPages( ResourceLoaderContext $context ) {
|
||||
return array();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,67 @@
|
|||
<?php
|
||||
|
||||
class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase {
|
||||
|
||||
/**
|
||||
* @covers ResourceLoaderWikiModule::isKnownEmpty
|
||||
* @dataProvider provideIsKnownEmpty
|
||||
*/
|
||||
public function testIsKnownEmpty( $titleInfo, $group, $expected ) {
|
||||
$module = $this->getMockBuilder( 'ResourceLoaderWikiModuleTestModule' )
|
||||
->setMethods( array( 'getTitleInfo', 'getGroup' ) )
|
||||
->getMock();
|
||||
$module->expects( $this->any() )
|
||||
->method( 'getTitleInfo' )
|
||||
->will( $this->returnValue( $titleInfo ) );
|
||||
$module->expects( $this->any() )
|
||||
->method( 'getGroup' )
|
||||
->will( $this->returnValue( $group ) );
|
||||
$context = $this->getMockBuilder( 'ResourceLoaderContext' )
|
||||
->disableOriginalConstructor()
|
||||
->getMock();
|
||||
$this->assertEquals( $expected, $module->isKnownEmpty( $context ) );
|
||||
}
|
||||
|
||||
public function provideIsKnownEmpty() {
|
||||
return array(
|
||||
// No valid pages
|
||||
array( array(), 'test1', true ),
|
||||
// 'site' module with a non-empty page
|
||||
array(
|
||||
array(
|
||||
'MediaWiki:Common.js' => array(
|
||||
'timestamp' => 123456789,
|
||||
'length' => 1234
|
||||
)
|
||||
), 'site', false,
|
||||
),
|
||||
// 'site' module with an empty page
|
||||
array(
|
||||
array(
|
||||
'MediaWiki:Monobook.js' => array(
|
||||
'timestamp' => 987654321,
|
||||
'length' => 0,
|
||||
),
|
||||
), 'site', false,
|
||||
),
|
||||
// 'user' module with a non-empty page
|
||||
array(
|
||||
array(
|
||||
'User:FooBar/common.js' => array(
|
||||
'timestamp' => 246813579,
|
||||
'length' => 25,
|
||||
),
|
||||
), 'user', false,
|
||||
),
|
||||
// 'user' module with an empty page
|
||||
array(
|
||||
array(
|
||||
'User:FooBar/monobook.js' => array(
|
||||
'timestamp' => 1357924680,
|
||||
'length' => 0,
|
||||
),
|
||||
), 'user', true,
|
||||
),
|
||||
);
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue