From a0947c9507065a83afe52b078f0f6d1c6163875e Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Fri, 25 Aug 2017 22:25:57 +0100
Subject: [PATCH 1/1] Skin: Make skins aware of their registered skin name

Remove the need for skin classes to have a hardcoded string as
skinname property value. This previously created the possibility
for the value to not match the skinname in the SkinFactory registry,
which creates confusing situations where message keys and load.php
urls are crafted with the internal skinname, but all other
handling (useskin, preferences, hooks, SkinFactory, ResourceLoader,
etc.) operate on the names in the registry.

We could enforce the matching by requiring a 1:1 relationship between
skinnames and Skin sub classes, but that is not backwards-compatible
with the 1:many map that wgValidSkinNames provides, and not compatible
SkinFactory either, which supports a factory function to return an
object. This makes a lot of sense and allows Skin-classees to be
re-used and composed with injection. If we do want to enforce 1:1,
we could validate it with a structure PHPUnit test, but instead this
change just uses the injected name from the constructor (passed by
ServiceWiring, previously unused).

The added unit test shows the new behaviour. Before this change,
getSkinName() on SkinFallback would always return 'fallback',
whereas now each instance of the class adheres to the registered
name (if it differs from the default).

Update the two direct uses of protected $skin->skinname to use
$skin->getSkinName() instead to enable sub-classes to optionally
implement an alternate source for the self-name (or to hardcode
it there as before).

Bug: T173546
Change-Id: I4383dcc3094da6e3c9ac12dc6c9311128db9db6e
---
 includes/skins/Skin.php                          | 16 +++++++++++++++-
 tests/phpunit/includes/skins/SkinFactoryTest.php | 12 ++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/includes/skins/Skin.php b/includes/skins/Skin.php
index e1d0034675..f92a66f2fe 100644
--- a/includes/skins/Skin.php
+++ b/includes/skins/Skin.php
@@ -34,7 +34,11 @@ use MediaWiki\MediaWikiServices;
  * @ingroup Skins
  */
 abstract class Skin extends ContextSource {
+	/**
+	 * @var string|null
+	 */
 	protected $skinname = null;
+
 	protected $mRelevantTitle = null;
 	protected $mRelevantUser = null;
 
@@ -134,7 +138,17 @@ abstract class Skin extends ContextSource {
 	}
 
 	/**
-	 * @return string Skin name
+	 * @since 1.31
+	 * @param string|null $skinname
+	 */
+	public function __construct( $skinname = null ) {
+		if ( is_string( $skinname ) ) {
+			$this->skinname = $skinname;
+		}
+	}
+
+	/**
+	 * @return string|null Skin name
 	 */
 	public function getSkinName() {
 		return $this->skinname;
diff --git a/tests/phpunit/includes/skins/SkinFactoryTest.php b/tests/phpunit/includes/skins/SkinFactoryTest.php
index d3663c84ad..a8b9fd1ba0 100644
--- a/tests/phpunit/includes/skins/SkinFactoryTest.php
+++ b/tests/phpunit/includes/skins/SkinFactoryTest.php
@@ -48,6 +48,18 @@ class SkinFactoryTest extends MediaWikiTestCase {
 		$skin = $factory->makeSkin( 'testfallback' );
 		$this->assertInstanceOf( 'Skin', $skin );
 		$this->assertInstanceOf( 'SkinFallback', $skin );
+		$this->assertEquals( 'fallback', $skin->getSkinName() );
+	}
+
+	/**
+	 * @covers Skin::__constructor
+	 * @covers Skin::getSkinName
+	 */
+	public function testGetSkinName() {
+		$skin = new SkinFallback();
+		$this->assertEquals( 'fallback', $skin->getSkinName(), 'Default' );
+		$skin = new SkinFallback( 'testname' );
+		$this->assertEquals( 'testname', $skin->getSkinName(), 'Constructor argument' );
 	}
 
 	/**
-- 
2.20.1