Change 562628 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Remove the need for Vector's ResourceLoaderLessModule and wgVectorPrintLogo

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

(Blocked on T232140)

Change 562628 merged by jenkins-bot:
[mediawiki/core@master] Remove the need for Vector's ResourceLoaderLessModule and wgVectorPrintLogo

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

Change 570197 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MonoBook@master] Drop mediawiki.skinning.interface in preparation for its deprecation

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

Change 555579 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Deprecate mediawiki.legacy.shared and mediawiki.skinning.interface

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

Change 570197 merged by jenkins-bot:
[mediawiki/skins/MonoBook@master] Drop mediawiki.skinning.interface in preparation for its deprecation

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

Change 574585 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Vector should get legacy styles from ResourceLoaderSkinModule

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

Change 574586 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MonoBook@master] Get legacy styles from ResourceLoaderSkinModule

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

Change 574588 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Wikibase@master] Do not reference mediawiki.legacy.shared in view/resources.php "for snakview"

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

Change 576429 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Legacy styles now come from ResourceLoaderSkinModule

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

Change 576433 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] mediawiki.legacy.shared is merged into existing skin modules and removed

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

I have lots of possible ways to fix my current goal which is mediawiki.legacy.shared in core will be marked as deprecated. To do this I need to do 2 things

1. Migrate existing deployed skins e.g. Vector, Monobook, Modern, CologneBlue to use ResourceLoaderSkinModule.
2. Send warnings to 3rd party skins including mediawiki.legacy.shared

This is a little tricky as there are some explicit usages and some inexplicit usages (skins not overriding Skin:getDefaultModules)

I can see 4 possible solutions to this and would appreciate a clear decision on which one to pursue (at least from @Krinkle :-)).

Solution 1: Use a skin version
https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/555579/
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/574585/
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MonoBook/+/574586/

Pros: Our skins can update safely without knowledge of internals of SkinTemplate in core. SkinTemplate can trigger warnings if skinVersion hasn't been bumped in news skins. New skins will get a warning when we mark mediawiki.legacy.shared in resources/Resources.php as deprecated and/or for not overriding skin version.

Cons: skinVersion could become annoying technical debt

Solution 2: Use getDefaultModules
Possibly cleaner. I'd rather avoid searching and removing from the array mediawiki.legacy.shared as this creates additional technical debt. We can however simply set the array on the expectation that mediawiki.legacy.commonPrint will soon be folded into ResourceLoaderSkinModule and $modules['styles']['core'] removed as well.

Pros: No changes to core required to pull skins off the mediawiki.legacy.shared dependency
Cons: After mediawiki.legacy.shared is removed from SkinTemplate we'll need to update all skins currently working around getDefaultModules results.

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

Solution 3: Mark as deprecated and remove from SkinTemplate right away, folding into mediawiki.skinning.*
Pros: We can update the Wikimedia deployed skins right away by adding a legacy features key; new skins lose legacy code.
Cons: Any skin already using ResourceLoaderSkinModule (unlikely and if they are hopefully they are on top of changes here) will lose legacy feature until they update their own code; Some skins may gain legacy styles unexpectedly if previously they were removing them (unlikely).

https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/576433 mediawiki.legacy.shared is merged into existing skin modules and removed

Solution 4: Rely on deprecation warnings
Pros: Can quickly react to all deprecation warnings in beta cluster.
Cons: Risk of deprecation warnings showing up in core if not done swiftly.
(I originally tried this but @Krinkle -1ing "Pending usage in our own code in core and bundled extensions:"

Thanks for writing that up @Jdlrobson.

• Using getDefaultModules (Solution 2):

Yeah, I agree array-searching is suboptimal here. Especially because that means skins can't really "migrate" as migration generally means you move towards an end-state that you can then stay on. If migrating means mentioning the deprecated module to exclude it, that code generally is something we'd want to remove again which means maintainers have to perform two stages instead of one. It also means any skin that is "doing the right thing" already still has to manually unload it.

Preferable would be if "doing the right thing" naturally leads to the legacy module not being queued by core. That also means RL's built-in deprecation concept can be used as a way to inform developers. If it doesn't end up being loaded (either because the skin never did, or because the skin used the modern way), then you don't get the deprecation warning. No false negative/positive.

A small change to this solution could be to for core to assign ['styles']['legacy'] instead of ['styles']['core']. Then new skins would only have to clear this array with an empty one, without needing to know what was in it. However that still leaves us with technical debt, because it means we can't move anything new into the legacy queue in the future for other deprecations.

• Fold into mediawiki.skinning.* (Solution 3):

I think this would work best. I agree the two cons you mentioned would be rare and in the worst case scenario they would still be relatively easy for the developer to mitigate. And even better if we document this gotcha in the release notes and inform Wikitech-l ahead of the release.

The one thing I'd change, is to maintain compatibility for one release cycle. That is, keep mediawiki.legacy.shared defined as deprecated module, and let the Skin code continue queuing it if (and only if) the skin doesn't queue any mediawiki.skinning.* or RLSkinModule-derived module. This logic could be isolated in a protected Skin method, e.g. called from OutputPage::loadSkinModules – where getDefaultModules() gets processed.

(For the use case of a skin not wanting to load any legacy styles and also not use SkinModule, their Skin class could no-op this method in their subclass.)

Thanks for the feedback and considering all the pros and cons! I'll push forward now with solution 3.

A small change to this solution could be to for core to assign ['styles']['legacy'] instead of ['styles']['core']

One downside of this is skins such as Minerva would need updating to clear out that new array. In future this will become less of a problem as I hope we can move all styles to the skin module.

The one thing I'd change, is to maintain compatibility for one release cycle. That is, keep mediawiki.legacy.shared defined as deprecated module, and let the Skin code continue queuing it if (and only if) the skin doesn't queue any mediawiki.skinning.* or RLSkinModule-derived module. This logic could be isolated in a protected Skin method, e.g. called from OutputPage::loadSkinModules – where getDefaultModules() gets processed.

That sounds like a good compromise. Thanks for the suggestion.

I'll move this to our radar for now and look forward to patch(es) in Gerrit. Feel free to CC or assign me there during code review.

The one thing I'd change, is to maintain compatibility for one release cycle. That is, keep mediawiki.legacy.shared defined as deprecated module, and let the Skin code continue queuing it if (and only if) the skin doesn't queue any mediawiki.skinning.* or RLSkinModule-derived module. This logic could be isolated in a protected Skin method, e.g. called from OutputPage::loadSkinModules – where getDefaultModules() gets processed.

Thinking about this some more, I am not seeing much advantage to this. A skin has every right to remove all modules and add its own module which is not a ResourceLoaderSkinModule.

In Icb910a563273bde92a09b1bb92857d5b6e348baa any skin loading mediawiki.skinning.interface or the already deprecated mediawiki.skinning.elements and mediawiki.skinning.content will already get the code and adding mediawiki.legacy.shared in addition to this will cause it to load twice. The CSS itself is not deprecated - only the loading mechanism it uses is.

In the rare case skins that were not using any of the mediawiki.skinning.* modules and are not using a ResourceLoaderSkinModule but want mediawiki.legacy.shared I don't see how we can notify them.

With that in mind I think the following 3 patches are ready for review:

Core: https://gerrit.wikimedia.org/r/c/576433
Monobook: https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MonoBook/+/574586/
Vector: https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/574585/

Change 577326 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Wikibase@master] Pull legacy styles from ResourceLoaderSkinModule rather than deprecated module

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

In T242177#5939466, @Jdlrobson wrote:

The one thing I'd change, is to maintain compatibility for one release cycle. That is, keep mediawiki.legacy.shared defined as deprecated module, and let the Skin code continue queuing it if (and only if) the skin doesn't queue any mediawiki.skinning.* or RLSkinModule-derived module. This logic could be isolated in a protected Skin method, e.g. called from OutputPage::loadSkinModules – where getDefaultModules() gets processed.

Thinking about this some more, I am not seeing much advantage to this. A skin has every right to remove all modules and add its own module which is not a ResourceLoaderSkinModule.

I agree, which is why this method can be disabled by sub classes. I was suggesting something like this, which a subclass of Skin can disable if they don't want them.

class Skin {
  …
  protected function addCompatModules( array $queued ) : array {
      if ( ! $queued contains skinning or SkinModule ) {
         return [ legacy.shared ]
     }
  }
}

However, I suppose this would effectively be a post-processing step that could also be implemented the other way around, which I think is what you're suggesting. That the skin would proactively, in their getDefaultModules() override, uncondiitonally set ['styles']['legacy'] to an empty array indefinitely.

However, that means every skin in existence will have to set legacy=[] in their getDefaultModules override in order to comply with the migration, to avoid deprecation warnings and breakage (in the form of double loading, which I consider breakage), and then every skin to change again when after MW 1.36 it becomes redundant (optional).

Is that correct?

addCompatModules

The issue with the proposed method is a skin is quite within its right to not contain a skinning or SkinModule and not include legacy.shared. All these modules are optional.
Side note: I'm also not sure how a protected method would get called by OutputPage which is the only place which knows the entire list of module names, nor how we can quickly look up the ResourceLoader class for those modules.

However, I suppose this would effectively be a post-processing step that could also be implemented the other way around, which I think is what you're suggesting. . That the skin would proactively, in their getDefaultModules() override, uncondiitonally set ['styles']['legacy'] to an empty array indefinitely.

No I'm not suggesting this for the very reasons you state. For skins we don't maintain that are already emptying the core array, we would now be adding mediawiki.legacy.shared to their skins. For skins which have added their own legacy key they will now not get the module. I think there's no clear solution that solves this problem for 100% of skins. I think if we really did want to keep the mediawiki.legacy.shared module around for those unknown, possibly non-existent impacted skins the best option would be to only add it to skins that are not overriding getDefaultModules but this is still not perfect as skins may be overriding it but not making any changes.

As a result I'm suggesting we don't try to retain it and rely on the RELEASE NOTE for other skins: https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/576433/3/includes/skins/Skin.php. If mediawiki.legacy.shared is not loaded it should not be catrastrophic to design. Skins will survive with or without this module. Those that have it will have the deprecation notice and those that don't but want it should notice rather quickly provided they are actively developing the skin and reading release notes. I know this is not perfect but I'm wary of doing more work that is needed here or that creates technical debt.

As I understand it, the vast majority of skins do not override getDefaultModules() at all right now, and the handful that do either only append or unset other things. Not core or legacy. Afaik that's just Minerva today. We shouldn't optimise for that scenario as such.

Right now mediawiki.legacy.shared is loaded by virtually all skins. As of late it has become possible to customise a SkinModule-based module to include this instead, which if a skin adopts that today, they also have to override getDefaultModules() and clear it from the legacy array, which his difficult right now as it contains unrelated legacy module as well. Do I understand that correctly, and do you agree this needs to change before we release?

What do you propose every skin will do for the next release cycle for the objective of no longer loading mediawiki.legacy.shared (and thus not its deprecation warnings) and how to instead have it load via SkinModule?

Change 574588 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Do not reference mediawiki.legacy.shared in view/resources.php "for snakview"

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

As I understand it, the vast majority of skins do not override getDefaultModules() at all right now, and the handful that do either only append or unset other things. Not core or legacy. Afaik that's just Minerva today. We shouldn't optimise for that scenario as such.

We don't know the answer to this. As far as I understand it only Monobook and Vector are impacted by the change I am proposing here. I can't talk for skins not on codesearch.wmflabs.org as we have no way of knowing whether they override getDefaultModules. Other skins are almost certain to be either using a mediawiki.skinning module (and will continue to get the CSS of mediawiki.legacy.shared OR they are rolling their own CSS module and overriding getDefaultModules (and unlikely to need mediawiki.legacy.shared)

Right now mediawiki.legacy.shared is loaded by virtually all skins

Yes that's clear. Whether those skins actually use it though is less clear.

which if a skin adopts that today, they also have to override getDefaultModules() and clear it from the legacy array, which his difficult right now as it contains unrelated legacy module as well.

yes.

do you agree this needs to change before we release?

Not necessarily because of the unknown I stated above. If there are skins that are tinkering with getDefaultModules to remove core styles like Minerva we will be adding mediawiki.legacy.shared to these skins and forcing a change to be made to them.

Skins which are not tinkering will also get deprecation warnings and will be forced to override it to suppress those. Basically everyone who doesn't want deprecation notices would have to override getDefaultModules from now as well as potentially updating their style module to be a ResourceLoaderSkinModule.

My proposed solution requires changes to 2 skins - Monobook and Vector (T242177#5939466) and the one you are proposing would also require changes to Minerva, CologneBlue, Modern and Timeless.

What do you propose every skin will do for the next release cycle for the objective of no longer loading mediawiki.legacy.shared

I'm not sure exactly what's being asked here.

We are making changes that require all skins to audit what styles they need and utilise ResourceLoaderSkinModule. Eventually a skin should provide one CSS module (or include mediawiki.skinning.interface which will not be deprecated). They will become aware of this need for change by the ResourceLoader deprecated modules or reading the release notes and noticing change in how their skin renders (if any).

That migration strategy is easy from my point of view. What's more painful is having to force all skins to update getDefaultModules regardless of whether they are or are not using it. The best migration strategy in my opinion is one where most skins do not need to do anything even if there is a risk that some skins without a mediawiki.skinning module will lose mediawiki.legacy.shared.

Where are we disagreeing? Should I schedule for us some time to talk about this one on one to avoid the problems of async communication?

Change 577326 abandoned by Jdlrobson:
Pull legacy styles from ResourceLoaderSkinModule rather than deprecated module

Reason:
No longer needed

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

Change 579362 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/CologneBlue@master] Use ResourceLoaderSkinModule

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

Change 579363 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Modern@master] Use ResourceLoaderSkinModule

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

Change 574585 had a related patch set uploaded (by Krinkle; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Get legacy styles from ResourceLoaderSkinModule

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

Change 576433 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.legacy.shared is merged into existing skin modules and removed

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

Change 579363 merged by jenkins-bot:
[mediawiki/skins/Modern@master] Use ResourceLoaderSkinModule

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

Change 579362 merged by jenkins-bot:
[mediawiki/skins/CologneBlue@master] Use ResourceLoaderSkinModule

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

I'm working on going through impacted skins and submitting patches/issues as applicable.

Change 574585 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Get legacy styles from ResourceLoaderSkinModule

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

Change 574586 merged by jenkins-bot:
[mediawiki/skins/MonoBook@master] Get legacy styles from ResourceLoaderSkinModule

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

After chatting with Timo we agreed to drop mediawiki.legacy.shared given that many skins are likely including it without even realising it. We've made a note of this in the release notes and I've gone through the skins that I am aware of (show up on codesearch.wmflabs.org) submitting patches/issues where I can. Please consider this my due-diligence!

Skins that possibly might be impacted by this change:

• WebPlatform (patch)
• Truglass (patch/issue)
• Tempo (patch/issue)
• Splash (patch/issue)
• Erudite (patch/issue)
• Mask (patch/issue)
• LibertySkin (patch/issue)

Skins not impacted by this change (or untestable)

• Poncho (not compatible with latest mw)
• Athena not compatible with latest mw
• BlueSky ( uses ResourceLoaderSkinModule)
• GreyStuff ( uses ResourceLoaderSkinModule)
• p2wiki (incompatible with latest mw)
• Tweeki (incompatible with latest mediawiki)
• chameleon (but opportunity to simplify their code > https://github.com/cmln/chameleon/issues/110)
• DarkVector (using mediawiki.skinning.interface)
• StopStopa (Blackout extension) - legacy styles are irrelevant when you are blackign out content :)
• DumpHtml SkinOffline - not compatible with latest mediawiki
• FemWikiSkin (using mediawiki.skinning.interface)
• SkinAmethyst (using mediawiki.skinning.interface)
• Anisa (using mediawiki.skinning.interface)
• Apex (using mediawiki.skinning.interface)
• Bouquet (using mediawiki.skinning.interface)
• Daddio will trigger deprecation notice as adding module directly
• SkinDeskMessMirrored (using mediawiki.skinning.interface)
• Dusk (using mediawiki.skinning.interface)
• DuskToDawn (using mediawiki.skinning.interface)
• ModernSkylight - already broken prior to this change Uncaught Error: Call to undefined method OutputPage::addModuleScripts()
• SkinEUCopyrightCampaign (using mediawiki.skinning.interface)
• GamePress (using mediawiki.skinning.interface)
• Material (using mediawiki.skinning.interface)
• Strapping not compatible with latest mw
• MetroLook (using mediawiki.skinning.interface)
• Nimbus (using mediawiki.skinning.interface)
• Nostalgia will follow deprecation policy
• HasSomeColors not compatible with latest mw
• Refeshed (using mediawiki.skinning.interface)
• WPtouch (using mediawiki.skinning.interface)
• WoogleShades (using ResourceLoaderSkinModule and not touching features)

Change 579413 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Nostalgia@master] Drop deprecated (or soon to be deprecated) mediawiki.legacy modules

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

Change 579415 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/CologneBlue@master] Drop deprecated (or soon to be deprecated) mediawiki.legacy modules

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

Change 579416 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Deprecate mediawiki.legacy.oldshared

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

Change 579420 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Move contents of mediawiki.legacy.commonPrint into ResourceLoaderSkinModule

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

Change 579413 merged by jenkins-bot:
[mediawiki/skins/Nostalgia@master] Drop deprecated (or soon to be deprecated) mediawiki.legacy modules

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

Change 579415 merged by jenkins-bot:
[mediawiki/skins/CologneBlue@master] Drop deprecated (or soon to be deprecated) mediawiki.legacy modules

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

Change 579416 merged by jenkins-bot:
[mediawiki/core@master] Deprecate mediawiki.legacy.oldshared

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

Change 579420 merged by jenkins-bot:
[mediawiki/core@master] Move contents of mediawiki.legacy.commonPrint into ResourceLoaderSkinModule

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

Change 607568 had a related patch set uploaded (by Gopavasanth; owner: Gopavasanth):
[mediawiki/core@master] Removed oldshared.css

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