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

diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php
index 58b7fa9..d45316f 100644 (file)
--- a/includes/resourceloader/ResourceLoaderWikiModule.php
+++ b/includes/resourceloader/ResourceLoaderWikiModule.php
@@ -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];
        }
 }

diff --git a/tests/TestsAutoLoader.php b/tests/TestsAutoLoader.php
index 0078d7e..2e8fed4 100644 (file)
--- a/tests/TestsAutoLoader.php
+++ b/tests/TestsAutoLoader.php
@@ -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",
 

diff --git a/tests/phpunit/ResourceLoaderTestCase.php b/tests/phpunit/ResourceLoaderTestCase.php
index 96eedb4..dc5549b 100644 (file)
--- a/tests/phpunit/ResourceLoaderTestCase.php
+++ b/tests/phpunit/ResourceLoaderTestCase.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();
+       }
+}

diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php
new file mode 100644 (file)
index 0000000..50f88c8
--- /dev/null
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php
@@ -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,
+                       ),
+               );
+       }
+}