From be3af6687676e61703a9b188a03267521ad24703 Mon Sep 17 00:00:00 2001
From: Thiemo Kreuz <thiemo.kreuz@wikimedia.de>
Date: Tue, 26 Apr 2022 17:39:04 +0200
Subject: [PATCH] Simplify code dealing with filter ids in FilterStore

Before the information if a filter was new was stored in 2 places:
In the bool $isNew and in the two variables $filter and $newID.
$newID was especially confusing because it was used for both old and
new ids.

Change-Id: I15bdf36c96c8d86a37f305aab2647f7d57bc2bf1
---
 includes/FilterStore.php | 70 +++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

diff --git a/includes/FilterStore.php b/includes/FilterStore.php
index b8d5ac618..a20d7b95f 100644
--- a/includes/FilterStore.php
+++ b/includes/FilterStore.php
@@ -81,14 +81,14 @@ class FilterStore {
 	 *  - Fatal in case of a permission-related error
 	 *
 	 * @param User $user
-	 * @param int|null $filter
+	 * @param int|null $filterId
 	 * @param Filter $newFilter
 	 * @param Filter $originalFilter
 	 * @return Status
 	 */
 	public function saveFilter(
 		User $user,
-		?int $filter,
+		?int $filterId,
 		Filter $newFilter,
 		Filter $originalFilter
 	): Status {
@@ -105,7 +105,7 @@ class FilterStore {
 
 		// Everything went fine, so let's save the filter
 		$wasGlobal = $originalFilter->isGlobal();
-		list( $newID, $historyID ) = $this->doSaveFilter( $user, $newFilter, $differences, $filter, $wasGlobal );
+		[ $newID, $historyID ] = $this->doSaveFilter( $user, $newFilter, $differences, $filterId, $wasGlobal );
 		return Status::newGood( [ $newID, $historyID ] );
 	}
 
@@ -115,7 +115,7 @@ class FilterStore {
 	 * @param User $user
 	 * @param Filter $newFilter
 	 * @param array $differences
-	 * @param int|null $filter
+	 * @param int|null $filterId
 	 * @param bool $wasGlobal
 	 * @return int[] first element is new ID, second is history ID
 	 */
@@ -123,7 +123,7 @@ class FilterStore {
 		User $user,
 		Filter $newFilter,
 		array $differences,
-		?int $filter,
+		?int $filterId,
 		bool $wasGlobal
 	): array {
 		$dbw = $this->loadBalancer->getConnectionRef( DB_PRIMARY );
@@ -134,8 +134,7 @@ class FilterStore {
 		$newRow['af_user'] = $user->getId();
 		$newRow['af_user_text'] = $user->getName();
 
-		$isNew = $filter === null;
-		$newID = $filter;
+		$isNew = $filterId === null;
 
 		// Preserve the old throttled status (if any) only if disabling the filter.
 		// TODO: It might make more sense to check what was actually changed
@@ -145,37 +144,34 @@ class FilterStore {
 		$rowForInsert = array_diff_key( $newRow, [ 'af_id' => true ] );
 
 		$dbw->startAtomic( __METHOD__ );
-		if ( $isNew ) {
+		if ( $filterId === null ) {
 			$dbw->insert( 'abuse_filter', $rowForInsert, __METHOD__ );
-			$newID = $dbw->insertId();
+			$filterId = $dbw->insertId();
 		} else {
-			$dbw->update( 'abuse_filter', $rowForInsert, [ 'af_id' => $newID ], __METHOD__ );
+			$dbw->update( 'abuse_filter', $rowForInsert, [ 'af_id' => $filterId ], __METHOD__ );
 		}
-		'@phan-var int $newID';
-		$newRow['af_id'] = $newID;
+		$newRow['af_id'] = $filterId;
 
 		$actions = $newFilter->getActions();
 		$actionsRows = [];
 		foreach ( $this->consequencesRegistry->getAllEnabledActionNames() as $action ) {
-			// Check if it's set
-			$enabled = isset( $actions[$action] );
-
-			if ( $enabled ) {
-				$parameters = $actions[$action];
-				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.
-					$parameters[0] = $newID;
-				}
-
-				$thisRow = [
-					'afa_filter' => $newID,
-					'afa_consequence' => $action,
-					'afa_parameters' => implode( "\n", $parameters )
-				];
-				$actionsRows[] = $thisRow;
+			if ( !isset( $actions[$action] ) ) {
+				continue;
 			}
+
+			$parameters = $actions[$action];
+			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.
+				$parameters[0] = $filterId;
+			}
+
+			$actionsRows[] = [
+				'afa_filter' => $filterId,
+				'afa_consequence' => $action,
+				'afa_parameters' => implode( "\n", $parameters ),
+			];
 		}
 
 		// Create a history row
@@ -205,7 +201,7 @@ class FilterStore {
 
 		$afhRow['afh_flags'] = implode( ',', $flags );
 
-		$afhRow['afh_filter'] = $newID;
+		$afhRow['afh_filter'] = $filterId;
 
 		// Do the update
 		$dbw->insert( 'abuse_filter_history', $afhRow, __METHOD__ );
@@ -213,7 +209,7 @@ class FilterStore {
 		if ( !$isNew ) {
 			$dbw->delete(
 				'abuse_filter_action',
-				[ 'afa_filter' => $filter ],
+				[ 'afa_filter' => $filterId ],
 				__METHOD__
 			);
 		}
@@ -230,10 +226,10 @@ class FilterStore {
 		$subtype = $isNew ? 'create' : 'modify';
 		$logEntry = new ManualLogEntry( 'abusefilter', $subtype );
 		$logEntry->setPerformer( $user );
-		$logEntry->setTarget( SpecialAbuseFilter::getTitleForSubpage( (string)$newID ) );
+		$logEntry->setTarget( SpecialAbuseFilter::getTitleForSubpage( (string)$filterId ) );
 		$logEntry->setParameters( [
 			'historyId' => $historyID,
-			'newId' => $newID
+			'newId' => $filterId
 		] );
 		$logid = $logEntry->insert( $dbw );
 		$logEntry->publish( $logid );
@@ -243,11 +239,11 @@ class FilterStore {
 			$this->tagsManager->purgeTagCache();
 		}
 
-		$this->filterProfiler->resetFilterProfile( $newID );
+		$this->filterProfiler->resetFilterProfile( $filterId );
 		if ( $newRow['af_enabled'] ) {
-			$this->emergencyCache->setNewForFilter( $newID, $newRow['af_group'] );
+			$this->emergencyCache->setNewForFilter( $filterId, $newRow['af_group'] );
 		}
-		return [ $newID, $historyID ];
+		return [ $filterId, $historyID ];
 	}
 
 	/**