Page MenuHomePhabricator

composer/installers and wfLoadSkin do not assume the same path ('skin-name' vs. 'SkinName')
Closed, ResolvedPublic

Description

The paths assumed by composer/installers and wfLoadSkin are not compatible: the first assumes 'skin-name' while the other one assumes 'SkinName'.

The install location of the composer/installer is determined in the function inflectSkinVars in MediaWikiInstaller.php like so:

protected function inflectSkinVars($vars)
{
    $vars['name'] = preg_replace('/-skin$/', '', $vars['name']);
    return $vars;
}

MediaWiki determines the location of a skin to load using the function wfLoadSkin defined in GlobalFunctions.php like so:

function wfLoadSkin( $skin, $path = null ) {
   if ( !$path ) {
       global $wgStyleDirectory;
       $path = "$wgStyleDirectory/$skin/skin.json";
   }
   ExtensionRegistry::getInstance()->queue( $path );
}

https://git.wikimedia.org/blob/mediawiki%2Fcore.git/master/includes%2FGlobalFunctions.php#L205

The composer name for the Vector skin for example is vector-skin, so the skin will be installed under skins/vector. The MediaWiki name of the Vector skin is Vector, so it will be loaded from skins/Vector. On some file systems, the two paths are not the same, so installing skins with composer does not work.

Event Timeline

Ingomueller-net raised the priority of this task from to Needs Triage.
Ingomueller-net updated the task description. (Show Details)
Ingomueller-net added a subscriber: Ingomueller-net.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptNov 5 2015, 10:24 AM
Ingomueller-net set Security to None.
saper added a comment.Dec 15 2015, 9:46 PM

shall we have some "extra" parameter in composer.json?

Change 259462 had a related patch set uploaded (by saper):
We are Vector, not vector

https://gerrit.wikimedia.org/r/259462

Change 259463 had a related patch set uploaded (by saper):
We are Slate, not slate.

https://gerrit.wikimedia.org/r/259463

Looks like extra.installer-name does the job, nice finding. Note the value is added as-is and the installer inflexion is bypassed, but that does not seem to cause any issue.

Change 259465 had a related patch set uploaded (by saper):
We are Metrolook, not metrolook.

https://gerrit.wikimedia.org/r/259465

I guess it does the job. However it makes the task of extension developers more error-prone. For the very least, it should be documented clearly (here?).

Thankyou for spotting this.

Change 259462 merged by jenkins-bot:
We are Vector, not vector

https://gerrit.wikimedia.org/r/259462

Change 259465 merged by jenkins-bot:
We are Metrolook, not metrolook.

https://gerrit.wikimedia.org/r/259465

Change 259463 merged by jenkins-bot:
We are Slate, not slate.

https://gerrit.wikimedia.org/r/259463

saper added a comment.Dec 19 2015, 7:28 PM

Thanks for quick merge and discussion. I actually think that even a better fix is possible - for skins Composer installer should just read the value added to "ValidSkinNames":

ValidSkinNames: { "vector": "Vector" }

Good question what should be done if multiple values are present (which can happen in theory I think)

Legoktm closed this task as Resolved.Dec 30 2015, 2:03 AM

https://www.mediawiki.org/w/index.php?title=Manual%3AComposer.json_best_practices&type=revision&diff=1989984&oldid=1950019

Thanks for quick merge and discussion. I actually think that even a better fix is possible - for skins Composer installer should just read the value added to "ValidSkinNames":
ValidSkinNames: { "vector": "Vector" }
Good question what should be done if multiple values are present (which can happen in theory I think)

No idea. Let's handle that as a separate task since this one is fixed? :)