Page MenuHomePhabricator

BaseTemplate::getFooterIcons is deprecated, use $this->get('footericons') instead
Open, Needs TriagePublic

Description

BaseTemplate::getFooterIcons was deprecated in 1.35 and will omit warnings in 1.36 (T267447). The method was replaced with a method on the Skin in a move towards making classes that extend BaseTemplates logic-less and only concerned with the render of data.

Practically this means:

  • If you are calling $this->getFooterIcons() without arguments, make use of $this->get('footericons').
  • Skins that are using icononly or nocopyright parameters will need to make modifications to the return value of $this->get('footericons') like so:

with copyright parameter:

$footericons = $this->get('footericons');
unset( $footericons['copyright'] );

with icononly parameter

$footericons = $this->get('footericons');
foreach ( $footericons as $footerIconsKey => &$footerIconsBlock ) {
        foreach ( $footerIconsBlock as $footerIconKey => $footerIcon ) {
                if ( !isset( $footerIcon['src'] ) ) {
                        unset( $footerIconsBlock[$footerIconKey] );
                }
        }
}

SkinTemplate::getFooterIcons()` outside the template.

Impacted skins are tagged accordingly in case these skins want to ensure they do not omit warnings in the 1.36 release:
https://codesearch.wmcloud.org/search/?q=getFooterIcons&i=nope&files=&excludeFiles=&repos=

TODO

  • Fix skins
  • Remove the code

Details

Show related patches Customize query in gerrit

Event Timeline

Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

Change 677081 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/Refreshed@master] Don't throw warnings about deprecated things

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

Change 677081 merged by jenkins-bot:

[mediawiki/skins/Refreshed@master] Don't throw warnings about deprecated things

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

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

[mediawiki/core@master] Installer: Do not use `mediawiki.skinning.interface` module

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

Change 680784 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/GuMaxDD@master] Don't call deprecated BaseTemplate#getFooterIcons

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

Change 680785 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/WoOgLeShades@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 680786 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/Tempo@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 680787 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/Anisa@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 680788 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/Metrolook@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 680788 merged by jenkins-bot:

[mediawiki/skins/Metrolook@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 680704 had a related patch set uploaded (by Paladox; author: Jack Phoenix):

[mediawiki/skins/Metrolook@REL1_36] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 680704 merged by jenkins-bot:

[mediawiki/skins/Metrolook@REL1_36] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681301 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/Bouquet@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681304 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/Dusk@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681305 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/DuskToDawn@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681307 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/Gamepress@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681311 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/GreyStuff@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681312 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/Splash@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681312 merged by jenkins-bot:

[mediawiki/skins/Splash@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681311 merged by jenkins-bot:

[mediawiki/skins/GreyStuff@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681307 merged by jenkins-bot:

[mediawiki/skins/Gamepress@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681305 merged by jenkins-bot:

[mediawiki/skins/DuskToDawn@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681304 merged by jenkins-bot:

[mediawiki/skins/Dusk@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 681301 merged by jenkins-bot:

[mediawiki/skins/Bouquet@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 680785 merged by jenkins-bot:

[mediawiki/skins/WoOgLeShades@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 684026 had a related patch set uploaded (by Jack Phoenix; author: Jack Phoenix):

[mediawiki/skins/DeskMessMirrored@master] BaseTemplate::getFooterIcons was deprecated in MediaWiki 1.35

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

Dumb question, but is there any way to make this replacement work in 1.35 without doing a version compare first?

Specifically, how we can avoid triggering this:

Notice: Undefined index: alt in /media/zephyrnia/mediawiki/1.35/includes/skins/Skin.php on line 1007

This comment was removed by Isarra.
This comment was removed by Isarra.

Change 684026 merged by jenkins-bot:

[mediawiki/skins/DeskMessMirrored@master] BaseTemplate::getFooterIcons was deprecated in MediaWiki 1.35

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

Change 685061 had a related patch set uploaded (by Isarra; author: Jack Phoenix):

[mediawiki/core@REL1_35] Fix annoying E_NOTICE about undefined 'alt' index in Skin#makeFooterIcon.

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

So either we need to merge the above (and a followup patch) into 1.35, or we gotta go back and redo all the skins that assumed the footerIcons would actually be validated after getting them (which BaseTemplate did, but 'get' does not)...

...yeah there's no way to validate the icons in skin.php without actually moving the whole getFooterIcons function into that instead. 'get' definitely can't do this.

But I suppose if this is where you want all the things then perhaps it should be?

The issue is that previously skins using BaseTemplate::getFooterIcons could assume that what they got back was validated in that it was only icons, and that no empty or non-icon items would be returned. Given the mw installer/standard LocalSettings config also expect this and indirectly set unused default icons to empty arrays (so it's not exactly like sysadmins are likely to figure out some way to work around it on their end, either), this results in problems for any skins styling the footer icons in any way that assumes there actually be an icon there (for example padding, borders, spacers).

So now skins have to do the logic to check first themselves? I thought the overall goal moving forward was to reduce the amount of logic and code duplication needed inside skin files?

The logic check should still happening, it's just been abstracted away so skins don't have to worry about it. Skins should not have any need to validate.
Let me know if you have found a bug, by providing a LocalSettings configuration that doesn't work and I'll take a look.

Confusingly SkinTemplate:getFooterIcons has not been deprecated, only BaseTemplate::getFooterIcons has been.
Calling ->get('footericons') indirectly calls`SkinTemplate::getFooterIcons`
See: https://github.com/wikimedia/mediawiki/blob/ebd5b629b27871685dd535f0a3b02d89c4e5c647/includes/skins/SkinTemplate.php#L218 for the validation code.

Change 680787 merged by jenkins-bot:

[mediawiki/skins/Anisa@master] Avoid deprecated BaseTemplate#getFooterIcons

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

Change 680784 merged by jenkins-bot:

[mediawiki/skins/GuMaxDD@master] Don't call deprecated BaseTemplate#getFooterIcons

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

The logic check should still happening, it's just been abstracted away so skins don't have to worry about it. Skins should not have any need to validate.
Let me know if you have found a bug, by providing a LocalSettings configuration that doesn't work and I'll take a look.

Oh, cool, I hadn't realised it'd been fixed later.

Can you find and backport the other patch(es) that added the rest of the validation to 1.35? I haven't really tested most of the affected skins in anything after 1.35 as a lot of them are pretty broken by the features stuff, but given this is when it's supposedly deprecated since, it'd make it a lot easier to figure out what to merge if it actually fully worked in it!

Nope, nevermind, it's not actually catching the empty copyright array in any current version.

So localsettings does this when selecting no copyright:

$wgRightsPage = ""; # Set to the title of a wiki page that describes your license/copyright
$wgRightsUrl = "";
$wgRightsText = "";
$wgRightsIcon = "";

Which somewhere along the lines results in an empty array, which SkinTemplate::getFooterIcons isn't catching.

But I figured it out. I can now die satisfied, having wasted all evening on this, and who knows how many other hours of confusion and frustration, all because I was too lazy to set a copyright on a migration test wiki...

Change 685625 had a related patch set uploaded (by Isarra; author: Isarra):

[mediawiki/core@master] Filter out random garbage icons in getFooterIcons

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

Change 685604 had a related patch set uploaded (by Isarra; author: Isarra):

[mediawiki/core@REL1_35] Filter out random garbage icons in getFooterIcons

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

Change 685061 merged by jenkins-bot:

[mediawiki/core@REL1_35] Fix annoying E_NOTICE about undefined 'alt' index in Skin#makeFooterIcon.

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

Change 685625 merged by jenkins-bot:

[mediawiki/core@master] getFooterIcons should not return empty arrays, part 2, Electric Boogaloo

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

Change 685604 merged by jenkins-bot:

[mediawiki/core@REL1_35] getFooterIcons should not return empty arrays, part 2, Electric Boogaloo

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

Change 680786 merged by jenkins-bot:

[mediawiki/skins/Tempo@master] Avoid deprecated BaseTemplate#getFooterIcons

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