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.