Page MenuHomePhabricator

IndexLayout::getTabPanel() and ::setTabPanel() do not validate their argument and can emit PHP notices
Closed, ResolvedPublic

Description

If you run the following code:

$l = new IndexLayout();
$l->setTabPanel( 'foobar' );

it will result in a PHP notice:

Notice: Undefined index: foobar in path/to/IndexLayout.php on line 120

And you'd get a similar notice with getTabPanel.

I think if the panel is not defined, these methods should either throw an exception or use a fallback, but not emit any PHP notices if the element does not exist in the array. I would say that throwing an exception would be better than accepting garbage, but the JS counterpart doesn't throw any error, so perhaps the fallback would be better.

Also, the code and documentation are slightly confusing:

/**
 * @return TabPanelLayout Tab panel, if found
 */
public function getTabPanel( $name ) {
	return $this->tabPanels[$name];
}

The return type should be nullable here, especially because it says "if found" (although it doesn't actually check whether it was found).

public function setTabPanel( $name ) {
	// ...
	$tabPanel = $this->getTabPanel( $name );
	// ...
	if ( $tabPanel ) {
		// ...
	}
}

The code also expects $tabPanel to be falsey, which agrees with the comment but not with the documentation.

Without diving deep into git blame, my first guess is that this code was ported from JavaScript, where accessing unset array elements does not emit any error, and that index checks were simply forgotten in the PHP code.

Event Timeline

Change 852292 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[oojs/ui@master] Avoid PHP notice in IndexLayout::getTabPanel()

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

Change 852292 merged by jenkins-bot:

[oojs/ui@master] Avoid PHP notice in IndexLayout::getTabPanel()

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

@Volker_E @matmarex Hi! Not sure who to ping about this, but I was wondering if this task can be closed as resolved, or if it should stay open until a new version of OOUI is released; I'm not familiar with the task lifecycle and release process. TIA!

Volker_E edited projects, added OOUI (OOUI-0.45.0); removed OOUI.
Volker_E triaged this task as Medium priority.

This is going out with next OOUI version and there will be a ping on task when the release is added to MediaWiki core.
Thanks @Daimona!

Change 865677 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/core@master] Update OOUI to v0.46.0

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

Change 865677 merged by jenkins-bot:

[mediawiki/core@master] Update OOUI to v0.46.0

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