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

diff --git a/includes/skins/Skin.php b/includes/skins/Skin.php
index e1d0034..f92a66f 100644 (file)
--- a/includes/skins/Skin.php
+++ b/includes/skins/Skin.php
@@ -34,7 +34,11 @@ use MediaWiki\MediaWikiServices;
  * @ingroup Skins
  */
 abstract class Skin extends ContextSource {
  * @ingroup Skins
  */
 abstract class Skin extends ContextSource {

+       /**
+        * @var string|null
+        */

        protected $skinname = null;
        protected $skinname = null;

+

        protected $mRelevantTitle = null;
        protected $mRelevantUser = 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;
         */
        public function getSkinName() {
                return $this->skinname;

diff --git a/tests/phpunit/includes/skins/SkinFactoryTest.php b/tests/phpunit/includes/skins/SkinFactoryTest.php
index d3663c8..a8b9fd1 100644 (file)
--- 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 );
                $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' );

        }
 
        /**
        }
 
        /**