From: Bartosz Dziewoński <matma.rex@gmail.com>
Date: Thu, 3 Sep 2015 21:30:36 +0000 (+0200)
Subject: ResourcesTest: Detect missing files in url(...) expressions
X-Git-Tag: 1.31.0-rc.0~9985^2
X-Git-Url: https://git.heureux-cyclage.org/?p=lhc%2Fweb%2Fwiklou.git;a=commitdiff_plain;h=8f5cd11d82fa5d58ab644b55b8f28e82879465a0;ds=sidebyside

ResourcesTest: Detect missing files in url(...) expressions

The way this is implemented is really dirty... but it found us
a few pre-existing bugs already (T111518, T111519, T111771).
I think it might be worth it.

* CSSMin: Add new method getAllLocalFileReferences() which skips the
  file_exists() check.
* ResourceLoaderModule: Make use of it to track missing files too.
* ResourcesTest: Verify that the missing files are missing.

Change-Id: I5a3cdeb7d53485f161ccf8133e76850cdf5b4579
---

diff --git a/includes/libs/CSSMin.php b/includes/libs/CSSMin.php
index 2c904319c3..26489214a0 100644
--- a/includes/libs/CSSMin.php
+++ b/includes/libs/CSSMin.php
@@ -60,13 +60,15 @@ class CSSMin {
 	/* Static Methods */
 
 	/**
-	 * Gets a list of local file paths which are referenced in a CSS style sheet
+	 * Gets a list of local file paths which are referenced in a CSS style sheet.
 	 *
-	 * This function will always return an empty array if the second parameter is not given or null
-	 * for backwards-compatibility.
+	 * If you wish non-existent files to be listed too, use getAllLocalFileReferences().
 	 *
-	 * @param string $source CSS data to remap
-	 * @param string $path File path where the source was read from (optional)
+	 * For backwards-compatibility, if the second parameter is not given or null,
+	 * this function will return an empty array instead of erroring out.
+	 *
+	 * @param string $source CSS stylesheet source to process
+	 * @param string $path File path where the source was read from
 	 * @return array List of local file references
 	 */
 	public static function getLocalFileReferences( $source, $path = null ) {
@@ -74,6 +76,25 @@ class CSSMin {
 			return array();
 		}
 
+		$files = self::getAllLocalFileReferences( $source, $path );
+
+		// Skip non-existent files
+		$files = array_filter( $files, function ( $file ) {
+			return file_exists( $file );
+		} );
+
+		return $files;
+	}
+
+	/**
+	 * Gets a list of local file paths which are referenced in a CSS style sheet, including
+	 * non-existent files.
+	 *
+	 * @param string $source CSS stylesheet source to process
+	 * @param string $path File path where the source wa
+	 * @return array List of local file references
+	 */
+	public static function getAllLocalFileReferences( $source, $path ) {
 		$path = rtrim( $path, '/' ) . '/';
 		$files = array();
 
@@ -87,13 +108,7 @@ class CSSMin {
 					break;
 				}
 
-				$file = $path . $url;
-				// Skip non-existent files
-				if ( file_exists( $file ) ) {
-					break;
-				}
-
-				$files[] = $file;
+				$files[] = $path . $url;
 			}
 		}
 		return $files;
diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php
index 4a4a828527..112ebc0052 100644
--- a/includes/resourceloader/ResourceLoaderFileModule.php
+++ b/includes/resourceloader/ResourceLoaderFileModule.php
@@ -152,6 +152,12 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 	 */
 	protected $localFileRefs = array();
 
+	/**
+	 * @var array Place where readStyleFile() tracks file dependencies for non-existent files.
+	 * Used in tests to detect missing dependencies.
+	 */
+	protected $missingLocalFileRefs = array();
+
 	/* Methods */
 
 	/**
@@ -917,10 +923,14 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 		$localDir = dirname( $localPath );
 		$remoteDir = dirname( $remotePath );
 		// Get and register local file references
-		$this->localFileRefs = array_merge(
-			$this->localFileRefs,
-			CSSMin::getLocalFileReferences( $style, $localDir )
-		);
+		$localFileRefs = CSSMin::getAllLocalFileReferences( $style, $localDir );
+		foreach ( $localFileRefs as $file ) {
+			if ( file_exists( $file ) ) {
+				$this->localFileRefs[] = $file;
+			} else {
+				$this->missingLocalFileRefs[] = $file;
+			}
+		}
 		return CSSMin::remap(
 			$style, $localDir, $remoteDir, true
 		);
diff --git a/tests/phpunit/structure/ResourcesTest.php b/tests/phpunit/structure/ResourcesTest.php
index 2c92e305b6..69f452503d 100644
--- a/tests/phpunit/structure/ResourcesTest.php
+++ b/tests/phpunit/structure/ResourcesTest.php
@@ -276,6 +276,25 @@ class ResourcesTest extends MediaWikiTestCase {
 					( $file instanceof ResourceLoaderFilePath ? $file->getPath() : $file ),
 				);
 			}
+
+			// To populate missingLocalFileRefs. Not sure how sane this is inside this test...
+			$module->readStyleFiles(
+				$module->getStyleFiles( $data['context'] ),
+				$module->getFlip( $data['context'] ),
+				$data['context']
+			);
+
+			$property = $reflectedModule->getProperty( 'missingLocalFileRefs' );
+			$property->setAccessible( true );
+			$missingLocalFileRefs = $property->getValue( $module );
+
+			foreach ( $missingLocalFileRefs as $file ) {
+				$cases[] = array(
+					$file,
+					$moduleName,
+					$file,
+				);
+			}
 		}
 
 		return $cases;