Page MenuHomePhabricator

Refactor modules related to SpecialAbuseFilter.php
Open, Needs TriagePublic

Description

I've recently started refactoring the code around AbuseFilter's revert functionality, and that work has uncovered a few more opportunities for refactoring around SpecialAbuseFilter.php:

1. Separate integration tests in SpecialAbuseFilterTest.php into View/?

Currently, tests for subclasses of AbuseFilterView are all packed into a single integration test file, namely SpecialAbuseFilterTest.php. This file has grown quite large, making it difficult to locate which functionality is tested where.
Since comprehensive test coverage is important when refactoring, these tests could be split into separate files under tests/phpunit/integration/View/, making them easier to navigate and maintain.

2. Do away with AbuseFilterView::$mParams?

AbuseFilterView is an abstract class subclassed by several classes, and ::$mParams is a protected property of it. However, the contents of this array differ significantly depending on the subclass, and because ::$mParams is just an array with no documented structure, it is not immediately clear what values it contains when debugging a View class.
This suggests that we could remove this protected property and initialize the required data directly in each subclass constructor. For example, AbuseFilterViewEdit's constructor looks like:

	public function __construct(
		// ...
		string $basePageName,
		array $params
	) {
		parent::__construct( $afPermManager, $context, $linkRenderer, $basePageName, $params );
		$this->specsFormatter->setMessageLocalizer( $this->getContext() );
		$this->filter = $this->mParams['filter'];
		$this->historyID = $this->mParams['history'] ?? null;
	}

::$mParams is not referenced anywhere else in the class, so we could simply initialize the required properties directly from $params instead of storing the entire array in the parent class.

3. Simplify title handling in AbuseFilterView?

While working on the edit success message flow in T381343, I noticed a few issues around title generation.

AbuseFilterView::getTitle() initializes the $subpage parameter to an empty string rather than false or null, causing calls such as $this->getTitle()->getLocalURL() to generate Special:AbuseFilter/ instead of Special:AbuseFilter. Although this is currently harmless, it differs from the behaviour of SpecialPage::getTitleFor().

AbuseFilterView.php
	/**
	 * @param string|int $subpage
	 * @return Title
	 */
	public function getTitle( $subpage = '' ) {
		return SpecialPage::getTitleFor( $this->basePageName, $subpage );
	}

In addition, AbuseFilterView::$basePageName appears to always be 'AbuseFilter', since view classes are instantiated exclusively via SpecialAbuseFilter::instantiateView(). This suggests that the property may be unnecessary.

The logic in AbuseFilterView::getTitle() is also duplicated by SpecialAbuseFilter::getTitleForSubpage(), meaning the same issue currently exists in two places. It may be possible to consolidate these implementations.

SpecialAbuseFilter.php
	/**
	 * Static variant to get the associated Title.
	 *
	 * @param string|int $subpage
	 * @return Title
	 */
	public static function getTitleForSubpage( $subpage ): Title {
		return self::getTitleFor( self::PAGE_NAME, $subpage );
	}
Acceptance criteria
  • Split SpecialAbuseFilterTest into view-specific integration test classes
  • Initialize view-specific state directly in each subclass instead of using AbuseFilterView::$mParams
  • Revisit title generation in AbuseFilterView and remove duplicated logic

Please feel free to update the task description if you find more things that could be improved.

Event Timeline

Change #1319915 had a related patch set uploaded (by Dragoniez; author: Dragoniez):

[mediawiki/extensions/AbuseFilter@master] [WIP] Split SpecialAbuseFilterTest

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

Change #1319915 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Split SpecialAbuseFilterTest tests

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

Dragoniez updated the task description. (Show Details)
Dragoniez updated the task description. (Show Details)

Change #1322654 had a related patch set uploaded (by Dragoniez; author: Dragoniez):

[mediawiki/extensions/AbuseFilter@master] Avoid broad `@covers` annotations in SpecialAbuseFilterTest

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

Change #1322654 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] Avoid broad `@covers` annotations in SpecialAbuseFilterTest

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

Change #1323463 had a related patch set uploaded (by Dragoniez; author: Dragoniez):

[mediawiki/extensions/AbuseFilter@master] Use SpecialAbuseFilter title helper for view subpages

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

Change #1323528 had a related patch set uploaded (by Dragoniez; author: Dragoniez):

[mediawiki/extensions/AbuseFilter@master] Remove `::$basePageName` from `AbuseFilterView` and `AbuseLogPager`

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