On 2014-06-01, 3:25 AM, Stephan Gambke wrote:
I'll not answer all your points. I'd just like to (again) question the need to keep all the names and identifiers of a skin identical whatever the cost. Maybe I am daft, everybody else seems to take this as some given great goal and nobody else is questioning it. As I see it it introduces all kinds of dependencies for no better reason than to not confuse some hypothetical user or admin. Decoupling has apparently gone out of fashion. And it is biting you already, you have or will have to special case and transform identifiers to somehow make it work introducing loads of bugs along the way. It adds more bad design to an already badly flawed skin system.
What kind of decoupling did you have in mind? This is way to abstract/ambiguous to write a response to.
I18n message names that are directly derived from some value in $wgValidSkinNames? What?
Yes, specifically the key.
When you set $wgValidSkinName for a skin: $wgValidSkinName['foobar'] = 'FooBar';
The value 'FooBar' is used to determine the SkinTemplate subclass to use, in this case SkinFooBar. (I once tried to make it possible to explicitly specify the class name instead of relying on the ancient "Prepend with 'Skin' behaviour" but that hasn't been merged) And the key 'foobar' is used: - In Skin::getSkinNameMessages to find the i18n key (in this case skinname-foobar) with the skin's name.
The $skinname you place in SkinFooBar (which is supposed to match the key in $wgValidSkinNames) is used: - In OutputPage::makeResourceLoaderLink to pass skin= to RL modules - Which is then used by ResourceLoaderSiteModule::getPages to determine the MediaWiki:{ucfirst($skinname)}.{js,css} to use. - Giving us MediaWiki:Foobar.css and MediaWiki:Foobar.js
And what's the value of lower-casing the useskin param? If some skin developer thought it was a great idea to use camel case there or upper case or whatever, why do you care? They will get fire from their users soon enough. I think it is a bad idea to convolute core code with special transformations just to protect the users from some skin designers bad decisions. It makes the core code slow and unmaintainable. And in the end it introduces exactly the kind of imprecision into a skin's specs that we are trying to avoid. Because now with all combinations of uc and lc being valid we really have created a case where nobody knows exactly what is supposed to go into this useskin param and why.
&useskin= is a user facing interface. I can't remember where in the discussion someone pointed out that – while most users user &useskin=monobook – some users have tried using &useskin=MonoBook and come asking for help. So the topic of making it case insensitive came up, then both work.
However the normal way of implementing case insensitive comparison is simply to compare two lower case strings. However in this situation &useskin= is not compared but rather used as an array key. Essentially (not exactly, but in summary similar to): $wgValidSkinNames[$useskin]
The normal way one would make $useskin in this situation case insensitive wold be, essentially: $wgValidSkinNames[strtolower($useskin)]
I was pointing out that – besides various other side effects – someone can't suggest we start doing '$wgValidSkinNames['FooBar'] = 'FooBar';` to eliminate the lowercase 'foobar' in favour of 'FooBar' if we want to make &useskin= case insensitive because we can't case insensitively get `$wgValidSkinNames['FooBar']` from a $useskin that is 'foobar' or 'FOOBAR'.
~Daniel Friesen (Dantman, Nadir-Seen-Fire) [http://danielfriesen.name/]