From eb37e9d1ffb2ebf3e847ae948b8f30455b1f3176 Mon Sep 17 00:00:00 2001 From: Kunal Mehta Date: Sat, 9 Aug 2014 13:36:35 +0100 Subject: [PATCH] Introduce SkinFactory Modeled similar to ConfigFactory, this lets skins register themselves via a callback, allowing for proper dependency injection. Loading via $wgValidSkinNames is still supported, but considered "legacy", not deprecated though. Skin::newFromKey is now deprecated (and had only one caller in an extension, which I'll update afterwards). Change-Id: I1960483f87c2ef55c994545239b728fa376f17f4 --- includes/AutoLoader.php | 18 +- includes/context/RequestContext.php | 16 +- includes/{ => skins}/Skin.php | 87 +------- includes/skins/SkinException.php | 29 +++ includes/skins/SkinFactory.php | 202 ++++++++++++++++++ includes/{ => skins}/SkinFallback.php | 0 includes/{ => skins}/SkinFallbackTemplate.php | 0 includes/{ => skins}/SkinTemplate.php | 0 tests/phpunit/includes/OutputPageTest.php | 2 +- .../includes/skins/SkinFactoryTest.php | 38 ++++ .../includes/{ => skins}/SkinTemplateTest.php | 0 11 files changed, 305 insertions(+), 87 deletions(-) rename includes/{ => skins}/Skin.php (93%) create mode 100644 includes/skins/SkinException.php create mode 100644 includes/skins/SkinFactory.php rename includes/{ => skins}/SkinFallback.php (100%) rename includes/{ => skins}/SkinFallbackTemplate.php (100%) rename includes/{ => skins}/SkinTemplate.php (100%) create mode 100644 tests/phpunit/includes/skins/SkinFactoryTest.php rename tests/phpunit/includes/{ => skins}/SkinTemplateTest.php (100%) diff --git a/includes/AutoLoader.php b/includes/AutoLoader.php index 872e89b7cc..c535ca0c30 100644 --- a/includes/AutoLoader.php +++ b/includes/AutoLoader.php @@ -36,7 +36,6 @@ $wgAutoloadLocalClasses = array( 'AuthPlugin' => 'includes/AuthPlugin.php', 'AuthPluginUser' => 'includes/AuthPlugin.php', 'Autopromote' => 'includes/Autopromote.php', - 'BaseTemplate' => 'includes/SkinTemplate.php', 'Block' => 'includes/Block.php', 'Category' => 'includes/Category.php', 'Categoryfinder' => 'includes/Categoryfinder.php', @@ -120,7 +119,6 @@ $wgAutoloadLocalClasses = array( 'MagicWordArray' => 'includes/MagicWord.php', 'MailAddress' => 'includes/UserMailer.php', 'MediaWiki' => 'includes/MediaWiki.php', - 'MediaWikiI18N' => 'includes/SkinTemplate.php', 'MediaWikiVersionFetcher' => 'includes/MediaWikiVersionFetcher.php', 'Message' => 'includes/Message.php', 'MessageBlobStore' => 'includes/MessageBlobStore.php', @@ -143,7 +141,6 @@ $wgAutoloadLocalClasses = array( 'PreferencesForm' => 'includes/Preferences.php', 'PrefixSearch' => 'includes/PrefixSearch.php', 'ProtectionForm' => 'includes/ProtectionForm.php', - 'QuickTemplate' => 'includes/SkinTemplate.php', 'RawMessage' => 'includes/Message.php', 'ReverseChronologicalPager' => 'includes/Pager.php', 'RevisionItem' => 'includes/RevisionList.php', @@ -156,10 +153,6 @@ $wgAutoloadLocalClasses = array( 'SiteConfiguration' => 'includes/SiteConfiguration.php', 'SiteStats' => 'includes/SiteStats.php', 'SiteStatsInit' => 'includes/SiteStats.php', - 'Skin' => 'includes/Skin.php', - 'SkinTemplate' => 'includes/SkinTemplate.php', - 'SkinFallback' => 'includes/SkinFallback.php', - 'SkinFallbackTemplate' => 'includes/SkinFallbackTemplate.php', 'SquidPurgeClient' => 'includes/SquidPurgeClient.php', 'SquidPurgeClientPool' => 'includes/SquidPurgeClient.php', 'StatCounter' => 'includes/StatCounter.php', @@ -933,6 +926,17 @@ $wgAutoloadLocalClasses = array( 'Sites' => 'includes/site/SiteSQLStore.php', 'SiteStore' => 'includes/site/SiteStore.php', + # includes/skins + 'BaseTemplate' => 'includes/skins/SkinTemplate.php', + 'MediaWikiI18N' => 'includes/skins/SkinTemplate.php', + 'QuickTemplate' => 'includes/skins/SkinTemplate.php', + 'Skin' => 'includes/skins/Skin.php', + 'SkinException' => 'includes/skins/SkinException.php', + 'SkinFactory' => 'includes/skins/SkinFactory.php', + 'SkinFallback' => 'includes/skins/SkinFallback.php', + 'SkinFallbackTemplate' => 'includes/skins/SkinFallbackTemplate.php', + 'SkinTemplate' => 'includes/skins/SkinTemplate.php', + # includes/specialpage 'ChangesListSpecialPage' => 'includes/specialpage/ChangesListSpecialPage.php', 'FormSpecialPage' => 'includes/specialpage/FormSpecialPage.php', diff --git a/includes/context/RequestContext.php b/includes/context/RequestContext.php index b847163086..8ab1122e83 100644 --- a/includes/context/RequestContext.php +++ b/includes/context/RequestContext.php @@ -352,12 +352,19 @@ class RequestContext implements IContextSource { $skin = null; wfRunHooks( 'RequestContextCreateSkin', array( $this, &$skin ) ); + $fallback = $this->getConfig()->get( 'FallbackSkin' ); + $factory = SkinFactory::getDefaultInstance(); // If the hook worked try to set a skin from it if ( $skin instanceof Skin ) { $this->skin = $skin; } elseif ( is_string( $skin ) ) { - $this->skin = Skin::newFromKey( $skin ); + try { + $this->skin = $factory->makeSkin( $skin ); + } catch ( SkinException $e ) { + $this->skin = $factory->makeSkin( $fallback ); + } + } // If this is still null (the hook didn't run or didn't work) @@ -372,7 +379,12 @@ class RequestContext implements IContextSource { $userSkin = $this->getConfig()->get( 'DefaultSkin' ); } - $this->skin = Skin::newFromKey( $userSkin ); + try { + $this->skin = $factory->makeSkin( $userSkin ); + } catch ( SkinException $e ) { + $this->skin = $factory->makeSkin( $fallback ); + } + } // After all that set a context on whatever skin got created diff --git a/includes/Skin.php b/includes/skins/Skin.php similarity index 93% rename from includes/Skin.php rename to includes/skins/Skin.php index a59d5678a3..6ba1efd992 100644 --- a/includes/Skin.php +++ b/includes/skins/Skin.php @@ -47,60 +47,7 @@ abstract class Skin extends ContextSource { * @return array Associative array of strings */ static function getSkinNames() { - global $wgValidSkinNames; - static $skinsInitialised = false; - - if ( !$skinsInitialised || !count( $wgValidSkinNames ) ) { - # Get a list of available skins - # Build using the regular expression '^(.*).php$' - # Array keys are all lower case, array value keep the case used by filename - # - wfProfileIn( __METHOD__ . '-init' ); - - global $wgStyleDirectory; - - $skinDir = dir( $wgStyleDirectory ); - - if ( $skinDir !== false && $skinDir !== null ) { - # while code from www.php.net - while ( false !== ( $file = $skinDir->read() ) ) { - // Skip non-PHP files, hidden files, and '.dep' includes - $matches = array(); - - if ( preg_match( '/^([^.]*)\.php$/', $file, $matches ) ) { - $aSkin = $matches[1]; - - // Explicitly disallow loading core skins via the autodiscovery mechanism. - // - // They should be loaded already (in a non-autodicovery way), but old files might still - // exist on the server because our MW version upgrade process is widely documented as - // requiring just copying over all files, without removing old ones. - // - // This is one of the reasons we should have never used autodiscovery in the first - // place. This hack can be safely removed when autodiscovery is gone. - if ( in_array( $aSkin, array( 'CologneBlue', 'Modern', 'MonoBook', 'Vector' ) ) ) { - wfLogWarning( - "An old copy of the $aSkin skin was found in your skins/ directory. " . - "You should remove it to avoid problems in the future." . - "See https://www.mediawiki.org/wiki/Manual:Skin_autodiscovery for details." - ); - continue; - } - - wfLogWarning( - "A skin using autodiscovery mechanism, $aSkin, was found in your skins/ directory. " . - "The mechanism will be removed in MediaWiki 1.25 and the skin will no longer be recognized. " . - "See https://www.mediawiki.org/wiki/Manual:Skin_autodiscovery for information how to fix this." - ); - $wgValidSkinNames[strtolower( $aSkin )] = $aSkin; - } - } - $skinDir->close(); - } - $skinsInitialised = true; - wfProfileOut( __METHOD__ . '-init' ); - } - return $wgValidSkinNames; + return SkinFactory::getDefaultInstance()->getSkinNames(); } /** @@ -197,34 +144,20 @@ abstract class Skin extends ContextSource { * Factory method for loading a skin of a given type * @param string $key 'monobook', 'vector', etc. * @return Skin + * @deprecated Use SkinFactory instead */ static function &newFromKey( $key ) { - global $wgStyleDirectory, $wgFallbackSkin; + wfDeprecated( __METHOD__, '1.24' ); + global $wgFallbackSkin; $key = Skin::normalizeKey( $key ); - - $skinNames = Skin::getSkinNames(); - $skinName = $skinNames[$key]; - $className = "Skin{$skinName}"; - - # Grab the skin class and initialise it. - if ( !class_exists( $className ) ) { - - require_once "{$wgStyleDirectory}/{$skinName}.php"; - - # Check if we got if not fallback to default skin - if ( !class_exists( $className ) ) { - # DO NOT die if the class isn't found. This breaks maintenance - # scripts and can cause a user account to be unrecoverable - # except by SQL manipulation if a previously valid skin name - # is no longer valid. - wfDebug( "Skin class does not exist: $className\n" ); - - $fallback = $skinNames[Skin::normalizeKey( $wgFallbackSkin )]; - $className = "Skin{$fallback}"; - } + $factory = SkinFactory::getDefaultInstance(); + try { + $skin = $factory->makeSkin( $key ); + } catch ( SkinException $e ) { + $skin = $factory->makeSkin( $wgFallbackSkin ); } - $skin = new $className( $key ); + return $skin; } diff --git a/includes/skins/SkinException.php b/includes/skins/SkinException.php new file mode 100644 index 0000000000..31ff1437e8 --- /dev/null +++ b/includes/skins/SkinException.php @@ -0,0 +1,29 @@ + callback + * @var array + */ + private $factoryFunctions = array(); + /** + * Map of name => human readable name + * @var array + */ + private $displayNames = array(); + + /** + * @var SkinFactory + */ + private static $self; + + public static function getDefaultInstance() { + if ( !self::$self ) { + self::$self = new self; + } + + return self::$self; + } + + /** + * Register a new Skin factory function + * Will override if it's already registered + * @param string $name + * @param string $displayName + * @param callable $callback That takes the skin name as an argument + * @throws InvalidArgumentException If an invalid callback is provided + */ + public function register( $name, $displayName, $callback ) { + if ( !is_callable( $callback ) ) { + throw new InvalidArgumentException( 'Invalid callback provided' ); + } + $this->factoryFunctions[$name] = $callback; + $this->displayNames[$name] = $displayName; + } + + /** + * @return array + */ + private function getLegacySkinNames() { + global $wgValidSkinNames; + static $skinsInitialised = false; + + if ( !$skinsInitialised || !count( $wgValidSkinNames ) ) { + # Get a list of available skins + # Build using the regular expression '^(.*).php$' + # Array keys are all lower case, array value keep the case used by filename + # + wfProfileIn( __METHOD__ . '-init' ); + + global $wgStyleDirectory; + + $skinDir = dir( $wgStyleDirectory ); + + if ( $skinDir !== false && $skinDir !== null ) { + # while code from www.php.net + while ( false !== ( $file = $skinDir->read() ) ) { + // Skip non-PHP files, hidden files, and '.dep' includes + $matches = array(); + + if ( preg_match( '/^([^.]*)\.php$/', $file, $matches ) ) { + $aSkin = $matches[1]; + + // Explicitly disallow loading core skins via the autodiscovery mechanism. + // + // They should be loaded already (in a non-autodicovery way), but old files might still + // exist on the server because our MW version upgrade process is widely documented as + // requiring just copying over all files, without removing old ones. + // + // This is one of the reasons we should have never used autodiscovery in the first + // place. This hack can be safely removed when autodiscovery is gone. + if ( in_array( $aSkin, array( 'CologneBlue', 'Modern', 'MonoBook', 'Vector' ) ) ) { + wfLogWarning( + "An old copy of the $aSkin skin was found in your skins/ directory. " . + "You should remove it to avoid problems in the future." . + "See https://www.mediawiki.org/wiki/Manual:Skin_autodiscovery for details." + ); + continue; + } + + wfLogWarning( + "A skin using autodiscovery mechanism, $aSkin, was found in your skins/ directory. " . + "The mechanism will be removed in MediaWiki 1.25 and the skin will no longer be recognized. " . + "See https://www.mediawiki.org/wiki/Manual:Skin_autodiscovery for information how to fix this." + ); + $wgValidSkinNames[strtolower( $aSkin )] = $aSkin; + } + } + $skinDir->close(); + } + $skinsInitialised = true; + wfProfileOut( __METHOD__ . '-init' ); + } + return $wgValidSkinNames; + + } + + /** + * Returns an associative array of: + * skin name => human readable name + * + * @return array + */ + public function getSkinNames() { + return array_merge( + $this->getLegacySkinNames(), + $this->displayNames + ); + } + + /** + * Get a legacy skin which uses $wgValidSkinNames + * or autoloading + * + * @param string $name + * @return Skin|bool false if the skin couldn't be constructed + */ + private function getLegacySkin( $name ) { + $skinNames = $this->getLegacySkinNames(); + if ( !isset( $skinNames[$name] ) ) { + return false; + } + $skinName = $skinNames[$name]; + $className = "Skin{$skinName}"; + + # Grab the skin class and initialise it. + if ( !class_exists( $className ) ) { + global $wgStyleDirectory; + require_once "{$wgStyleDirectory}/{$skinName}.php"; + + # Check if we got it + if ( !class_exists( $className ) ) { + # DO NOT die if the class isn't found. This breaks maintenance + # scripts and can cause a user account to be unrecoverable + # except by SQL manipulation if a previously valid skin name + # is no longer valid. + return false; + } + } + $skin = new $className( $name ); + return $skin; + + } + + /** + * Create a given Skin using the registered callback for $name. + * @param string $name Name of the skin you want + * @throws SkinException If a factory function isn't registered for $name + * @throws UnexpectedValueException If the factory function returns a non-Skin object + * @return Skin + */ + public function makeSkin( $name ) { + if ( !isset( $this->factoryFunctions[$name] ) ) { + // Check the legacy method of skin loading + $legacy = $this->getLegacySkin( $name ); + if ( $legacy ) { + return $legacy; + } + throw new SkinException( "No registered builder available for $name." ); + } + $skin = call_user_func( $this->factoryFunctions[$name], $name ); + if ( $skin instanceof Skin ) { + return $skin; + } else { + throw new UnexpectedValueException( "The builder for $name returned a non-Skin object." ); + } + } +} diff --git a/includes/SkinFallback.php b/includes/skins/SkinFallback.php similarity index 100% rename from includes/SkinFallback.php rename to includes/skins/SkinFallback.php diff --git a/includes/SkinFallbackTemplate.php b/includes/skins/SkinFallbackTemplate.php similarity index 100% rename from includes/SkinFallbackTemplate.php rename to includes/skins/SkinFallbackTemplate.php diff --git a/includes/SkinTemplate.php b/includes/skins/SkinTemplate.php similarity index 100% rename from includes/SkinTemplate.php rename to includes/skins/SkinTemplate.php diff --git a/tests/phpunit/includes/OutputPageTest.php b/tests/phpunit/includes/OutputPageTest.php index e866386244..8532f5cd28 100644 --- a/tests/phpunit/includes/OutputPageTest.php +++ b/tests/phpunit/includes/OutputPageTest.php @@ -195,7 +195,7 @@ mw.loader.implement("test.quux",function($,jQuery){mw.test.baz({token:123});},{" $method = $class->getMethod( 'makeResourceLoaderLink' ); $method->setAccessible( true ); $ctx = new RequestContext(); - $ctx->setSkin( Skin::newFromKey( 'vector' ) ); + $ctx->setSkin( SkinFactory::getDefaultInstance()->makeSkin( 'fallback' ) ); $ctx->setLanguage( 'en' ); $out = new OutputPage( $ctx ); $rl = $out->getResourceLoader(); diff --git a/tests/phpunit/includes/skins/SkinFactoryTest.php b/tests/phpunit/includes/skins/SkinFactoryTest.php new file mode 100644 index 0000000000..4497cbabec --- /dev/null +++ b/tests/phpunit/includes/skins/SkinFactoryTest.php @@ -0,0 +1,38 @@ +register( 'fallback', 'Fallback', function() { + return new SkinFallback(); + } ); + $this->assertTrue( true ); // No exception thrown + $this->setExpectedException( 'InvalidArgumentException' ); + $factory->register( 'invalid', 'Invalid', 'Invalid callback' ); + } + + /** + * @covers SkinFactory::makeSkin + */ + public function testMakeSkinWithNoBuilders() { + $factory = new SkinFactory(); + $this->setExpectedException( 'SkinException' ); + $factory->makeSkin( 'nobuilderregistered' ); + } + + /** + * @covers SkinFactory::makeSkin + */ + public function testMakeSkinWithInvalidCallback() { + $factory = new SkinFactory(); + $factory->register( 'unittest', 'Unittest', function () { + return true; // Not a Skin object + } ); + $this->setExpectedException( 'UnexpectedValueException' ); + $factory->makeSkin( 'unittest' ); + } +} diff --git a/tests/phpunit/includes/SkinTemplateTest.php b/tests/phpunit/includes/skins/SkinTemplateTest.php similarity index 100% rename from tests/phpunit/includes/SkinTemplateTest.php rename to tests/phpunit/includes/skins/SkinTemplateTest.php -- 2.20.1