Page MenuHomePhabricator

javascript variable "skin" defined twice, breaks with &useskin=
Closed, ResolvedPublic

Description

Author: herd

Description:
In /includes/Skin.php in the gen=js, "skin" and "stylepath" are each defined twice. Once in the in-page <script> tag, and once in the gen=js handler:

$s .= "var skin = '{$this->skinname}';\nvar stylepath = '{$wgStylePath}';";

As the gen=js script (MediaWiki:Skinname.js and MediaWiki:Common.js) is loaded later, this overwrites the former definition. This causes a problem with the &useskin URL parameter and "skin" variable, as the gen=js script is cached with the "skin" variable defined by the site default or user preference skin choice. This means, that any check for skin, like "if(skin=='monobook')" is doomed to failure when used with &useskin

Live example: http://test.wikipedia.org/wiki/Skin_Alert_Thingy

Suggested fix: As the skin definition in gen=js appears to be a legacy issue it could probably simply be removed, along with stylepath. However, for backwards support, a simple if-defined check would work just as well (to check if defined, check if the parent object has such an attribute; for a global variable, this is the window object) :

$s .= "if(!window.skin) var skin = '{$this->skinname}';\nvar stylepath = '{$wgStylePath}';";


Version: 1.11.x
Severity: normal
URL: http://test.wikipedia.org/w/index.php?title=-&action=raw&smaxage=0&gen=js

Details

Reference
bz10316

Event Timeline

bzimport raised the priority of this task from to High.Nov 21 2014, 9:47 PM
bzimport set Reference to bz10316.
bzimport added a subscriber: Unknown Object (MLST).

egon.wiki wrote:

Fix for old JS variables declaration

Fix for bug.

Attached:

The gen=js also includes the skin-specific JS, such as MediaWiki:Monobook.js,
so any incompatibility would go to much more than that one variable.

A cache-safe fix would be for the gen=js link to include variant information (eg the current skin name) in the URL.

I've done this in r23924.

Not sure whether the global 'skin' and 'stylepath' settings should be removed from the gen=js or from the inline vars, but this fixes the inconsistency between them.
It also fixes the inconsistent use of skin-specific .js files (MediaWiki:Monobook.js loaded for wrong skin, etc).
By passing the skin name directly in the gen=js, we ensure both that we have the correct skin information cached
and that you'll get the JS along with useskin= on an HTML page.

Normally useskin= prevents caching, but RawPage handles its own caching headers, so this doesn't cause any problems here. Doesn't seem to be a performance problem in my quick ab testing either.