From 92d6be8a38ff78ea06b06d9ba5a39d00de4c9f38 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Sat, 16 Feb 2019 23:30:46 +0000
Subject: [PATCH] resourceloader: Require $context parameter for
 FileModule::readStyleFiles()

Deprecated since MW 1.27.

Also update ResourcesTest to use TestingAccessWrapper instead of long-form
object reflection, and also apply it to its call for this method given
its meant to be private.

Change-Id: I9cc1af93730f632e4f8bf3a16d514a51ee73cb03
---
 RELEASE-NOTES-1.33                            |  2 ++
 .../resourceloader/ResourceLoaderContext.php  |  1 -
 .../ResourceLoaderFileModule.php              | 17 +++---------
 tests/phpunit/structure/ResourcesTest.php     | 26 +++++--------------
 4 files changed, 13 insertions(+), 33 deletions(-)

diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33
index cacfa51965..e545af8557 100644
--- a/RELEASE-NOTES-1.33
+++ b/RELEASE-NOTES-1.33
@@ -247,6 +247,8 @@ because of Phabricator reports.
 * The mw.user.stickyRandomId() method, deprecated in 1.32, was removed.
   Use mw.user.getPageviewToken() instead.
 * Removed deprecated class property WikiRevision::$importer.
+* ResourceLoaderFileModule::readStyleFiles() now requires its $context
+  parameter.
 
 === Deprecations in 1.33 ===
 * The configuration option $wgUseESI has been deprecated, and is expected
diff --git a/includes/resourceloader/ResourceLoaderContext.php b/includes/resourceloader/ResourceLoaderContext.php
index d6573afc46..67de1921b2 100644
--- a/includes/resourceloader/ResourceLoaderContext.php
+++ b/includes/resourceloader/ResourceLoaderContext.php
@@ -136,7 +136,6 @@ class ResourceLoaderContext implements MessageLocalizer {
 	 *
 	 * Use cases:
 	 * - Creating html5shiv script tag in OutputPage.
-	 * - FileModule::readStyleFiles (deprecated, to be removed).
 	 * - Unit tests (deprecated, create empty instance directly or use RLTestCase).
 	 *
 	 * @return ResourceLoaderContext
diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php
index 0e53e5e731..4444b1357e 100644
--- a/includes/resourceloader/ResourceLoaderFileModule.php
+++ b/includes/resourceloader/ResourceLoaderFileModule.php
@@ -878,25 +878,16 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
 	/**
 	 * Get the contents of a list of CSS files.
 	 *
-	 * This is considered a private method. Exposed for internal use by WebInstallerOutput.
-	 *
-	 * @private
+	 * @internal This is considered a private method. Exposed for internal use by WebInstallerOutput.
 	 * @param array $styles Map of media type to file paths to read, remap, and concatenate
 	 * @param bool $flip
-	 * @param ResourceLoaderContext|null $context
+	 * @param ResourceLoaderContext $context
 	 * @return array List of concatenated and remapped CSS data from $styles,
 	 *     keyed by media type
 	 * @throws MWException
-	 * @since 1.27 Calling this method without a ResourceLoaderContext instance
-	 *   is deprecated.
 	 */
-	public function readStyleFiles( array $styles, $flip, $context = null ) {
-		if ( $context === null ) {
-			wfDeprecated( __METHOD__ . ' without a ResourceLoader context', '1.27' );
-			$context = ResourceLoaderContext::newDummyContext();
-		}
-
-		if ( empty( $styles ) ) {
+	public function readStyleFiles( array $styles, $flip, $context ) {
+		if ( !$styles ) {
 			return [];
 		}
 		foreach ( $styles as $media => $files ) {
diff --git a/tests/phpunit/structure/ResourcesTest.php b/tests/phpunit/structure/ResourcesTest.php
index e08ffd77eb..776dee1b4f 100644
--- a/tests/phpunit/structure/ResourcesTest.php
+++ b/tests/phpunit/structure/ResourcesTest.php
@@ -1,6 +1,7 @@
 <?php
 
 use MediaWiki\MediaWikiServices;
+use Wikimedia\TestingAccessWrapper;
 
 /**
  * Sanity checks for making sure registered resources are sane.
@@ -245,9 +246,6 @@ class ResourcesTest extends MediaWikiTestCase {
 	/**
 	 * Get all resource files from modules that are an instance of
 	 * ResourceLoaderFileModule (or one of its subclasses).
-	 *
-	 * Since the raw data is stored in protected properties, we have to
-	 * overrride this through ReflectionObject methods.
 	 */
 	public static function provideResourceFiles() {
 		$data = self::getAllModules();
@@ -275,14 +273,12 @@ class ResourcesTest extends MediaWikiTestCase {
 				continue;
 			}
 
-			$reflectedModule = new ReflectionObject( $module );
+			$moduleProxy = TestingAccessWrapper::newFromObject( $module );
 
 			$files = [];
 
 			foreach ( $filePathProps['lists'] as $propName ) {
-				$property = $reflectedModule->getProperty( $propName );
-				$property->setAccessible( true );
-				$list = $property->getValue( $module );
+				$list = $moduleProxy->$propName;
 				foreach ( $list as $key => $value ) {
 					// 'scripts' are numeral arrays.
 					// 'styles' can be numeral or associative.
@@ -297,9 +293,7 @@ class ResourcesTest extends MediaWikiTestCase {
 			}
 
 			foreach ( $filePathProps['nested-lists'] as $propName ) {
-				$property = $reflectedModule->getProperty( $propName );
-				$property->setAccessible( true );
-				$lists = $property->getValue( $module );
+				$lists = $moduleProxy->$propName;
 				foreach ( $lists as $list ) {
 					foreach ( $list as $key => $value ) {
 						// We need the same filter as for 'lists',
@@ -313,29 +307,23 @@ class ResourcesTest extends MediaWikiTestCase {
 				}
 			}
 
-			// Get method for resolving the paths to full paths
-			$method = $reflectedModule->getMethod( 'getLocalPath' );
-			$method->setAccessible( true );
-
 			// Populate cases
 			foreach ( $files as $file ) {
 				$cases[] = [
-					$method->invoke( $module, $file ),
+					$moduleProxy->getLocalPath( $file ),
 					$moduleName,
 					( $file instanceof ResourceLoaderFilePath ? $file->getPath() : $file ),
 				];
 			}
 
 			// To populate missingLocalFileRefs. Not sure how sane this is inside this test...
-			$module->readStyleFiles(
+			$moduleProxy->readStyleFiles(
 				$module->getStyleFiles( $data['context'] ),
 				$module->getFlip( $data['context'] ),
 				$data['context']
 			);
 
-			$property = $reflectedModule->getProperty( 'missingLocalFileRefs' );
-			$property->setAccessible( true );
-			$missingLocalFileRefs = $property->getValue( $module );
+			$missingLocalFileRefs = $moduleProxy->missingLocalFileRefs;
 
 			foreach ( $missingLocalFileRefs as $file ) {
 				$cases[] = [
-- 
2.20.1