Page MenuHomePhabricator

ResourceModuleSkinStyles does not resolve skin aliases
Closed, ResolvedPublic

Description

Due to the migration out of MobileFrontend, the Minerva skin has 2 aliases

"ValidSkinNames": {
               "minerva": "Minerva"
               "minervaneue": "MinervaNeue"
}

However, when removing the minervaneue key from ValidSkinNames, all skinStyles defined with "minerva" fail to apply.

This is despite SkinMinervaNeue class and SkinMinerva class defining $skinname as 'minerva'

Acceptance criteria

  • ResourceModuleSkinStyles should not make guesses from the skin class name about what the name of the skin is when $skinname is available.

Details

Related Gerrit Patches:

Event Timeline

Krinkle added a subscriber: Krinkle.EditedAug 19 2017, 2:53 AM

The skin registry, as known to SkinFactory and Skin::getSkinNames, has no concept of aliases.

The skinname property within the Skin class is actually mostly unrelated (and unused). The skin name is primarily known and used externally (outside the Skin class). Internally no such property should need to exist. However, in the days before ResourceLoader and ExtensionRegistry, skinname was often used to find the directory where static assets (CSS, JS, images) are stored.

After ResourceLoader, the property was unused again, but then the Vector skin introduced a new use for it: As prefix for interface messages that skins could override. This was needed because the skin name is not (yet) injected into the Skin class constructor, so it also needed to be set within the Skin subclass. I suppose this made it possible (in theory) for multiple skins to exist where one re-uses the same interface messages, although that's not an intentional (nor documented) feature.

However, aside from that one detail with message keys, there is no re-use of resources possible between skins (unless explicitly referencing the same file), and there is no skin alias concept in MediaWiki core.

Back to the issue at hand:

However, when removing the minervaneue key from ValidSkinNames, all skinStyles defined with "minerva" fail to apply.
This is despite SkinMinervaNeue class and SkinMinerva class defining $skinname as 'minerva'

Can you clarify what the problem is exactly? It sounds like when both classes are registered in ValidSkinNames, styles added to modules using skinStyles[minerva] are also used by MinervaNeue, but when removing minervaneue: MinervaNeue and leaving minerva: MinervaNeue, the styles for skinStyles[minerva] no longer apply. Is that right?

I'm not sure how that could be possible. The code in ResourceLoader::register and ResourceLoaderContext::skin only uses the skin name as known to the skin registry, which would be different for minerva and minervaneue.

Can you clarify what the problem is exactly? It sounds like when both classes are registered in ValidSkinNames, styles added to modules using skinStyles[minerva] are also used by MinervaNeue, but when removing minervaneue: MinervaNeue and leaving minerva: MinervaNeue, the styles for skinStyles[minerva] no longer apply. Is that right?

Yep that is indeed the problem statement.

I'm not sure how that could be possible

Me neither, but it seems to be the case. https://en.wikipedia.org/w/index.php?title=Headings&action=edit&useskin=minervaneue is applying skin styles from mediawiki.action.edit.styles

The code in ResourceLoader::register and ResourceLoaderContext::skin only uses the skin name as known to the skin registry, which would be different for minerva and minervaneue.

I'll dig into this a little more and try to understand exactly what's happening here.

Jdlrobson added a comment.EditedAug 21 2017, 8:02 PM

I see...
A visit to https://en.wikipedia.org/w/index.php?title=Headings&action=edit&useskin=minervaneue generates a url to load.php which passes skin=minerva
Applying https://gerrit.wikimedia.org/r/#/c/372472/1 makes it point to skin=minervaneue

It seems Skin::$skinname is used to generate that url and $skinname must be in ValidSkinNames for that to work (https://github.com/wikimedia/mediawiki/blob/master/includes/resourceloader/ResourceLoaderContext.php#L95).

This all seems a little messy.

Since migration is not worth the hassle, we may have to standardise on the key being "minerva" and the class and repo name being MinervaNeue (https://gerrit.wikimedia.org/r/#/c/372472/2). We'll have to migrate any existing users over to minerva shortly after the deployment.

Krinkle added a comment.EditedAug 25 2017, 10:00 PM

It seems Skin::$skinname is used to generate that url and $skinname must be in ValidSkinNames for that to work (https://github.com/wikimedia/mediawiki/blob/master/includes/resourceloader/ResourceLoaderContext.php#L95).
This all seems a little messy.

Indeed. ResourceLoaderContext's constructor does not use Skin::$skinname. But there is indeed one use of Skin::$skinname, aside from the aforementioned internal use for prefixing message keys in SkinTemplate. The one external use is in OutputPage when creating the query parameters for the load.php url.

This was probably done that way because MediaWiki's main RequestContext object loses the string ID that it used to create the Skin object, so it uses getSkinName as a work-around to re-discover it afterward, on the assumption they'll be equal.

@Jdlrobson Would you support solving this by removing the discrepancy? Thus removing the only non-internal use of Skin::$skinname, in favour of always using the name used in the registry (as other code like SkinFactory/preferences/uselang does already.)

In writing the patch, I realised there is actually a way to create an alias skin with a different name that still renders the same skin in all ways. For that, Minerva could have two different keys in ValidSkinNames (and potentially two different classes, though they may also share the same PHP class), and make sure to overwrite getSkinName() to always return the same string, or explicitly set Skin::$skinname to be the same (if using multiple classes).

However, as far I can see Minerva is doing exactly that. I think the problem may've been that the aliasing does work, but internally only one skinname can be canonical. (e.g. not a merge of two, but one being only a setup-level alias to the canonical name).

Standardising on skinname minervaneue could work, but that does mean interface message keys, skinStyles, body class (e.g. skin-minerva), and mw.config.get('skin') will change. The alias would serve to support existing preferences and useskin queries. I think for compat it might indeed be useful to keep standardising on the skinname minerva internally and only use "Neue" in interface messages, unless you want to the change affect message keys, skin styles, body class and mw.config. (And possible breakage/migration that requires for extensions and user scripts)

Change 373971 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] Skin: Make skins aware of their registered skin name

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

Change 373971 merged by jenkins-bot:
[mediawiki/core@master] Skin: Make skins aware of their registered skin name

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

Restricted Application added a project: Performance-Team. · View Herald TranscriptJan 2 2018, 8:18 PM
Krinkle closed this task as Resolved.Jan 2 2018, 8:19 PM
Krinkle claimed this task.
Krinkle moved this task from Backlog to Accepted Enhancement on the MediaWiki-ResourceLoader board.