What about all the other skins that use it? Namely, er, nearly all of them?

BaseTemplate::getFooterIcons is being hard deprecated NOT SkinTemplate::getFooterIcons, which is the subject of this task.

Confusingly class SkinTemplate extends Skin and class TimelessTemplate extends BaseTemplate so any skin making use of BaseTemplate::getFooterIcons needs to transition to SkinTemplate::getFooterIconsor $this->get('footericons')

Note the point of these changes is to get away from this confusion. In future BaseTemplate / QuickTemplate will only be concerned with outputting data, not constructing data themselves. Hope this clears up confusion.

In T267447#6638443, @Jdlrobson wrote:

BaseTemplate::getFooterIcons is being hard deprecated NOT SkinTemplate::getFooterIcons, which is the subject of this task.

SkinnameTemplate extending BaseTemplate has been standard since skins were broken out of core and reimplemented as standardised extensions. Thus, unless they're using BaseTemplate::getFooter instead (unlikely as A, it took me way too long to get that added after the fact, and B, I put the wrong one in there so it's borderline unusable anyway) or the skin in question either doesn't have it or really went out of their way to reinvent the wheel and isn't following any of the usual practices, it's going to have this.

Confusingly class SkinTemplate extends Skin and class TimelessTemplate extends BaseTemplate so any skin making use of BaseTemplate::getFooterIcons needs to transition to SkinTemplate::getFooterIconsor `$this->get('footericons')

So, pretty much all of them?

Note the point of these changes is to get away from this confusion. In future BaseTemplate / QuickTemplate will only be concerned with outputting data, not constructing data themselves. Hope this clears up confusion.

BaseTemplate::getFooterIcons is basically just a helper function to BaseTemplate::getFooter (or whatever version thereof the skin actually implements) to add the footer icons. Are you saying it should be doing so by calling SkinTemplate::getFooterIcons to get the data itself? And if so, why? What possible other purpose could it have besides for rendering BaseTemplate::execute output?

SkinnameTemplate extending BaseTemplate has been standard since skins were broken out of core and reimplemented as standardised extensions.

Sure and they will continue to work. The purpose of these changes is to more clearly separates the model from the view layer in skins. This work helps us move towards an API-driven frontend (T140664).

The various skins that use this particular method will need to be patched eventually as with any deprecated functions. This should be pretty straightforward. It replaces one function call with another. If skins do this and raise any problems they have while doing so that is useful input into making the data model more flexible. What exactly are you seeing as the challenge here? If the fear is that this migration cannot happen before 1.36, I want to assure you that if not enough skins haven't switched over this can always be bumped to 1.37.

BaseTemplate::getFooterIcons is basically just a helper function to BaseTemplate::getFooter (or whatever version thereof the skin actually implements) to add the footer icons
Are you saying it should be doing so by calling SkinTemplate::getFooterIcons to get the data itself?

BaseTemplate::getFooter outputs an HTML string - that's what a template should do.
In this particular case calling $this->get('footericons') is all that should be needed to read the data. Whenever you call getFooterIcons('icononly') you are recreating data to return the value of $this->get('footericons') inefficiently. Updating to use footericons is in the skin's best interest to avoid this.

The`nocopyright` parameter adds confusion but is used by only 2 skins and easily replicated in the skin itself:
https://codesearch.wmcloud.org/search/?q=%27nocopyright&i=nope&files=&repos=

The fact that BaseTemplate makes its own template variables is what we're trying to move away from. The only concern of a template renderer should be querying and outputting that data (calling ->get, ->getSidebar, ->text, ->haveData and ->message, etc..).

In this particular case calling $this->get('footericons') is all that should be needed to read the data. Whenever you call getFooterIcons('icononly') you are recreating data to return the value of $this->get('footericons') inefficiently. Updating to use footericons is in the skin's best interest to avoid this.

So it sounds like getFooterIcons is inefficient and should be made less so? Because the alternative you seem to be recommending is to copy the same code into twenty different skins, which may or may not be the optimal code, or may not be the optimal code down the line, and not only is this just going to result in a mess of unmaintained code copy, but even if some of them do get updated eventually, this is how you get random divergence and inconsistent output.

One of the main problems with working with skins, in terms of making and maintaining the skins themselves and also, especially, making content, extensions, and other components work across different skins is this. Skinning output is just pointlessly inconsistent. When there is no damn reason for it to be so. Because there aren't common functions for common things in core, or the common functions don't work for what skins actually need.

And you want to remove one that actually does seem to be consistently used and have every skin roll its own output? Seriously, why? Fix the function to be less silly, but we need to keep the output consistent.

So it sounds like getFooterIcons is inefficient and should be made less so?

No that's not what I'm saying.

One of the main problems with working with skins, in terms of making and maintaining the skins themselves and also, especially, making content, extensions, and other components work across different skins is this. Skinning output is just pointlessly inconsistent. When there is no damn reason for it to be so. Because there aren't common functions for common things in core, or the common functions don't work for what skins actually need.

Which is exactly what we're trying to work towards solving here.

And you want to remove one that actually does seem to be consistently used and have every skin roll its own output? Seriously, why? Fix the function to be less silly, but we need to keep the output consistent.

Because there is absolutely no need to call it. Please look at the code and see that.

In the line https://github.com/wikimedia/mediawiki/blob/master/includes/skins/SkinTemplate.php#L350 the value of SkinTemplate::getFooterIcons is set to the QuickTemplate (not to be confused with SkinTemplate) value 'footericons':

$tpl->set( 'footericons', $this->getFooterIcons() );

That method is defined here https://github.com/wikimedia/mediawiki/blob/master/includes/skins/SkinTemplate.php#L207

The method in BaseTemplate is redundant, incomplete and does some needless type checking:
https://github.com/wikimedia/mediawiki/blob/master/includes/skins/BaseTemplate.php#L329

This is why it is deprecated.

And you want to remove one that actually does seem to be consistently used and have every skin roll its own output? Seriously, why? Fix the function to be less silly, but we need to keep the output consistent.

There is absolutely no need to copy this method. As I've pointed out above just use $this->get('footericons')
Functions that are not using it are generating inconsistent output. The existence of the function is what is silly.

Change 644933 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MonoBook@master] BaseTemplate::getFooterIcons is deprecated

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

Change 644933 merged by jenkins-bot:
[mediawiki/skins/MonoBook@master] BaseTemplate::getFooterIcons is deprecated

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

I suspect this has caused T269776: Undefined index: alt at includes/skins/Skin.php:1063 logspam.

I've commented there. This change delegated to core, so the issue likely relates to core code with certain types of config.

Change 668479 had a related patch set uploaded (by Zabe; owner: Zabe):
[mediawiki/skins/Timeless@master] BaseTemplate::getFooterIcons is deprecated

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

Change 668479 abandoned by Zabe:
[mediawiki/skins/Timeless@master] BaseTemplate::getFooterIcons is deprecated

Reason:
Overlocked that there is the 'nocopyright' option

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

Change 668479 restored by Zabe:
[mediawiki/skins/Timeless@master] BaseTemplate::getFooterIcons is deprecated

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

Change 668479 merged by jenkins-bot:
[mediawiki/skins/Timeless@master] BaseTemplate::getFooterIcons is deprecated

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

Change 673494 had a related patch set uploaded (by Zabe; owner: Zabe):
[mediawiki/core@master] Hard deprecate BaseTemplate::getFooterIcons()

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

Change 673494 merged by jenkins-bot:
[mediawiki/core@master] Hard deprecate BaseTemplate::getFooterIcons()

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

Thank you @Zabe!