From: Kunal Mehta <legoktm@gmail.com>
Date: Fri, 29 Aug 2014 06:31:44 +0000 (-0700)
Subject: Check page_len in ResourceLoaderWikiModule::isKnownEmpty() for 'user' modules
X-Git-Tag: 1.31.0-rc.0~14220
X-Git-Url: http://git.cyclocoop.org/%24self?a=commitdiff_plain;h=8968d8787fee47739649ccb2e32e883cdd2b0f4d;p=lhc%2Fweb%2Fwiklou.git

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 58b7fa968c..d45316fd2d 100644
--- 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 0078d7e2c7..2e8fed446e 100644
--- 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 96eedb4940..dc5549bddd 100644
--- 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
index 0000000000..50f88c825f
--- /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,
+			),
+		);
+	}
+}