Page MenuHomePhabricator
Create Task
Maniphest T270852

Dashes in AbuseFilter hook names cause issues with interfaces
Closed, ResolvedPublic
Actions

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 );

Details

Related Changes in Gerrit:
SubjectRepoBranchLines +/-
Hooks: Map dash character to underscore when generating hook namesmediawiki/coreREL1_35+2 -2
Hooks: Map dash character to underscore when generating hook namesmediawiki/coremaster+2 -2
Customize query in gerrit

Related Objects

Event Timeline

taavi created this task.Dec 27 2020, 12:28 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 27 2020, 12:28 PM
Reedy subscribed.Dec 27 2020, 9:32 PM

This feels... Dejavu.

Reedy added a comment.Dec 27 2020, 9:38 PM

Looks like there are 9 AbuseFilter- hooks...

Reedy added a comment.Dec 27 2020, 10:50 PM
$funcName = 'on' . str_replace( ':', '_',  ucfirst( $hook ) );

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

Daimona subscribed.Dec 30 2020, 1:00 PM
In T270852#6712641, @Reedy wrote:
$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.

Daimona added a project: Platform Engineering.Dec 30 2020, 1:00 PM
taavi added a comment.Dec 30 2020, 1:01 PM
In T270852#6714491, @Daimona wrote:
In T270852#6712641, @Reedy wrote:
$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()?

Daimona added a comment.Dec 30 2020, 1:03 PM
In T270852#6714494, @Majavah wrote:

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

Yeah, it would have to become

$funcName = 'on' . str_replace( [ ':', '-' ], [ '_', '' ],  ucfirst( $hook ) );
• holger.knust triaged this task as Low priority.Jan 5 2021, 9:23 PM
• holger.knust edited projects, added Platform Team Workboards (Clinic Duty Team); removed Platform Engineering.
• holger.knust moved this task from Inbox to Later on the Platform Team Workboards (Clinic Duty Team) board.
tstarling subscribed.Jan 5 2021, 9:33 PM

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).

gerritbot added a comment.Jan 6 2021, 2:00 PM

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

gerritbot added a project: Patch-For-Review.Jan 6 2021, 2:00 PM
Daimona claimed this task.Jan 6 2021, 2:00 PM
In T270852#6724023, @tstarling wrote:

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.

gerritbot added a comment.Jan 27 2021, 1:14 AM

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

gerritbot added a comment.Jan 27 2021, 1:19 AM

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

gerritbot added a comment.Jan 27 2021, 1:49 AM

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

ReleaseTaggerBot added projects: MW-1.35-notes, MW-1.36-notes (1.36.0-wmf.29; 2021-02-02).Jan 27 2021, 2:00 AM
Maintenance_bot removed a project: Patch-For-Review.Jan 27 2021, 2:10 AM
Daimona closed this task as Resolved.Feb 22 2023, 12:27 PM
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