Change 631681 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Move SkinTemplate::getFooterLinks() to Skin.

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

Change 631681 merged by jenkins-bot:
[mediawiki/core@master] Move SkinTemplate::getFooterLinks() to Skin.

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

Change 631905 had a related patch set uploaded (by Ammarpad; owner: Ammarpad):
[mediawiki/core@master] Move SkinTemplate::buildContentNavigationUrl to Skin

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

I think there's a need to discuss this more before attempting implementing

buildContentNavigationUrl is promoted to SkinTemplate
The SkinTemplateNavigation::Universal hook is modified to run with a Skin instance instead of a SkinTemplate.

As noted in review, over time this method has degenerated to almost uncontrollable logic. It's larger than many classes and even worse, it is also running four hooks, each with instance of SkinTemplate. (two are now fortunately soft-deprecated, but still supported and used by a lot of things).

• SkinTemplatePreventOtherActiveTabs
• SkinTemplateNavigation::SpecialPage
• SkinTemplateNavigation
• SkinTemplateNavigation::Universal

With the new hook system, each of them is an interface. So how will changing them to run with Skin instance work now with least disruption?

In T255319, SkinTemplateNavigation::SpecialPage and SkinTemplateNavigation are being deprecated in favor of SkinTemplateNavigation::Universal, but once all code has been migrated, SkinTemplateNavigation name will be free, and basically there's no need for the ::Universal suffix, which has its own inconveniences. For instance, apart from looking rather complicated than it's actually, it also has to differ from the actual interface name where colons are not allowed.

In my quick thought, to make this simpler. SkinTemplate::buildContentNavigationUrl(), and all that it's dragging (two private methods, plus 4 hooks ) should not be moved to Skin. Anyway it's done, it would be quite risky and prone to error.

Instead of moving, just leave it where it's now and completely recreate the method with more reasonable logic in Skin. Forget about all the SkinTemplate hooks and introduce single new hook 'SkinNavigation' to be used in the method. The method will run this hook and only it.

Two of the hooks are already deprecated, so there's no need to carry them in the new method. The other non-deprecated hook (SkinTemplatePreventOtherActiveTabs) is currently not being used by any client code. If it's still needed, reassess who need it and expose the possibility in 'SkinNavigation hook.

The only downsides of this plan as far as I can see, is that it will introduce mild inconvenience by requiring more migration after the one ongoing in T255319 especially for those who have already done so, but that is easily offset by the long-term benefit of fixing the mess of this method and those hooks. For those who have not yet migrated, there's nothing to worry about, the task can simply be stalled and let everyone know to wait to migrate to SkinNavigation completely.

After all reasonably maintained client extensions/skins have been migrated to use the new Skin method and SkinNavigation, then simply delete the SkinTemplate::buildContentNavigationUrl().

A new hook "SkinNavigation" a new skin method seems like an interesting idea but we'd want to make sure the older hooks never execute alongside the new SkinNavigation hook. One way to do that would be to use the same function name without inheritance e.g. Skin::buildContentNavigationUrl

I would suggest refactoring SkinTemplate::buildContentNavigationUrl where we can, to use any new Skin helper methods that come out of writing the new method

The only downsides of this plan as far as I can see, is that it will introduce mild inconvenience by requiring more migration after the one ongoing in T255319 espec

Perhaps but If T255319 is done we could make SkinTemplateNavigation::Universal alias to the new hook to which it will be compatible. A SkinTemplate is always a Skin, but a Skin is not always a SkinTemplate so SkinTemplateNavigation::Universal hook will be interchangeable with the SkinNavigation hook (just not the other way round).

Change 637889 had a related patch set uploaded (by Reedy; owner: Ammarpad):
[mediawiki/core@REL1_35] Move SkinTemplate::getFooterLinks() to Skin.

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

Change 637889 merged by jenkins-bot:
[mediawiki/core@REL1_35] Move SkinTemplate::getFooterLinks() to Skin.

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

Change 631905 abandoned by Ammarpad:
[mediawiki/core@master] Move SkinTemplate::buildContentNavigationUrl to Skin

Reason:
Needs new strategy

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

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

[mediawiki/core@master] Going forward its suggested that QuickTemplate related code is moved out of SkinTemplate into QuickTemplate as much as possible

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

@Ammarpad @Mainframe98 I've been rethinking this since we last spoke. Please take a look at my recent patch (which should be considerable reviewable/mergeable) about the suggested approach.
In future I think we can work towards the following:

• Skin
  • SkinTemplate
    • SkinMustache
    • SkinVue

Let me know what you think.

In 709094, @Mainframe98 wrote:

My only concern about keeping SkinTemplate as in-between is that it causes a weird inheritance model.
There's two (three soon) ways to render a skin: SkinTemplate using QuickTemplate, SkinMustache using Mustache and soon SkinVue using Vue. By having the parent of SkinMustache also providing the QuickTemplate related templating logic, any child of it will also lug that legacy with it.
Having that said, it would be way to much work (and backwards-incompatible changes) to go through with that. If the end goal is to move most of that to QuickTemplate, then this is a fine intermediate step.

Thanks for the thoughts! Ideally I would create SkinPHP which extends SkinTemplate and implements (not extends).

protected function setupTemplate( $classname ) {
                return new $classname( $this->getConfig() );
        }

and make SkinTemplate::setupTemplate look like:

protected function setupTemplate( $classname ) {
                wfDeprecated( __METHOD__, '1.??', 'Please extend SkinPHP instead; );
                return new $classname( $this->getConfig() );
        }

This would however be quite a disruptive change, as it would impact all legacy skins, so I think we're a few years off, but we could work towards that goal. Having just one method to worry about (setupTemplate) seems obtainable for now.

Either the setupTemplate or generateHTML would need to be modified to achieve the above.

As for Skin vs SkinTemplate, if we ever got to that situation it would make sense to merge the two classes.

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

[mediawiki/skins/MinervaNeue@master] Prepare for separation of QuickTemplate code from SkinTemplate

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

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

[mediawiki/core@master] Promote SkinMustache methods to SkinTemplate

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

Change 710354 abandoned by Jdlrobson:

[mediawiki/skins/MinervaNeue@master] Prepare for separation of QuickTemplate code from SkinTemplate

Reason:

No longer necessary

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

Change 713721 merged by jenkins-bot:

[mediawiki/core@master] Promote SkinMustache methods to SkinTemplate

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

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

[mediawiki/core@master] Introduce SkinBaseTemplate class, deprecate SkinTemplate extension

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

In T263213#7312646, @gerritbot wrote:

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

[mediawiki/core@master] Introduce SkinBaseTemplate class, deprecate SkinTemplate extension

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

I assume this is named after "BaseTemplate", but it sound rather confusing since SkinTemplate is already the "base" class for all skins and its description explicitly positions itself as the Skin subclass to for skins that use QuickTemplate.

I also don't understand why we need to introduce or break compat for anything in this area. What problem do you see that this would solve? Perhaps I can help offer a simpler way.

The main problem I see today is that the introduction of SkinMustache was done in a way that doubled the skin generation cost as it did not re-use the $tpl variables, nor such that the mustache data vars are the base that feed into $pl. Instead it re-computed the same things a second time on all page views (flame graph highlight: F34625780). If I understand the state of master correctly, this problem is solved by recent commits and going out in this week's train, via the override of generateHTML in SkinMustache avoiding the QuickTemplate init from happening. Is that right?

If understand correctly, you don't want SkinMustache to extend Skin directly but rather keep the two-layers of base classes (Skin and SkinTemplate) for non-QuickTemplate skins as well (e.g. SkinMustache) and evolve SkinTemplate to be the base for all skins. If that is the intention, would it make sense to move out most QuickTemplate-specific stuff into either the BaseTemplate or QuickTemplate classes, with just a small foorprint enabling based on the '$template' skin parameter (which effectively toggles QuickTemplate for compat). Alternatively, if we want to remove all traces of QuickTemplate (incl the $template option) from the SkinMustache's parent class, would it make sense to extend Skin directly?

Regarding data sharing - in what way do the two skin classes need to share data? I assume at run-time only ever one will be instantiated, either Mustache or QuickTemplate. If you mean the sharing of logic for how to gather the data, a getter method like that seems uncontroversial to surface in the Skin class.

Hey @Krinkle thanks for your comments! I hope this makes sense, and would love to chat long term skin plan with you to make sure we're on the same page if that's helpful.

Right now SkinTemplate is very BaseTemplate-orientated. There is a lot of code executed there for SkinMustache that doesn't need to be and the bloat in the class makes it really difficult as a developer to read and make changes confidently. Many of the methods are deprecated which also hurts readability at current time. SkinTemplate is 1673 lines and Skin is 2627 lines. Most of this code relates to Menu's and in future is envisioned as being pulled out of the Skin into a separate component (see T272624 and https://gerrit.wikimedia.org/r/c/mediawiki/core/+/612686)

Also, we've been moving skins away from generating the HEAD element of the document, and delegating that responsibility to OutputPage. One of the motivations for this, is getting all skins templates in a form where they can be driven by an API. That's a significant change/constraint and I think it warrants a new class per the above I've suggested. For example, in this new world we need to avoid BaseTemplate skins from using the following variables:

$tpl->set( 'bottomscripts', $this->bottomScripts() );
$tpl->set( 'headelement', $out->headElement( $this ) );

I see the new class as the safest way to carefully strip away those responsibilities and handle deprecations of template variables.

If understand correctly, you don't want SkinMustache to extend Skin directly
would it make sense to extend Skin directly?

In an ideal world, SkinMustache would extend Skin and another class would extend Skin to support BaseTemplate-based skins. SkinMustache requires methods currently in SkinTemplate as well as Skin and there is one hook that requires an instance of SkinTemplate as a parameter which makes SkinMustache extending Skin more difficult than it should be. Another approach could certainly explore upstreaming all the necessary code to Skin.php and deprecating that hook in favor of a Skin (rather than SkinTemplate) based hook.

Skin and SkinTemplate should be converged as an outcome. It doesn't make sense to have two classes for this concept anymore IMO. Are there benefits of having two?

I also don't understand why we need to introduce or break compat for anything in this area.

No compatibility is broken by this change at this point. I'm proposing soft deprecation (at least for now). To avoid duplicating function bodys I've broken out some static functions on the new class used in the old class. However refactoring SkinTemplate will hopefully clear up what's deprecated and what's not.

Regarding data sharing - in what way do the two skin classes need to share data?

The data inside SkinMustache is a subset of BaseTemplate's. A lot of the data in BaseTemplate is no longer needed, or can be derived from SkinMustache based data. Some of the data in BaseTemplate is no longer relevant and we need to get skins off it (e.g. bottomscripts and headelement example above).

In T263213#7331769, @Jdlrobson wrote:

Right now SkinTemplate is very BaseTemplate-orientated.

SkinTemplate exists for the sole purpose of being a Skin subclass for skins that use QuickTemplate. It was a mistake to use it as parent for SkinMustache. I think we're aligned that you'd like to resolve that too, and I believe this can be done cleanly and without breaking changes.

Also, we've been moving skins away from generating the HEAD element of the document, and delegating that responsibility to OutputPage. One of the motivations for this, is getting all skins templates in a form where they can be driven by an API. […]

$out->headElement( $skin );
$skin->bottomScripts();

The head element has already been standardised and injected from OutputPage for over a decade. I haven't seen any significant change in that area.

I agree we can and should provide the same for bottomScripts indeed.

However, I don't see how this relates to an API-driven future (i.e. T140664). The way I see it, this future is mainly constrained by the principle of "frontend HTML response must come from a single front-most layer, using only data that it was given by an earlier layer". In other words, no backpeddling where a skin method goes back into the database to fetch more data.

That front-most layer is the skin. When a skin is given two datasets (page data for given language+skin, and user data), I'm not sure how much it matters whether the Skin's has its own inline <head> tag that uses some page fields, or whether that tag as a whole comes from a substituted page field. From what I can tell, either would be acceptable and suppported in the model and would not make things better or worse in terms of what we can do with it. Again, I agree that we should standardise this for ease of maintenance and to ease boostrapping new skins. But, I don't see how it beneifts the API-driven model, and thus don't see a problem with allowing skins to roll their own for any reason, or why that'd make core work any different.

I see the new class as the safest way to carefully strip away those responsibilities and handle deprecations of template variables.

As far as I can tell, all we need to do here is move the functions you want to re-use for SkinMustache from SkinTemplate to Skin, and then have SkinMustache extend that directly, thus not inheriting the methods that (also) pay for setting up a QuickTemplate object that it wouldn't use.

If understand correctly, you don't want SkinMustache to extend Skin directly
would it make sense to extend Skin directly?

In an ideal world, SkinMustache would extend Skin and another class would extend Skin to support BaseTemplate-based skins. SkinMustache requires methods currently in SkinTemplate as well as Skin and there is one hook that requires an instance of SkinTemplate as a parameter which makes SkinMustache extending Skin more difficult than it should be. Another approach could certainly explore upstreaming all the necessary code to Skin.php and deprecating that hook in favor of a Skin (rather than SkinTemplate) based hook.

Which hook is that? Do its consumers actually interact with QuickTemplate-specific methods? Do Mustache-based skins have to invoke that hook? If they do, is this not already broken by SkinMustache ignoring what hooks do to template variables?

Based on the answers, you can either naturally lose it when SkinMustache extends Skin directly (thus limiting the hook to SkinTemplate skins), or possibly introduce a new hook that is higher up that provides a narrower contract that both can fulfill in a meaningful way. For example, if the hook relates to page data fields, that might be reason to include a basic "get template data" method high up in the Skin class, and then extend that for SkinMustache by adding keys, and in SkinTemplate it would port use that dataset when calling tpl->set() for data that is shared among skins.

I also don't understand why we need to introduce or break compat for anything in this area.

No compatibility is broken by this change at this point. I'm proposing soft deprecation (at least for now).

Deprecating code means consumers and cross-project volunteers will be encouraged to disuse it. Introducing a new stable class encourages adoption. It sounds like neither disuse of SkinTemplate nor adoption and long-term support of this new class are intended. Given that, it seems better not to do that in the first place if we already know we don't need that additional complexity and breakage.

Change 719153 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] skins: Clarify SkinTemplate and QuickTemplate class doc comments

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

Which hook is that? Do its consumers actually interact with QuickTemplate-specific methods? Do Mustache-based skins have to invoke that hook? If they do, is this not already broken by SkinMustache ignoring what hooks do to template variables?

There are several hooks and all of them are used by both SkinMustache and QuickTemplate based skins,

These hooks are:

• PersonalUrls
• SkinTemplateNavigation
• SkinTemplateNavigation__SpecialPage
• SkinTemplateNavigation__Universal

The associated methods are SkinTemplate::buildPersonalUrls and SkinTemplate::runOnSkinTemplateNavigationHooks

Both of these methods take an instance of SkinTemplate as an argument.

Deprecating either of these hooks would be extremely disruptive and changing the parameter class from SkinTemplate to Skin could be a breaking change for any skin that is relying on the BaseTemplate-based methods.

This is why I figured it might make sense to converge on SkinTemplate since Mustache is a type of template too and eventually make SkinTemplate an alias of Skin.

In T263213#7336658, @Jdlrobson wrote:

Which hook is that? Do its consumers actually interact with QuickTemplate-specific methods? Do Mustache-based skins have to invoke that hook? If they do, is this not already broken by SkinMustache ignoring what hooks do to template variables?

There are several hooks and all of them are used by both SkinMustache and QuickTemplate based skins,

These hooks are:

• PersonalUrls
• SkinTemplateNavigation
• SkinTemplateNavigation__SpecialPage
• SkinTemplateNavigation__Universal

Both of these methods take an instance of SkinTemplate as an argument.

A class is more than a name, it provides behaviours. If you're confident that the behaviours related to QuickTemplate aren't used by consumers of these hooks, or that it's rare enough that we can manage the deprecation/migration of those, then we can move those hooks up to Skin* and deprecate/remove the SkinTemplate-specific ones. We've done this numerous times before, including with the Revision and PageIdentity refactors.

f those QuickTemplate behaviours are actually used and expected, then we have a different problem. I suspect though that the vast majority don't call any of those on the Skin object they get, and likely don't even striclty type it either so dropping in a non-SkinTemplate Skin would likely just work. But we should go through deprecation of course.

Okay, that sounds good. Do you think it's acceptable to retain the existing hook names e.g. SkinTemplateNavigation if run inside Skin.php ? If so I am happy to proceed in this fashion.

I think it's more a case of it being rare and unexpected but we could mitigate this by making sure all relevant methods are upstreamed from SkinTemplate to Skin first. If an extension hook is using methods relating to BaseTemplate they are only going to work on BaseTemplate based skins anyway.

I'd copy rather than move, with the new version having the plainer "Skin" prefix in its hook name. For at least one release, they'd both work and be called (for skins based on SkinTemplate). Any given extension should only use one of them of course, and we'd find/replace at least the usage in deployed code before deprecating.

Change 719153 merged by jenkins-bot:

[mediawiki/core@master] skins: Clarify SkinTemplate and QuickTemplate class doc comments

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

Change 715058 abandoned by Jdlrobson:

[mediawiki/core@master] Introduce SkinBaseTemplate class, deprecate SkinTemplate extension

Reason:

We're reconsidered the approach here. I'll post a new patch soon.

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

Change 709094 abandoned by Jdlrobson:

[mediawiki/core@master] WIP: Separation of QuickTemplate code from SkinTemplate

Reason:

We're reconsidered the approach here. I'll post a new patch soon.

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

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

[mediawiki/core@master] WIp: Introduce SkinMenuDirector

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

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

[mediawiki/core@master] Make Skin.php readable (interface)

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

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

[mediawiki/core@master] Promote 3 SkinTemplate methods to Skin

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

Change 731179 merged by jenkins-bot:

[mediawiki/core@master] Promote 4 SkinTemplate methods to Skin, add getAction

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

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

[mediawiki/core@master] WIP: Introduce SkinComponentSearch

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

Change 730618 merged by jenkins-bot:

[mediawiki/core@master] Introduce SkinComponent and SkinComponentLogos

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