Page MenuHomePhabricator

BaseTemplate::getFooterIcons is deprecated, use $this->get('footericons') instead
Closed, ResolvedPublic

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=-%3EgetFooterIcons%5C(&i=nope&files=&excludeFiles=&repos=

TODO

  • Fix skins
  • Remove the code

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
mediawiki/skins/Pivotmaster+20 -3
mediawiki/skins/PivotREL1_39+20 -3
mediawiki/skins/apexmaster+40 -69
mediawiki/skins/apexREL1_39+40 -69
mediawiki/skins/HasSomeColoursREL1_39+14 -21
mediawiki/skins/HasSomeColoursmaster+14 -21
mediawiki/skins/AmethystREL1_39+12 -4
mediawiki/skins/Amethystmaster+12 -4
mediawiki/skins/Foregroundmaster+18 -12
mediawiki/skins/BlueSkymaster+14 -12
mediawiki/coremaster+29 -11
mediawiki/skins/mediawiki-strappingmaster+13 -4
mediawiki/coremaster+88 -0
mediawiki/skins/Tempomaster+5 -2
mediawiki/coreREL1_35+6 -0
mediawiki/coreREL1_35+6 -6
mediawiki/skins/GuMaxDDmaster+3 -1
mediawiki/skins/Anisamaster+9 -7
mediawiki/skins/DeskMessMirroredmaster+3 -1
mediawiki/skins/WoOgLeShadesmaster+9 -7
mediawiki/skins/Bouquetmaster+10 -5
mediawiki/skins/Duskmaster+9 -4
mediawiki/skins/DuskToDawnmaster+10 -5
mediawiki/skins/GreyStuffmaster+10 -8
mediawiki/skins/Gamepressmaster+10 -5
mediawiki/skins/Splashmaster+10 -8
mediawiki/skins/MetrolookREL1_36+6 -3
mediawiki/skins/Metrolookmaster+6 -3
mediawiki/skins/Refreshedmaster+13 -2
Show related patches Customize query in gerrit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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

[mediawiki/skins/mediawiki-strapping@master] Fix strapping language section rendering and footer deprecation warning

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

Change 711496 merged by jenkins-bot:

[mediawiki/skins/mediawiki-strapping@master] Fix strapping language section rendering and footer deprecation warning

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

Hey there. This task is proposed as a blocker to MediaWiki 1.37, which will be cut in less than three weeks' time. Please consider whether this will make that deadline, and if not, move it to block the MediaWiki 1.38 release (MW-1.38-release) or remove as a blocker entirely.

Not enough skins have migrated over for my liking, so happy to bump this to 1.38 since losing 8 skins is basically 10% of all the known available MediaWiki skins (8/76).

I ran into an issue following the deprecation notice for a skin as it seems BaseTemplate::getFooter which is not deprecated calls getFooterIcons. Given footericons contains icononly by default, I believe that code in core needs to be rewritten like so:

	/**
	 * Renderer for getFooterIcons and getFooterLinks
	 *
	 * @param string $iconStyle $option for getFooterIcons: "icononly", "nocopyright"
	 * @param string $linkStyle $option for getFooterLinks: "flat"
	 *
	 * @return string html
	 * @since 1.29
	 */
	protected function getFooter( $iconStyle = 'icononly', $linkStyle = 'flat' ) {
		$validFooterIcons = $this->get( 'footericons' );
		if ( $iconStyle === 'copyright' ) {
			unset( $validFooterIcons['copyright'] );
		}
		$validFooterLinks = $this->getFooterLinks( $linkStyle );

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

[mediawiki/core@master] BaseTemplate::getFooter should not trigger deprecation warning

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

Change 802560 merged by jenkins-bot:

[mediawiki/core@master] BaseTemplate::getFooter should not trigger deprecation warning

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

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

[mediawiki/skins/BlueSky@master] Fixes template warnings (getFooterIcons, headelement)

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

Change 804625 merged by Jdlrobson:

[mediawiki/skins/BlueSky@master] Fixes template warnings (getFooterIcons, headelement)

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

Change 831040 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/skins/Foreground@master] Fix deprecations and skin config

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

Change 831040 merged by jenkins-bot:

[mediawiki/skins/Foreground@master] Remove usages of deprecated BaseTemplate methods

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

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

[mediawiki/skins/Amethyst@master] Upgrade for 1.39

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

Change 831641 merged by jenkins-bot:

[mediawiki/skins/Amethyst@master] Upgrade for 1.39

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

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

[mediawiki/skins/Amethyst@REL1_39] Upgrade for 1.39

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

Change 831849 merged by jenkins-bot:

[mediawiki/skins/Amethyst@REL1_39] Upgrade for 1.39

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

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

[mediawiki/skins/HasSomeColours@master] Upgrade for 1.39

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

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

[mediawiki/skins/GreyStuff@master] Upgrade for 1.39

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

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

[mediawiki/skins/HasSomeColours@REL1_39] Upgrade for 1.39

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

Change 831915 merged by jenkins-bot:

[mediawiki/skins/HasSomeColours@master] Upgrade for 1.39

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

Change 831851 merged by jenkins-bot:

[mediawiki/skins/HasSomeColours@REL1_39] Upgrade for 1.39

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

GuMaxDD was fixed in 957bfb90
BlueSky in a7b60a0d
Metrolook in e33e891d
Poncho doesn't seem to use the function

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

[mediawiki/skins/Pivot@master] Fixes for 1.39

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

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

[mediawiki/skins/Pivot@REL1_39] Fixes for 1.39

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

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

[mediawiki/skins/apex@master] Upgrade for 1.39

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

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

[mediawiki/skins/apex@REL1_39] Upgrade for 1.39

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

Change 832308 merged by jenkins-bot:

[mediawiki/skins/apex@REL1_39] Upgrade for 1.39

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

Change 832296 merged by jenkins-bot:

[mediawiki/skins/apex@master] Upgrade for 1.39

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

Change 832306 merged by Jforrester:

[mediawiki/skins/Pivot@REL1_39] Fixes for 1.39

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

Change 832286 merged by Jforrester:

[mediawiki/skins/Pivot@master] Fixes for 1.39

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