From 0be2e83ef4bf892d69dfd8e57bee4e4c14cfb7e8 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Thu, 18 Mar 2021 17:22:37 +0000
Subject: [PATCH] resourceloader: Restore feedback for SkinModule invalid
 argument

This was added in 381499a675 in response to the issue that when a skin
adopts a feature that was new in MW 1.36, it caused an exception when
that version of the skin is then installed on MW 1.35. That's expected,
and generally helpful because the default support policy requires
like-for-like (MW 1.35 with REL1_35, this applies to all skins and
extensions, except for a few like Translate and SemanticMediaWiki).

However, while supporting LTS within master is unusual, there are
indeed the Translate/SemanticMediaWiki projects, and this problem has
already been solved years ago there by using a conditional with
$wgVersion. The proposed approach on T271441 actually also involves
such a conditional, but late at runtime instead of in the module
definition. Anyway, the way it was fixed in Timeless already solves
the problem by not passing down the invalid feature in the first
place, thus making the 381499a675 patch obsolete. I'm not aware of
any skin in Gerrit having such a compat policy, but if they exist,
they can fix it the same way, by defining the module in code with
a condition, just as Translate/SemanticMediaWiki do.

Bug: T271441
Change-Id: If97b3d04ac1e3355ee4f229db11e524d24ebd27a
---
 .../ResourceLoaderSkinModule.php              |  5 ++-
 .../ResourceLoaderSkinModuleTest.php          | 34 +++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/includes/resourceloader/ResourceLoaderSkinModule.php b/includes/resourceloader/ResourceLoaderSkinModule.php
index dd831114424..de22650c832 100644
--- a/includes/resourceloader/ResourceLoaderSkinModule.php
+++ b/includes/resourceloader/ResourceLoaderSkinModule.php
@@ -246,9 +246,12 @@ class ResourceLoaderSkinModule extends ResourceLoaderLessVarFileModule {
 
 		$listMode = false;
 		foreach ( $features as $key => $enabled ) {
-			// TODO: Restore feature key validation (T271441)
 			if ( is_string( $enabled ) ) {
 				$listMode = true;
+				$key = $enabled;
+			}
+			if ( !isset( self::FEATURE_FILES[$key] ) ) {
+				throw new InvalidArgumentException( "Feature '$key' is not recognised" );
 			}
 		}
 
diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderSkinModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderSkinModuleTest.php
index 1f73fd8ed8b..00e3925f7ec 100644
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderSkinModuleTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderSkinModuleTest.php
@@ -579,4 +579,38 @@ CSS
 			array_values( $actual )
 		);
 	}
+
+	public static function provideInvalidFeatures() {
+		yield 'listed unknown' => [
+			[ 'logo', 'unknown' ],
+		];
+
+		yield 'enabled unknown' => [
+			[
+				'logo' => true,
+				'toc' => false,
+				'unknown' => true,
+			],
+		];
+
+		yield 'disbled unknown' => [
+			[
+				'logo' => true,
+				'toc' => false,
+				'unknown' => false,
+			],
+		];
+	}
+
+	/**
+	 * @covers ResourceLoaderSkinModule
+	 * @dataProvider provideInvalidFeatures
+	 */
+	public function testConstructInvalidFeatures( array $features ) {
+		$this->expectException( InvalidArgumentException::class );
+		$this->expectExceptionMessage( "Feature 'unknown' is not recognised" );
+		$module = new ResourceLoaderSkinModule( [
+			'features' => $features,
+		] );
+	}
 }