r79017 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r79016‎ | r79017 | r79018 >
Date:14:15, 26 December 2010
Author:dantman
Status:ok
Tags:
Comment:
Changing the skin loader to load classes using the pattern "Skin{$skinName}" instead of `"Skin" . ucfirst($key)` as the current behavior has been causing bugs with extension based skins attempting to use the autoloader to load their skins.
Under current behavior for "MonoBook" the skin loader will load "SkinMonobook" instead of "SkinMonoBook". Because the skin system loads from skins/ when it can't find a skin and php's class system is case insensitive by a fluke MonoBook has been fine despite the skin loader trying to load SkinMonobook despite the class being named SkinMonoBook.
However our autoloader is case-sensitive, and as a result if you try to name your extension based skin class something like SkinStereoBook the skin will break with a cryptic error message because the skin loader attempts to load SkinStereobook, doesn't find it in the autoloader, then throws a php error when it tries to load skins/StereoBook.php and can't find the file.
We have also been using the $skinName to generate the name of the file to load from skins/ so this value is already expected to be safe for use in this way.
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/Skin.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/DefaultSettings.php
——@@ -4460,7 +4460,10 @@
44614461
44624462 /**
44634463 * List of valid skin names.
4464 - * The key should be the name in all lower case, the value should be a display name.
 4464+ * The key should be the name in all lower case, the value should be a properly
 4465+ * cased name for the skin. This value will be prefixed with "Skin" to create the
 4466+ * class name of the skin to load, and if the skin's class cannot be found through
 4467+ * the autoloader it will be used to load a .php file by that name in the skins directory.
44654468 * The default skins will be added later, by Skin::getSkinNames(). Use
44664469 * Skin::getSkinNames() as an accessor if you wish to have access to the full list.
44674470 */
Index: trunk/phase3/includes/Skin.php
——@@ -142,7 +142,7 @@
143143
144144 $skinNames = Skin::getSkinNames();
145145 $skinName = $skinNames[$key];
146 - $className = 'Skin' . ucfirst( $key );
 146+ $className = "Skin{$skinName}";
147147
148148 # Grab the skin class and initialise it.
149149 if ( !class_exists( $className ) ) {
Index: trunk/phase3/RELEASE-NOTES
——@@ -23,6 +23,9 @@
2424 WantedPages::getQueryInfo . This may break older extensions.
2525 * $wgUseCombinedLoginLink controls whether to output a combined login / create account
2626 link in the personal bar, or to output separate login and create account links
 27+* Skin names are no longer created based on a ucfirst version of the key in $wgValidSkinNames but now
 28+ the value. This means for $wgValidSkinNames["monobook"] = "MonoBook"; the skin
 29+ loader will no longer try loading SkinMonobook and will instead load SkinMonoBook.
2730
2831 === New features in 1.18 ===
2932 * Added a special page, disabled by default, that allows users with the

Status & tagging log