Page MenuHomePhabricator

Dashes in AbuseFilter hook names cause issues with interfaces
Open, LowPublic

Description

I ran into some issues when working on T270846. When using the following partial extension.json and properly autoloaded CentralAuthAbuseFilterHooks.php:

{
	"Hooks": {
		"AbuseFilter-builder": "abusefilter"
	},
	"HookHandlers": {
		"abusefilter": {
			"class": "CentralAuthAbuseFilterHooks"
		}
	}
}
<?php

use MediaWiki\Extension\AbuseFilter\Hooks\AbuseFilterBuilderHook;

/**
 * CentralAuth hook runners for the AbuseFilter extension
 */
class CentralAuthAbuseFilterHooks implements AbuseFilterBuilderHook {
	/**
	 * Tell AbuseFilter about our global_user_groups variable
	 *
	 * @param array $realValues
	 *
	 * @return bool
	 */
	public function onAbuseFilterBuilder( array &$realValues ) {
		// Uses: 'abusefilter-edit-builder-vars-global-user-groups'
		$builderValues['vars']['global_user_groups'] = 'global-user-groups';
		return true;
	}
}

Now when I navigate to http://a.mediawiki.test/wiki/Special:AbuseFilter/new I will see the following exception:

[e3d59c98a38d9cea530c97ff] /wiki/Special:AbuseFilter/new Error from line 163 of /home/taavi/src/mediawiki/includes/HookContainer/HookContainer.php: Call to undefined method CentralAuthAbuseFilterHooks::onAbuseFilter-builder()

Backtrace:

#0 /home/taavi/src/mediawiki/extensions/AbuseFilter/includes/Hooks/AbuseFilterHookRunner.php(67): MediaWiki\HookContainer\HookContainer->run(string, array)
#1 /home/taavi/src/mediawiki/extensions/AbuseFilter/includes/KeywordsManager.php(217): MediaWiki\Extension\AbuseFilter\Hooks\AbuseFilterHookRunner->onAbuseFilterBuilder(array)
#2 /home/taavi/src/mediawiki/extensions/AbuseFilter/includes/EditBoxBuilder.php(133): MediaWiki\Extension\AbuseFilter\KeywordsManager->getBuilderValues()
#3 /home/taavi/src/mediawiki/extensions/AbuseFilter/includes/View/AbuseFilterViewEdit.php(378): MediaWiki\Extension\AbuseFilter\EditBoxBuilder->buildEditBox(string, boolean)
#4 /home/taavi/src/mediawiki/extensions/AbuseFilter/includes/View/AbuseFilterViewEdit.php(178): MediaWiki\Extension\AbuseFilter\View\AbuseFilterViewEdit->buildFilterEditor(NULL, MediaWiki\Extension\AbuseFilter\Filter\MutableFilter, NULL, NULL)
#5 /home/taavi/src/mediawiki/extensions/AbuseFilter/includes/special/SpecialAbuseFilter.php(145): MediaWiki\Extension\AbuseFilter\View\AbuseFilterViewEdit->show()
#6 /home/taavi/src/mediawiki/includes/specialpage/SpecialPage.php(645): SpecialAbuseFilter->execute(string)
#7 /home/taavi/src/mediawiki/includes/specialpage/SpecialPageFactory.php(1404): SpecialPage->run(string)
#8 /home/taavi/src/mediawiki/includes/MediaWiki.php(310): MediaWiki\SpecialPage\SpecialPageFactory->executePath(Title, RequestContext)
#9 /home/taavi/src/mediawiki/includes/MediaWiki.php(945): MediaWiki->performRequest()
#10 /home/taavi/src/mediawiki/includes/MediaWiki.php(548): MediaWiki->main()
#11 /home/taavi/src/mediawiki/index.php(53): MediaWiki->run()
#12 /home/taavi/src/mediawiki/index.php(46): wfIndexMain()
#13 {main}

onAbuseFilter-builder() isn't a valid PHP method name and AbuseFilterBuilderHook suggests gives the following signature:

public function onAbuseFilterBuilder( array &$realValues );

Event Timeline

Looks like there are 9 AbuseFilter- hooks...

$funcName = 'on' . str_replace( ':', '_',  ucfirst( $hook ) );

I imagine we will want to end up doing something similar for - as we do/did for :

$funcName = 'on' . str_replace( ':', '_',  ucfirst( $hook ) );

I imagine we will want to end up doing something similar for - as we do/did for :

Or we might rename legacy hooks with weird names... Which is quite painful though, so +1 to this hack.

$funcName = 'on' . str_replace( ':', '_',  ucfirst( $hook ) );

I imagine we will want to end up doing something similar for - as we do/did for :

Or we might rename legacy hooks with weird names... Which is quite painful though, so +1 to this hack.

Wouldn't that create onAbuseFilter_builder() instead of the wanted onAbuseFilterBuilder()?

Wouldn't that create onAbuseFilter_builder() instead of the wanted onAbuseFilterBuilder()?

Yeah, it would have to become

$funcName = 'on' . str_replace( [ ':', '-' ], [ '_', '' ],  ucfirst( $hook ) );

I think it should be strtr( ucfirst( $hook ), ':-', '__' ), and you will have onAbuseFilter_builder(). Replacing the hyphen with an empty string would mean that you can't deprecate and rename the non-canonical hook name. Daniel has been talking about deprecating all the hook names with colons in them, in the context of T258665.

Note that this code is hot (performance sensitive).

Change 654614 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/core@master] Map dash character to underscore when generating hook names

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

I think it should be strtr( ucfirst( $hook ), ':-', '__' ), and you will have onAbuseFilter_builder(). Replacing the hyphen with an empty string would mean that you can't deprecate and rename the non-canonical hook name. Daniel has been talking about deprecating all the hook names with colons in them, in the context of T258665.

Ah, good point.

Note that this code is hot (performance sensitive).

Rewritten using strtr.

Change 654614 merged by jenkins-bot:
[mediawiki/core@master] Hooks: Map dash character to underscore when generating hook names

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

Change 658617 had a related patch set uploaded (by Reedy; owner: Daimona Eaytoy):
[mediawiki/core@REL1_35] Hooks: Map dash character to underscore when generating hook names

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

Change 658617 merged by jenkins-bot:
[mediawiki/core@REL1_35] Hooks: Map dash character to underscore when generating hook names

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