Represent new filters with null instead of 'new'

PHP is not strongly typed, so it's not a good idea to use scalars of
different types (here it's an integer vs the string 'new') to represent
different possibilities. This can have bad effects when type juggling
occurs, and it's also harder to figure out what the type of the
parameter can be (because a numeric ID might have been passed as a
string). Using integer vs null avoids all of this, and also allows us to
use nullable typehints.

These changes were partly copied from
If981cb35bf19a8469aa6c43c907e107cf8c65bc2 and should help with the
migration to the Filter value objects.

Change-Id: I8837d46c3c33761fea53f67b530b721dc7bd49b0

includes/AbuseFilter.php

@@ -855,7 +855,7 @@ class AbuseFilter {
 	 *  - Fatal in case of a permission-related error
 	 *
 	 * @param User $user
-	 * @param int|string $filter
+	 * @param int|null $filter
 	 * @param stdClass $newRow
 	 * @param array $actions
 	 * @param stdClass $originalRow
@@ -867,7 +867,7 @@ class AbuseFilter {
 	 */
 	public static function saveFilter(
 		User $user,
-		$filter,
+		?int $filter,
 		stdClass $newRow,
 		array $actions,
 		stdClass $originalRow,
@@ -1003,7 +1003,7 @@ class AbuseFilter {
 	 * @param User $user
 	 * @param stdClass $newRow
 	 * @param array $differences
-	 * @param int|string $filter
+	 * @param int|null $filter
 	 * @param array $actions
 	 * @param bool $wasGlobal
 	 * @param IDatabase $dbw DB_MASTER where the filter will be saved
@@ -1014,7 +1014,7 @@ class AbuseFilter {
 		User $user,
 		$newRow,
 		$differences,
-		$filter,
+		?int $filter,
 		$actions,
 		$wasGlobal,
 		IDatabase $dbw,
@@ -1031,13 +1031,8 @@ class AbuseFilter {
 		$dbw->startAtomic( __METHOD__ );
 
 		// Insert MAIN row.
-		if ( $filter === 'new' ) {
-			$new_id = null;
-			$is_new = true;
-		} else {
-			$new_id = $filter;
-			$is_new = false;
-		}
+		$is_new = $filter === null;
+		$new_id = $filter;
 
 		// Reset throttled marker, if we're re-enabling it.
 		$newRow['af_throttled'] = $newRow['af_throttled'] && !$newRow['af_enabled'];
@@ -1065,7 +1060,7 @@ class AbuseFilter {
 
 			if ( $enabled ) {
 				$parameters = $actions[$action];
-				if ( $action === 'throttle' && $parameters[0] === 'new' ) {
+				if ( $action === 'throttle' && $parameters[0] === null ) {
 					// FIXME: Do we really need to keep the filter ID inside throttle parameters?
 					// We'd save space, keep things simpler and avoid this hack. Note: if removing
 					// it, a maintenance script will be necessary to clean up the table.
@@ -1113,7 +1108,7 @@ class AbuseFilter {
 		// Do the update
 		$dbw->insert( 'abuse_filter_history', $afh_row, __METHOD__ );
 		$history_id = $dbw->insertId();
-		if ( $filter !== 'new' ) {
+		if ( $filter !== null ) {
 			$dbw->delete(
 				'abuse_filter_action',
 				[ 'afa_filter' => $filter ],
@@ -1136,7 +1131,7 @@ class AbuseFilter {
 		}
 
 		// Logging
-		$subtype = $filter === 'new' ? 'create' : 'modify';
+		$subtype = $filter === null ? 'create' : 'modify';
 		$logEntry = new ManualLogEntry( 'abusefilter', $subtype );
 		$logEntry->setPerformer( $user );
 		$logEntry->setTarget(

includes/FilterProfiler.php

@@ -77,10 +77,10 @@ class FilterProfiler {
 	/**
 	 * Retrieve per-filter statistics.
 	 *
-	 * @param string $filter
+	 * @param string|int $filter
 	 * @return array
 	 */
-	public function getFilterProfile( string $filter ) : array {
+	public function getFilterProfile( $filter ) : array {
 		$profile = $this->objectStash->get( $this->filterProfileKey( $filter ) );
 
 		if ( $profile !== false ) {

includes/Views/AbuseFilterView.php

@@ -5,7 +5,7 @@ use Wikimedia\Rdbms\IDatabase;
 
 abstract class AbuseFilterView extends ContextSource {
 	/**
-	 * @var string|null The ID of the current filter
+	 * @var string|int|null The ID of the current filter, null if new
 	 */
 	public $mFilter;
 	/**

includes/Views/AbuseFilterViewDiff.php

@@ -97,12 +97,12 @@ class AbuseFilterViewDiff extends AbuseFilterView {
 	public function loadData() {
 		$oldSpec = $this->mParams[3];
 		$newSpec = $this->mParams[4];
-		$this->mFilter = $this->mParams[1];
 
-		if ( !is_numeric( $this->mFilter ) ) {
+		if ( !is_numeric( $this->mParams[1] ) ) {
 			$this->getOutput()->addWikiMsg( 'abusefilter-diff-invalid' );
 			return false;
 		}
+		$this->mFilter = (int)$this->mParams[1];
 
 		$this->mOldVersion = $this->loadSpec( $oldSpec, $newSpec );
 		$this->mNewVersion = $this->loadSpec( $newSpec, $oldSpec );

includes/Views/AbuseFilterViewEdit.php

@@ -31,7 +31,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 		$out->addHelpLink( 'Extension:AbuseFilter/Rules format' );
 
 		$filter = $this->mFilter;
-		if ( !is_numeric( $filter ) && $filter !== 'new' ) {
+		if ( !is_numeric( $filter ) && $filter !== null ) {
 			$out->addHTML(
 				Xml::tags(
 					'p',
@@ -64,7 +64,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 
 		$canEdit = AbuseFilter::canEdit( $user );
 
-		if ( $filter === 'new' && !$canEdit ) {
+		if ( $filter === null && !$canEdit ) {
 			$out->addHTML(
 				Xml::tags(
 					'p',
@@ -120,10 +120,10 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 	}
 
 	/**
-	 * @param int|string $filter The filter ID or 'new'.
+	 * @param int|null $filter The filter ID or null for a new filter
 	 * @param int|null $history_id The history ID of the filter, if applicable. Otherwise null
 	 */
-	private function saveCurrentFilter( $filter, $history_id ) : void {
+	private function saveCurrentFilter( ?int $filter, $history_id ) : void {
 		$out = $this->getOutput();
 		$reqStatus = $this->loadRequest( $filter );
 		if ( !$reqStatus->isGood() ) {
@@ -179,21 +179,16 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 	 * @param string|null $error An error message to show above the filter box.
 	 * @param stdClass|null $row abuse_filter row representing this filter, null if it doesn't exist
 	 * @param array $actions Actions enabled and their parameters
-	 * @param int|string $filter The filter ID or 'new'.
+	 * @param int|null $filter The filter ID, or null for a new filter
 	 * @param int|null $history_id The history ID of the filter, if applicable. Otherwise null
 	 */
 	protected function buildFilterEditor(
 		$error,
 		?stdClass $row,
 		array $actions,
-		$filter,
+		?int $filter,
 		$history_id
 	) {
-		if ( $filter === null ) {
-			return;
-		}
-
-		// Build the edit form
 		$out = $this->getOutput();
 		$out->enableOOUI();
 		$out->addJsConfigVars( 'isFilterEditor', true );
@@ -203,7 +198,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 		if (
 			$row === null ||
 			// @fixme Temporary stopgap for T237887
-			( $history_id && $row->af_id !== $filter )
+			( $history_id && (int)$row->af_id !== $filter )
 		) {
 			$out->addHTML(
 				Xml::tags(
@@ -222,14 +217,14 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 		}
 
 		$out->addSubtitle( $this->msg(
-			$filter === 'new' ? 'abusefilter-edit-subtitle-new' : 'abusefilter-edit-subtitle',
-			$filter === 'new' ? $filter : $this->getLanguage()->formatNum( $filter ),
+			$filter === null ? 'abusefilter-edit-subtitle-new' : 'abusefilter-edit-subtitle',
+			$filter === null ? $filter : $this->getLanguage()->formatNum( $filter ),
 			$history_id
 		)->parse() );
 
 		// Hide hidden filters.
 		if (
-			( $row->af_hidden || ( $filter !== 'new' && AbuseFilter::filterHidden( $filter ) ) ) &&
+			( $row->af_hidden || ( $filter !== null && AbuseFilter::filterHidden( $filter ) ) ) &&
 			!AbuseFilter::canViewPrivate( $user )
 		) {
 			$out->addHTML( $this->msg( 'abusefilter-edit-denied' )->escaped() );
@@ -256,9 +251,9 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 		$fields = [];
 
 		$fields['abusefilter-edit-id'] =
-			$this->mFilter === 'new' ?
+			$this->mFilter === null ?
 				$this->msg( 'abusefilter-edit-new' )->escaped() :
-				$lang->formatNum( $filter );
+				$lang->formatNum( (string)$filter );
 		$fields['abusefilter-edit-description'] =
 			new OOUI\TextInputWidget( [
 				'name' => 'wpFilterDescription',
@@ -306,7 +301,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 			$fields['abusefilter-edit-hitcount'] = $hitCount;
 		}
 
-		if ( $filter !== 'new' && $row->af_enabled ) {
+		if ( $filter !== null && $row->af_enabled ) {
 			// Statistics
 			list( $totalCount, $matchesCount, $avgTime, $avgCond ) =
 				AbuseFilterServices::getFilterProfiler()->getFilterProfile( $filter );
@@ -412,7 +407,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 
 		$fields['abusefilter-edit-flags'] = $flags;
 
-		if ( $filter !== 'new' ) {
+		if ( $filter !== null ) {
 			$tools = '';
 			if ( MediaWikiServices::getInstance()->getPermissionManager()
 				->userHasRight( $user, 'abusefilter-revert' )
@@ -500,9 +495,10 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 			);
 		}
 
+		$urlFilter = $filter === null ? 'new' : (string)$filter;
 		$form = Xml::tags( 'form',
 			[
-				'action' => $this->getTitle( $filter )->getFullURL(),
+				'action' => $this->getTitle( $urlFilter )->getFullURL(),
 				'method' => 'post',
 				'id' => 'mw-abusefilter-editing-form'
 			],
@@ -1104,12 +1100,12 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 
 	/**
 	 * Loads filter data from the database by ID.
-	 * @param int|string $id The filter's ID number, or 'new'
+	 * @param int|null $id The filter's ID number, or null for a new filter
 	 * @return array|null Either a [ DB row, actions ] array representing the filter,
 	 *  or NULL if the filter does not exist.
 	 */
-	public function loadFilterData( $id ) {
-		if ( $id === 'new' ) {
+	public function loadFilterData( ?int $id ) {
+		if ( $id === null ) {
 			return [
 				(object)[
 					'af_pattern' => '',
@@ -1176,13 +1172,13 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 
 	/**
 	 * Load filter data to show in the edit view from the DB.
-	 * @param int|string $filter The filter ID being requested or 'new'.
+	 * @param int|null $filter The filter ID being requested or null for a new filter
 	 * @param int|null $history_id If any, the history ID being requested.
 	 * @return array|null Array with filter data if available, otherwise null.
 	 * The first element contains the abuse_filter database row,
 	 *  the second element is an array of related abuse_filter_action rows.
 	 */
-	private function loadFromDatabase( $filter, $history_id = null ) {
+	private function loadFromDatabase( ?int $filter, $history_id = null ) {
 		if ( $history_id ) {
 			return $this->loadHistoryItem( $history_id );
 		} else {
@@ -1194,12 +1190,12 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 	 * Load data from the already-POSTed HTTP request.
 	 *
 	 * @throws BadMethodCallException If called without the request being POSTed or when trying
-	 *   to import a filter but $filter is not 'new'
-	 * @param int|string $filter The filter ID being requested.
+	 *   to import a filter but $filter is not null
+	 * @param int|null $filter The filter ID being requested, or null for a new filter
 	 * @return Status If good, the value is the array [ row, actions ]. If not, it contains an
 	 * error message.
 	 */
-	public function loadRequest( $filter ): Status {
+	public function loadRequest( ?int $filter ): Status {
 		$request = $this->getRequest();
 		if ( !$request->wasPosted() ) {
 			// Sanity
@@ -1218,7 +1214,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 		// Check for importing
 		$import = $request->getVal( 'wpImportText' );
 		if ( $import ) {
-			if ( $filter !== 'new' ) {
+			if ( $filter !== null ) {
 				// Sanity
 				throw new BadMethodCallException( __METHOD__ . ' called for importing on existing filter.' );
 			}
@@ -1248,7 +1244,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
 				$row->$name = $importRow->$name;
 			}
 		} else {
-			if ( $filter !== 'new' ) {
+			if ( $filter !== null ) {
 				// These aren't needed when saving the filter, but they are otherwise (e.g. if
 				// saving fails and we need to show the edit interface again).
 				$row->af_id = $origRow->af_id;

includes/Views/AbuseFilterViewRevert.php

@@ -62,8 +62,8 @@ class AbuseFilterViewRevert extends AbuseFilterView {
 		$max = wfTimestampNow();
 		$filterLink =
 			$this->linkRenderer->makeLink(
-				SpecialPage::getTitleFor( 'AbuseFilter', $filter ),
-				$lang->formatNum( $filter )
+				SpecialPage::getTitleFor( 'AbuseFilter', (string)$filter ),
+				$lang->formatNum( (string)$filter )
 			);
 		$searchFields = [];
 		$searchFields['filterid'] = [

includes/special/SpecialAbuseFilter.php

@@ -91,11 +91,16 @@ class SpecialAbuseFilter extends AbuseFilterSpecialPage {
 		}
 
 		if ( is_numeric( $subpage ) || $subpage === 'new' ) {
-			return [ AbuseFilterViewEdit::class, 'edit', [ 'filter' => $subpage ] ];
+			return [
+				AbuseFilterViewEdit::class,
+				'edit',
+				[ 'filter' => is_numeric( $subpage ) ? (int)$subpage : null ]
+			];
 		}
 
 		if ( $params ) {
 			if ( count( $params ) === 2 && $params[0] === 'revert' && is_numeric( $params[1] ) ) {
+				$params[1] = (int)$params[1];
 				return [ AbuseFilterViewRevert::class, 'revert', $params ];
 			}
 
@@ -109,13 +114,16 @@ class SpecialAbuseFilter extends AbuseFilterSpecialPage {
 
 			if ( $params[0] === 'history' || $params[0] === 'log' ) {
 				if ( count( $params ) <= 2 ) {
+					if ( isset( $params[1] ) ) {
+						$params[1] = (int)$params[1];
+					}
 					return [ AbuseFilterViewHistory::class, 'recentchanges', $params ];
 				}
 				if ( count( $params ) === 4 && $params[2] === 'item' ) {
 					return [
 						AbuseFilterViewEdit::class,
 						'',
-						[ 'filter' => $params[1], 'history' => (int)$params[3] ]
+						[ 'filter' => (int)$params[1], 'history' => (int)$params[3] ]
 					];
 				}
 				if ( count( $params ) === 5 && $params[2] === 'diff' ) {

tests/phpunit/AbuseFilterSaveTest.php

@@ -126,7 +126,7 @@ class AbuseFilterSaveTest extends MediaWikiTestCase {
 	public function testSaveFilter( $args ) {
 		$user = $this->getUserMock( $args['testData']['userPerms'] ?? [] );
 
-		$filter = $args['row']['af_id'] = $args['row']['af_id'] ?? 'new';
+		$filter = $args['row']['af_id'] = $args['row']['af_id'] ?? null;
 		[ $newRow, $actions, $origRow, $origActions ] = $this->getRowAndActionsFromTestSpecs( $args );
 
 		/** @var IDatabase|MockObject $dbw */
@@ -374,7 +374,7 @@ class AbuseFilterSaveTest extends MediaWikiTestCase {
 						'af_comments' => 'Throttle... Again',
 					],
 					'actions' => [
-						'throttle' => [ 'new', '11,111', "user\nfoo" ]
+						'throttle' => [ null, '11,111', "user\nfoo" ]
 					],
 					'testData' => [
 						'expectedMessage' => 'abusefilter-edit-invalid-throttlegroups',

tests/phpunit/unit/SpecialAbuseFilterTest.php

@@ -18,19 +18,19 @@ class SpecialAbuseFilterTest extends MediaWikiUnitTestCase {
 		return [
 			[ null, AbuseFilterViewList::class, 'home' ],
 			[ 'foo', AbuseFilterViewList::class, 'home' ],
-			[ '1', AbuseFilterViewEdit::class, 'edit', [ 'filter' => '1' ] ],
-			[ 'new', AbuseFilterViewEdit::class, 'edit', [ 'filter' => 'new' ] ],
+			[ '1', AbuseFilterViewEdit::class, 'edit', [ 'filter' => 1 ] ],
+			[ 'new', AbuseFilterViewEdit::class, 'edit', [ 'filter' => null ] ],
 			[ 'history', AbuseFilterViewHistory::class, 'recentchanges', [ 'history' ] ],
-			[ 'history/1', AbuseFilterViewHistory::class, 'recentchanges', [ 'history', '1' ] ],
-			[ 'history/1/item/2', AbuseFilterViewEdit::class, '', [ 'filter' => '1', 'history' => 2 ] ],
+			[ 'history/1', AbuseFilterViewHistory::class, 'recentchanges', [ 'history', 1 ] ],
+			[ 'history/1/item/2', AbuseFilterViewEdit::class, '', [ 'filter' => 1, 'history' => 2 ] ],
 			[ 'history/foo/bar', AbuseFilterViewList::class, 'home' ],
 			[ 'history/1/diff/2/3', AbuseFilterViewDiff::class, '', [ 'history', '1', 'diff', '2', '3' ] ],
 			[ 'history/1/diff/prev/3', AbuseFilterViewDiff::class, '', [ 'history', '1', 'diff', 'prev', '3' ] ],
 			[ 'history/1/diff/prev/cur', AbuseFilterViewDiff::class, '', [ 'history', '1', 'diff', 'prev', 'cur' ] ],
 			[ 'history/1/foo/2/3', AbuseFilterViewList::class, 'home' ],
 			[ 'log', AbuseFilterViewHistory::class, 'recentchanges', [ 'log' ] ],
-			[ 'log/1', AbuseFilterViewHistory::class, 'recentchanges', [ 'log', '1' ] ],
-			[ 'log/1/item/2', AbuseFilterViewEdit::class, '', [ 'filter' => '1', 'history' => 2 ] ],
+			[ 'log/1', AbuseFilterViewHistory::class, 'recentchanges', [ 'log', 1 ] ],
+			[ 'log/1/item/2', AbuseFilterViewEdit::class, '', [ 'filter' => 1, 'history' => 2 ] ],
 			[ 'log/foo/bar', AbuseFilterViewList::class, 'home' ],
 			[ 'log/1/diff/2/3', AbuseFilterViewDiff::class, '', [ 'log', '1', 'diff', '2', '3' ] ],
 			[ 'log/1/foo/2/3', AbuseFilterViewList::class, 'home' ],
@@ -41,7 +41,7 @@ class SpecialAbuseFilterTest extends MediaWikiUnitTestCase {
 			[ 'test', AbuseFilterViewTestBatch::class, 'test', [ 'test' ] ],
 			[ 'test/1', AbuseFilterViewTestBatch::class, 'test', [ 'test', '1' ] ],
 			[ 'revert', AbuseFilterViewList::class, 'home' ],
-			[ 'revert/1', AbuseFilterViewRevert::class, 'revert', [ 'revert', '1' ] ],
+			[ 'revert/1', AbuseFilterViewRevert::class, 'revert', [ 'revert', 1 ] ],
 			[ 'revert/1/foo', AbuseFilterViewList::class, 'home' ],
 			[ 'examine', AbuseFilterViewExamine::class, 'examine', [ 'examine' ] ],
 			[ 'examine/foo/bar', AbuseFilterViewExamine::class, 'examine', [ 'examine', 'foo', 'bar' ] ],