I ran into an issue while experimenting with something today.
I created a voidbook skin with a SkinVoidBook class which was going to
be a test skin to duplicate monobook in a compiled template language.
But I did it using the extension technique of setting $wgValidSkinNames
and $wgAutoloadClasses like any other 3rd party skin.
I ran into an issue with the skin not being loaded.
After I debugged it I found out that for several years our skin system
has been doing something utterly screwed up...
Here's how Skin::newFromKey works with monobook.
With $key = "monobook" passed to it the method Skin::getSkinNames() to
get the fully filled $wgValidSkinNames data.
$skinName becomes "MonoBook" and $className becomes "SkinMonobook"
`$className = 'Skin' . ucfirst( $key );`
The method does a class_exists triggering the autoloader checking for a
"SkinMonobook" class.
SkinMonobook is not found so the method loads up skins/MonoBook.php
after optionally loading skins/MonoBook.deps.php, this loads the
SkinMonoBook class.
class_exists returns true, and so `new $className` is called... This
creates an instance of SkinMonoBook from the $className SkinMonobook.
See the screw up there? For a long time we've been sticking to naming
our skin classes by the SkinMonoBook convention... while really, the
skin system has been trying to load SkinMonobook and only succeeding
because we use require_once directly loading SkinMonoBook and php's
class system happens to be case insensitive so when we ask for
SkinMonobook it gives us SkinMonoBook.
Problem! Our autoloader is NOT case insensitive.
So anyone following our internal conventions while trying to create a
skin inside of an extension and they happen to decide on a skin name
using a capital letter in the middle of the word gets a nasty suprise.
Instead of the way it played out with MonoBook, VoidBook gets this result.
$key = "voidbook"; Skin::newFromKey sets $skinName = "VoidBook";
$className = "SkinVoidbook";
class_name calls our autoloader asking for "SkinVoidbook", the class is
actually SkinVoidBook so our autoloader does NOT load the class. The
class_exists returns false, Skin::newFromKey continues along, doesn't
see skins/SkinVoidBook.deps.php so it skips it... then it tires to
require skins/SkinVoidBook.php, now because this is an extension based
skin it trips up and we get a cryptic fatal php error.
Now we've defined $wgValidSkinNames as an array mapping skin ids to
names of those skins... however from what I see convention violates this
notion. "cologne" is "CologneBlue" yet the skin's actual name is
"Cologne Blue". "standard" is "Standard" yet the skin's
actual name is
"Classic".
Despite the array being documented as a list of skin names, it really
appears to conform to something that maps lower case skin ids to their
proper cased counterparts which when prefixed with "Skin" will give you
a skin's class name. While we use the i18n system for the real skin "name".
This is actually fairly well supported by the fact that we use that skin
"name" when we require a skin file from the skins folder.
So to fix this bug I propose we change the documented format of
$wgValidSkinNames to be an array mapping skin ids "monobook" to the
proper cased key used for building class names and requiring files. And
change `$className = 'Skin' . ucfirst( $key );` to `$className =
"Skin{$skinName}";` so that "monobook" will try to load SkinMonoBook
instead of SkinMonobook.
As a side effect of this, it will also theoretically become possible to
alias skins by doing something like `$wgValidSkinNames = "MonoBook";`
which considering there are already cases in the wild where people have
duplicated monobook to have varying styles like ReferenceBook,
WhiteBook, etc... would probably be a desirable feature.
--
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [
http://daniel.friesen.name]