Page MenuHomePhabricator

CirrusSearch support for PHP 8.1
Closed, ResolvedPublic

Description

00:04:18.861 1) CirrusSearch\Maintenance\ScriptsRunnableTest::testScriptCanBeLoaded with data set #6 ('/workspace/src/extensions/Cir...ch.php')
00:04:18.861 Failed asserting that 255 is identical to 0.
00:04:18.861 
00:04:18.861 /workspace/src/extensions/CirrusSearch/tests/phpunit/integration/Maintenance/ScriptsRunnableTest.php:38
00:04:18.861 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:518
00:04:18.861 === Logs generated by test case
00:04:18.861 [objectcache] [debug] MainWANObjectCache using store {class} {"class":"EmptyBagOStuff"}

Event Timeline

The actual error that’s causing the maintenance script to exit nonzero is:

PHP Fatal error:  Cannot acquire reference to $GLOBALS in /workspace/src/extensions/CirrusSearch/maintenance/RunSearch.php on line 139

Apparently this maintenance script allows setting any config option (or other global, for that matter), recursively:

RunSearch::applyGlobals()
$forceChange = $this->getOption( 'i-know-what-im-doing', false );
foreach ( $options as $key => $value ) {
	if ( strpos( $key, '.' ) !== false ) {
		// key path
		$path = explode( '.', $key );
		if ( !isset( $changeable[$path[0]] ) ) {
			$this->error( "\nERROR: $key is not a globally changeable variable\n" );
		}
		$cur =& $GLOBALS;
		foreach ( $path as $pathel ) {
			if ( !array_key_exists( $pathel, $cur ) ) {
				$this->error( "\nERROR: $key is not a valid global variable path\n" );
				exit();
			}
			$cur =& $cur[$pathel];
		}
		$cur = $value;
	} elseif ( $forceChange || isset( $changeable[$key] ) ) {
		// This is different from the keypath case above in that this can set
		// variables that haven't been loaded yet. In particular at this point
		// in the MW load process explicitly configured variables are
		// available, but defaults from extension.json have not yet been
		// loaded.
		$GLOBALS[$key] = $value;
	} else {
		$this->error( "\nERROR: $key is not a globally changeable variable\n" );
		exit();
	}
}

And taking a reference to $GLOBALS isn’t allowed under PHP 8.1 anymore.

Do we really need this functionality at all? (It dates back to the initial change introducing the script, so CC @EBernhardson.) I don’t see any reference to the i-know-what-im-doing option guarding it in Puppet, at least.

Edit: It only allows setting globals from CirrusSearch’s own extension.json, at least. Yay.

Change 812401 had a related patch set uploaded (by Ebernhardson; author: MarkAHershberger):

[mediawiki/extensions/CirrusSearch@master] Avoid PHP fatal in php 8.1

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

This is used for experimenting with different variations of search, it wont be found in any automation. It's still used but not all that often. There is a patch, gerrit:812401 that was previously failing due to an unrelated test failure in Echo that I didn't understand. Checking back into this patch today it seems to be working, will need to get this reviewed now.

Change 854944 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/CirrusSearch@master] Cannot reference the entire &$GLOBALS any more in PHP 8.1

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

Change 812401 abandoned by MarkAHershberger:

[mediawiki/extensions/CirrusSearch@master] More code cleanup after fatal fix for PHP 8.1

Reason:

Ief8b8f6a3b1b5d4d46171e942a861949bb067afd

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

Change 812401 restored by MarkAHershberger:

[mediawiki/extensions/CirrusSearch@master] More code cleanup after fatal fix for PHP 8.1

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

Change 854944 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@master] Cannot reference the entire &$GLOBALS any more in PHP 8.1

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

Change 812401 merged by jenkins-bot:

[mediawiki/extensions/CirrusSearch@master] More code cleanup after fatal fix for PHP 8.1

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