Page MenuHomePhabricator

Allow skins to mark themselves as internal
Closed, ResolvedPublicBUG REPORT

Description

In core there are two skins SkinApi and SkinFallback.
ContentTranslation uses an internal skin which is only used on Special:ContentTranslation
wgSkipSkins can be used to hide skins in preferences, but is meant to be used for phasing out deployed skins, not for this purpose.

Eextensions themselves cannot disable them without modifying globals so it means ContentTranslation by default shows a skin it shouldn't.
https://gerrit.wikimedia.org/g/operations/mediawiki-config/+/022cafec405b2052a668b0a4c807d438e5b15ee1/wmf-config/InitialiseSettings.php#20470

Vector currently uses 2 skins under the same key. We would like to split those into 2 skins as in T291098. To support our QA process we would like to temporarily make the 2 new skins internal e.g. selectable for testing but not present them to users until we are ready to do so.

What should have happened instead?:

It's proposed that a skin when registered in the SkinFactory can mark itself as internal. This will generalize the existing code.

Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc:

Event Timeline

Change 721106 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Allow skins to be registered as internal

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

wgSkipSkins can be used to hide skins in preferences, but is meant to be used for phasing out deployed skins, not for this purpose.

It wasn't what it was designed for. It was only recently changed to support that use case. But yes, it can now also be used to phase out skins.

In core there are two skins SkinApi and SkinFallback.

SkinFallback is a skippable skin that we intentionally register with SkinFactory to expose it in all aspects of core and the extension eco system. It is intentionally registered with SkinFactory (instead of run-time DI for when it is needed only) to allow you to select it in anywhere anytime, locally, in CI, and even on random articles in production. You can also select it as a hidden preference (or by API call) to browse around with for a short while when testing things that require multiple page loads, useskin, user script pages and gadgets etc. The same as deprecated skins basically.

ContentTranslation uses an internal skin which is only used on Special:ContentTranslation

Reading a bit between the lines on T263093, and based on what @santhosh has previously shared in various conversations and the hurdles his team ran into over the years, I believe what the Language team mainly want is for this special page to be a standalone web app. And as part of the CX skin refactor this is essentially now reflected in code with the layout now nicely condensed into one central place, with the Skin subclass as mostly-empty shell to pull in a few bits of data (eg. personal links menu, incl extension hooks).

Note that a Skin class like this can be instantiated without registering with ValidSkinNames/SkinFactory. See P17287 for the minimal diff to showcase that.

Registering with SkinFactory implicitly also does the following:

I don't know if these werre anticipated, but this seems quite useful indeed. Note that if it is wanted to disable user JS entirely on these (e.g. common.js as well) one can call OutputPage::disallowUserJs() for that.

We would like to split [Vector] into 2 skins as in T291098. To support our QA process we would like to temporarily make the 2 new skins internal e.g. selectable for testing but not present them to users until we are ready to do so.

We might not want to use SkinFactory mechanism for that. This sounds more like configuration rather than software.

How would you address testing and local development? Presumably you'd not want to have to carry over an unmerged patch for several months to undo-and-redo continuosly during testing. Using $wgSkipSkins might be a better fit for that one.

Change 724184 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/ContentTranslation@master] Mark ContentTranslation skin as skippable

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

Change 721106 merged by jenkins-bot:

[mediawiki/core@master] SkinFactory: Allow skins to be registered as \"skippable\"

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

Jdlrobson claimed this task.

Thanks @Krinkle for the guidance with this one!

Change 724184 merged by jenkins-bot:

[mediawiki/extensions/ContentTranslation@master] Mark ContentTranslation skin as skippable

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