Merge "Beautify old, broken abuse_filter_history rows"

i18n/en.json

@@ -212,6 +212,7 @@
 	"abusefilter-throttle-editcount": "edit count",
 	"abusefilter-throttle-site": "whole site",
 	"abusefilter-throttle-page": "page",
+	"abusefilter-throttle-none": "(none)",
 	"abusefilter-throttle-details": "Allow $1 {{PLURAL:$1|action|actions}} every $2 {{PLURAL:$2|second|seconds}}, group throttle by: $3",
 	"abusefilter-edit-warn-message": "System message to use for warning:",
 	"abusefilter-edit-warn-other": "Other message",

i18n/qqq.json

@@ -246,6 +246,7 @@
 	"abusefilter-throttle-editcount": "Throttle option.",
 	"abusefilter-throttle-site": "Throttle option.",
 	"abusefilter-throttle-page": "Throttle option.",
+	"abusefilter-throttle-none": "Bogus throttle option, means that no options are enabled.",
 	"abusefilter-throttle-details": "Description for Special:AbuseFilter/history with a detailed description for throttle action. Parameters:\n* $1 is the number of allowed actions, $2 is the time in seconds to use before resetting the action count, $3 is a list of throttled groups",
 	"abusefilter-edit-warn-message": "Field label for dropdown list with system messages.",
 	"abusefilter-edit-warn-other": "Option in dropdown menu to specify no item from the list should be used.\n\nSee also:\n* {{msg-mw|abusefilter-edit-disallow-other}}",

includes/AbuseFilter.php

@@ -3188,37 +3188,29 @@ class AbuseFilter {
 				array_shift( $parameters );
 				list( $actions, $time ) = explode( ',', array_shift( $parameters ) );
 
-				if ( $parameters === [ '' ] ) {
-					// Having empty groups won't happen for new filters due to validation upon saving,
-					// but old entries may have it. We'd better not show a broken message. Also,
-					// the array has an empty string inside because we haven't been passing an empty array
-					// as the default when retrieving wpFilterThrottleGroups with getArray (when it was
-					// a CheckboxMultiselect).
-					$groups = '';
-				} else {
-					// Join comma-separated groups in a commaList with a final "and", and convert to messages.
-					// Messages used here: abusefilter-throttle-ip, abusefilter-throttle-user,
-					// abusefilter-throttle-site, abusefilter-throttle-creationdate, abusefilter-throttle-editcount
-					// abusefilter-throttle-range, abusefilter-throttle-page
-					foreach ( $parameters as &$val ) {
-						if ( strpos( $val, ',' ) !== false ) {
-							$subGroups = explode( ',', $val );
-							foreach ( $subGroups as &$group ) {
-								$msg = wfMessage( "abusefilter-throttle-$group" );
-								// We previously accepted literally everything in this field, so old entries
-								// may have weird stuff.
-								$group = $msg->exists() ? $msg->text() : $group;
-							}
-							unset( $group );
-							$val = $lang->listToText( $subGroups );
-						} else {
-							$msg = wfMessage( "abusefilter-throttle-$val" );
-							$val = $msg->exists() ? $msg->text() : $val;
+				// Join comma-separated groups in a commaList with a final "and", and convert to messages.
+				// Messages used here: abusefilter-throttle-ip, abusefilter-throttle-user,
+				// abusefilter-throttle-site, abusefilter-throttle-creationdate, abusefilter-throttle-editcount
+				// abusefilter-throttle-range, abusefilter-throttle-page, abusefilter-throttle-none
+				foreach ( $parameters as &$val ) {
+					if ( strpos( $val, ',' ) !== false ) {
+						$subGroups = explode( ',', $val );
+						foreach ( $subGroups as &$group ) {
+							$msg = wfMessage( "abusefilter-throttle-$group" );
+							// We previously accepted literally everything in this field, so old entries
+							// may have weird stuff.
+							$group = $msg->exists() ? $msg->text() : $group;
 						}
+						unset( $group );
+						$val = $lang->listToText( $subGroups );
+					} else {
+						$msg = wfMessage( "abusefilter-throttle-$val" );
+						$val = $msg->exists() ? $msg->text() : $val;
 					}
-					unset( $val );
-					$groups = $lang->semicolonList( $parameters );
 				}
+				unset( $val );
+				$groups = $lang->semicolonList( $parameters );
+
 				$displayAction = self::getActionDisplay( $action ) .
 				wfMessage( 'colon-separator' )->escaped() .
 				wfMessage( 'abusefilter-throttle-details' )->params( $actions, $time, $groups )->escaped();

maintenance/normalizeThrottleParameters.php

@@ -11,6 +11,10 @@
  *   highly unlikely to happen anyway. (T203585)
  * - If throttle groups are empty (or only contain unknown keywords), ask users to fix every case
  *   by hand. (T203584)
+ * - Change some edge cases of throttle parameters saved in abuse_filter_history (T215787):
+ *     - parameters = null ==> parameters = [ filterID, "0,0", 'none' ]
+ *     - at least a number missing from parameters[1] ==> insert 0 in place of the missing param
+ *     - empty groups ==> 'none' (special case, uses the message abusefilter-throttle-none)
  *
  * @ingroup Maintenance
  */
@@ -41,7 +45,7 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance {
 		return __CLASS__;
 	}
 
-	/** @var \Wikimedia\Rdbms\IDatabase $db The master database */
+	/** @var \Wikimedia\Rdbms\Database $dbw The master database */
 	private $dbw;
 
 	/**
@@ -133,12 +137,12 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance {
 	}
 
 	/**
-	 * @see Maintenance::doDBUpdates
-	 * @return bool
+	 * Main logic of parameters normalization
+	 *
+	 * @return int Amount of normalized rows
 	 */
-	public function doDBUpdates() {
+	protected function normalizeParameters() {
 		$user = AbuseFilter::getFilterUser();
-		$this->dbw = wfGetDB( DB_MASTER );
 		$dryRun = $this->hasOption( 'dry-run' );
 
 		// IDs of filters with invalid rate (count or period)
@@ -152,8 +156,6 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance {
 		// holds an empty string and (unserialize(afh_actions))['throttle'] is null.
 		$totallyEmpty = [];
 
-		$this->beginTransaction( $this->dbw, __METHOD__ );
-
 		// Only select throttle actions
 		$actionRows = $this->dbw->select(
 			'abuse_filter_action',
@@ -320,8 +322,7 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance {
 		$touchedIDs = array_merge( $changeActionIDs, $deleteActionIDs );
 		if ( count( $touchedIDs ) === 0 ) {
 			$this->output( "No throttle parameters to normalize.\n" );
-			$this->commitTransaction( $this->dbw, __METHOD__ );
-			return !$dryRun;
+			return 0;
 		}
 
 		// Create new history rows for every changed filter
@@ -396,13 +397,97 @@ class NormalizeThrottleParameters extends LoggedUpdateMaintenance {
 				);
 			}
 		}
+		return $affectedActionRows + $historyCount;
+	}
+
+	/**
+	 * Beautify empty/missing/corrupted parameters in abuse_filter_history
+	 *
+	 * @return int Amount of beautified rows
+	 */
+	protected function beautifyHistory() {
+		$dryRun = $this->hasOption( 'dry-run' );
+
+		// We need any row containing throttle, but there's no
+		// need to lock as these rows aren't changed by the actual code.
+		$likeClause = $this->dbw->buildLike(
+			$this->dbw->anyString(),
+			'throttle',
+			$this->dbw->anyString()
+		);
+		$histRows = $this->dbw->select(
+			'abuse_filter_history',
+			[ 'afh_id', 'afh_actions', 'afh_filter' ],
+			[ 'afh_actions ' . $likeClause ],
+			__METHOD__
+		);
+
+		$beautyIDs = [];
+		foreach ( $histRows as $row ) {
+			$acts = unserialize( $row->afh_actions );
+			if ( !array_key_exists( 'throttle', $acts ) ) {
+				// The LIKE clause is very raw, so this could happen
+				continue;
+			}
+
+			if ( $acts['throttle'] === null ) {
+				// Corrupted row, rebuild it (T215787)
+				$acts['throttle'] = [ $row->afh_filter, '0,0', 'none' ];
+			} elseif ( $this->checkThrottleRate( $acts['throttle'][1] ) !== null ) {
+				// Missing count, make it explicitly 0
+				$acts['throttle'][1] = preg_replace( '/^,/', '0,', $acts['throttle'][1] );
+				// Missing period, make it explicitly 0
+				$acts['throttle'][1] = preg_replace( '/,$/', ',0', $acts['throttle'][1] );
+			} elseif ( count( $acts['throttle'] ) === 2 ) {
+				// Missing groups, make them explicitly "none" (special group)
+				$acts['throttle'][] = 'none';
+			} else {
+				// Everything's fine!
+				continue;
+			}
+
+			$beautyIDs[] = $row->afh_id;
+			if ( !$dryRun ) {
+				$this->dbw->update(
+					'abuse_filter_history',
+					[ 'afh_actions' => serialize( $acts ) ],
+					[ 'afh_id' => $row->afh_id ],
+					__METHOD__
+				);
+			}
+		}
+
+		$changed = count( $beautyIDs );
+		if ( $changed ) {
+			$verb = $dryRun ? 'would beautify' : 'beautified';
+			$this->output(
+				"normalizeThrottleParameter $verb $changed rows in abuse_filter_history" .
+				" for the following history IDs: " . implode( ', ', $beautyIDs ) . "\n"
+			);
+		}
+		return $changed;
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	public function doDBUpdates() {
+		$dryRun = $this->hasOption( 'dry-run' );
+		$this->dbw = wfGetDB( DB_MASTER );
+		$this->beginTransaction( $this->dbw, __METHOD__ );
+
+		$normalized = $this->normalizeParameters();
+		$beautified = $this->beautifyHistory();
 
 		$this->commitTransaction( $this->dbw, __METHOD__ );
-		$changed = $affectedActionRows + $historyCount;
+
+		$changed = $normalized + $beautified;
+
 		$resultMsg = $dryRun ?
 			"Throttle parameter normalization would change a total of $changed rows.\n" :
 			"Throttle parameters successfully normalized. Changed $changed rows.\n";
 		$this->output( $resultMsg );
+
 		return !$dryRun;
 	}
 }