Page MenuHomePhabricator
Create Task
Maniphest T291325

TypeError: Return value of BaseTemplate::getCopyrightIconHTML() must be of the type string, array returned
Closed, ResolvedPublicBUG REPORT
Actions

Description

List of steps to reproduce (step by step, including full links if applicable):

  • install MediaWiki
  • install the Minerva skin
  • configure $wgRightsIcon and $wgRightsText (e.g. "$wgResourceBasePath/resources/assets/licenses/cc-0.png" and "Creative Commons Zero (Public Domain)")
  • don’t customize $wgFooterIcons
  • load a page with the Minerva skin

To see it live:

  • Open Not Wikilambda with the Minerva skin and $wgFooterIcons not configured, using this link

What happens?:

MediaWiki internal error.

Original exception: [cd825e51c258fe8c7c6c8517] /w/index.php?title=Z10157&mobileaction=toggle_view_mobile TypeError: Return value of BaseTemplate::getCopyrightIconHTML() must be of the type string, array returned
Backtrace:
from /data/project/notwikilambda/public_html/w/includes/skins/BaseTemplate.php(60)
#0 /data/project/notwikilambda/public_html/w/includes/skins/SkinTemplate.php(382): BaseTemplate::getCopyrightIconHTML(GlobalVarConfig)
#1 /data/project/notwikilambda/public_html/w/skins/MinervaNeue/includes/Skins/SkinMinerva.php(125): SkinTemplate->prepareQuickTemplate()
#2 /data/project/notwikilambda/public_html/w/skins/MinervaNeue/includes/Skins/SkinMinerva.php(78): SkinMinerva->prepareQuickTemplate()
#3 /data/project/notwikilambda/public_html/w/includes/skins/SkinMustache.php(56): SkinMinerva->getTemplateData()
#4 /data/project/notwikilambda/public_html/w/includes/skins/SkinTemplate.php(144): SkinMustache->generateHTML()
#5 /data/project/notwikilambda/public_html/w/includes/OutputPage.php(2644): SkinTemplate->outputPage()
#6 /data/project/notwikilambda/public_html/w/includes/MediaWiki.php(939): OutputPage->output(boolean)
#7 /data/project/notwikilambda/public_html/w/includes/MediaWiki.php(952): MediaWiki::{closure}()
#8 /data/project/notwikilambda/public_html/w/includes/MediaWiki.php(559): MediaWiki->main()
#9 /data/project/notwikilambda/public_html/w/index.php(53): MediaWiki->run()
#10 /data/project/notwikilambda/public_html/w/index.php(46): wfIndexMain()
#11 {main}

What should have happened instead?:
No crash.

Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc:
MediaWiki core 01dea43, MinervaNeue be64114

I think this is a MediaWiki core bug. Here’s BaseTemplate::getCopyrightIconHTML():

	/**
	 * @internal for usage by BaseTemplate or SkinTemplate.
	 * @param Config $config
	 * @return string
	 */
	public static function getCopyrightIconHTML( Config $config ): string {
		$out = '';
		$footerIcons = $config->get( 'FooterIcons' );
		if (
			isset( $footerIcons['copyright']['copyright'] ) &&
			$footerIcons['copyright']['copyright']
		) {
			$out = $footerIcons['copyright']['copyright'];
		} elseif ( $config->get( 'RightsIcon' ) ) {
			$icon = htmlspecialchars( $config->get( 'RightsIcon' ) );
			$url = $config->get( 'RightsUrl' );
			if ( $url ) {
				$out .= '<a href="' . htmlspecialchars( $url ) . '">';
			}
			$text = htmlspecialchars( $config->get( 'RightsText' ) );
			$out .= "<img src=\"$icon\" alt=\"$text\" width=\"88\" height=\"31\" />";
			if ( $url ) {
				$out .= '</a>';
			}
		}
		return $out;
	}

This effectively returns $wgFooterIcons['copyright']['copyright']. And here’s the default value of $wgFooterIcons:

DefaultSettings.php
$wgFooterIcons = [
	"copyright" => [
		"copyright" => [], // placeholder for the built in copyright icon
	],
	"poweredby" => [
		"mediawiki" => [
			// Defaults to point at
			// "$wgResourceBasePath/resources/assets/poweredby_mediawiki_88x31.png"
			// plus srcset for 1.5x, 2x resolution variants.
			"src" => null,
			"url" => "https://www.mediawiki.org/",
			"alt" => "Powered by MediaWiki",
		]
	],
];

$wgFooterIcons['copyright']['copyright'] is an array. Returning it from getCopyrightIconHTML(), which declares a return type of string, will cause a crash.

Details

SubjectRepoBranchLines +/-
skins: Correctly index the copyright icon, caused failure in MFmediawiki/coremaster+31 -12
skins: Correctly index the copyright icon, caused failure in MFmediawiki/coreREL1_37+31 -12
BaseTemplate::getCopyrightIconHTML should always return HTMLmediawiki/coremaster+7 -4
Customize query in gerrit

Related Objects

Event Timeline

LucasWerkmeister created this task.Sep 18 2021, 1:13 PM
Restricted Application added subscribers: Masumrezarock100, Aklapper. · View Herald TranscriptSep 18 2021, 1:13 PM
LucasWerkmeister added subscribers: Jdlrobson, Ammarpad.Sep 18 2021, 1:16 PM

I suspect this was introduced in commit f9be83e310a5407db1ce101395fa6df0deb0966b, which added the static type hint. Previously, Skin::getCopyrightIcon() had a similar mistake in returning $wgFooterIcons['copyright']['copyright'] as null|string, but this return type was only in phpdoc, so it didn’t cause a crash.

LucasWerkmeister added a project: MW-1.37-release.Sep 18 2021, 1:19 PM

Tentatively marking as a 1.37 blocker, since the commit mentioned above has been backported to REL1_37.

$wgFooterIcons is not configured in Not Wikilambda’s LocalSettings.php, by the way. (You can see the whole file at ~tools.notwikilambda/public_html/w/LocalSettings.php on Toolforge.)

Ammarpad added a comment.Sep 19 2021, 10:23 AM

This is already being fixed at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/721770

Yes, the problem is not new, but it manifests now because of the added type hint.

LucasWerkmeister updated the task description. (Show Details)Sep 19 2021, 10:46 AM
Stashbot added a comment.Sep 19 2021, 10:47 AM

Mentioned in SAL (#wikimedia-cloud) [2021-09-19T10:47:21Z] <wm-bot> <lucaswerkmeister> configured $wgFooterIcons if notwikilambda-configure-wgFooterIcons URL parameter isn’t set to false, working around T291325

LucasWerkmeister added a comment.Sep 19 2021, 10:47 AM

I’ve deployed a workaround similar to the one from T284170#7129989 to Not Wikilambda:

if ( wfStringToBool( $_GET['notwikilambda-configure-wgFooterIcons'] ?? 'true' ) ) {
	$wgFooterIcons['copyright']['copyright'] = 'Workaround for <a href=https://phabricator.wikimedia.org/T291325>T291325</a>';
}
Ammarpad changed the task status from Open to In Progress.Sep 19 2021, 2:18 PM
Ammarpad assigned this task to xSavitar.
Restricted Application added a project: User-xSavitar. · View Herald TranscriptSep 19 2021, 2:18 PM
LucasWerkmeister updated the task description. (Show Details)Sep 19 2021, 2:21 PM
gerritbot added a comment.Sep 20 2021, 10:14 AM

Change 721770 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):

[mediawiki/core@master] skins: Correctly index the copyright icon, caused failure in MF

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

gerritbot added a project: Patch-For-Review.Sep 20 2021, 10:14 AM
gerritbot added a comment.Sep 20 2021, 3:29 PM

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

[mediawiki/core@master] BaseTemplate::getCopyrightIconHTML should always return HTML

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

Jdlrobson added a comment.Sep 20 2021, 3:29 PM

I was having difficulty replicating and thus working out the best solution here. I'm running 7.4.12, where empty arrays should be falsey. When I follow the current steps in the description, I'm seeing it as an empty array:

<?php
$arr = [];
var_dump( !!$arr );
`

Is there another step I should be taking to replicate this locally? e.g. setting $wgRightsIcon and $wgRightsText ?
https://gerrit.wikimedia.org/g/mediawiki/core/+/bf281870028648b8131014399306258f03ed11e2/includes/Setup.php#295

I can only replicate it when I do:

$wgFooterIcons['copyright'] = [
	'copyright' => [
		'url' => '/copyright',
		'src' => 'foo.gif'
	]
];

The Cavendish and Webplatform skins seem to be the only skins that actively make use of the 'copyrightico' property on skins and they both have handling for HTML strings and data to be passed to Skin::makeFooterIcon. I suggest calling that inside the newly added method as I do in https://gerrit.wikimedia.org/r/722383.

LucasWerkmeister added a comment.Sep 20 2021, 6:44 PM

I don’t see what the array being truthy or falsy has to do with this bug…? You can’t return an array as a string, even an empty array:

$ php -r 'function f(): string { return []; } f();'
PHP Fatal error:  Uncaught TypeError: f(): Return value must be of type string, array returned in Command line code:1
Stack trace:
#0 Command line code(1): f()
#1 {main}
  thrown in Command line code on line 1
Jdlrobson added a comment.Sep 20 2021, 8:40 PM

I don’t see what the array being truthy or falsy has to do with this bug…?

It doesn't have anything to do with the bug. I just couldn't replicate this with the replication steps in the task description. The default is an empty array, so according to the code, an empty array can never be returned. Only when I set $wgRightsIcon and $wgRightsText do I get the error.

gerritbot added a comment.Sep 20 2021, 9:07 PM

Change 722383 abandoned by Jdlrobson:

[mediawiki/core@master] BaseTemplate::getCopyrightIconHTML should always return HTML

Reason:

see https://gerrit.wikimedia.org/r/c/mediawiki/core/+/721770

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

LucasWerkmeister updated the task description. (Show Details)Sep 20 2021, 9:10 PM

Ah, I see. My apologies – I’d noticed in shell.php that the default copyright array wasn’t empty (despite not being configured directly in LocalSettings.php), but I didn’t realize that it was populated from other $wgRights* variables, and that in a fully default MediaWiki install the $out = $footerIcons['copyright']['copyright']; branch wouldn’t be taken. Hopefully it’s better now?

Jdlrobson added a comment.Sep 20 2021, 9:35 PM

Glad we're on the same page! Definite bug, and glad there's not some other weird variant of this :-)
@xSavitar let me know if you need any help with the patchset. This should definitely be backported to 1.37.

xSavitar added a comment.Sep 20 2021, 10:14 PM

Thank you @Jdlrobson & @LucasWerkmeister for the discussion. I've read and updated the patch. Looking forward to reviews :]

gerritbot added a comment.Sep 21 2021, 1:25 AM

Change 722452 had a related patch set uploaded (by Jdlrobson; author: Derick Alangi):

[mediawiki/core@REL1_37] skins: Correctly index the copyright icon, caused failure in MF

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

gerritbot added a comment.Sep 21 2021, 1:38 AM

Change 722452 merged by jenkins-bot:

[mediawiki/core@REL1_37] skins: Correctly index the copyright icon, caused failure in MF

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

gerritbot added a comment.Sep 21 2021, 1:46 AM

Change 721770 merged by jenkins-bot:

[mediawiki/core@master] skins: Correctly index the copyright icon, caused failure in MF

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

Maintenance_bot removed a project: Patch-For-Review.Sep 21 2021, 2:10 AM
Jdlrobson added a comment.Sep 21 2021, 3:00 AM

@LucasWerkmeister could you please confirm this is fixed on your wiki now without your temporary fix and resolve this ticket if so?

ReleaseTaggerBot added projects: MW-1.37-notes, MW-1.38-notes (1.38.0-wmf.2; 2021-09-28).Sep 21 2021, 4:00 PM
Stashbot added a comment.Sep 21 2021, 7:29 PM

Mentioned in SAL (#wikimedia-cloud) [2021-09-21T19:29:52Z] <wm-bot> <lucaswerkmeister> undeployed workaround for T291325, seems to be fixed

LucasWerkmeister closed this task as Resolved.Sep 21 2021, 7:30 PM

Yup, seems to work fine! Thanks everyone!

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits