block: Show a special log entry when blocking an already blocked target

"Sysop added a block for Target" instead of "Sysop blocked Target".

Bug: T378150
Change-Id: I2febf1bcb57d014e48bd58eecbdecaa3c2f9c724

7 files changed, 163 insertions, 53 deletions

diff --git a/includes/block/BlockUser.php b/includes/block/BlockUser.php
index 092a1f9d26cc..f204e554277e 100644
--- a/includes/block/BlockUser.php
+++ b/includes/block/BlockUser.php
@@ -607,6 +607,7 @@ class BlockUser {
 
 		$expectedTargetCount = 0;
 		$priorBlock = null;
+		$priorBlocks = $this->getPriorBlocksForTarget();
 
 		if ( $this->blockToUpdate !== null ) {
 			if ( $block->equals( $this->blockToUpdate ) ) {
@@ -629,37 +630,33 @@ class BlockUser {
 			}
 			$expectedTargetCount = null;
 			$update = false;
+		} elseif ( !$priorBlocks ) {
+			$update = false;
 		} else {
-			// Is there a conflicting block?
-			$priorBlocks = $this->getPriorBlocksForTarget();
-			if ( !$priorBlocks ) {
-				$update = false;
-			} else {
-				// Reblock only if the caller wants so
-				if ( $conflictMode !== self::CONFLICT_REBLOCK ) {
-					$this->logger->debug(
-						'placeBlockInternal: already blocked and reblock not requested' );
-					return Status::newFatal( 'ipb_already_blocked', $block->getTargetName() );
-				}
-
-				// Can't update multiple blocks unless blockToUpdate was given
-				if ( count( $priorBlocks ) > 1 ) {
-					throw new \RuntimeException(
-						"Can\'t reblock a user with multiple blocks already present. " .
-						"Update calling code for multiblocks, providing a specific block to update." );
-				}
+			// Reblock only if the caller wants so
+			if ( $conflictMode !== self::CONFLICT_REBLOCK ) {
+				$this->logger->debug(
+					'placeBlockInternal: already blocked and reblock not requested' );
+				return Status::newFatal( 'ipb_already_blocked', $block->getTargetName() );
+			}
 
-				// Check for identical blocks
-				$priorBlock = $priorBlocks[0];
-				if ( $block->equals( $priorBlock ) ) {
-					// Block settings are equal => user is already blocked
-					$this->logger->debug( 'placeBlockInternal: already blocked, no change' );
-					return Status::newFatal( 'ipb_already_blocked', $block->getTargetName() );
-				}
+			// Can't update multiple blocks unless blockToUpdate was given
+			if ( count( $priorBlocks ) > 1 ) {
+				throw new \RuntimeException(
+					"Can\'t reblock a user with multiple blocks already present. " .
+					"Update calling code for multiblocks, providing a specific block to update." );
+			}
 
-				$update = true;
-				$block = $this->configureBlock( $priorBlock );
+			// Check for identical blocks
+			$priorBlock = $priorBlocks[0];
+			if ( $block->equals( $priorBlock ) ) {
+				// Block settings are equal => user is already blocked
+				$this->logger->debug( 'placeBlockInternal: already blocked, no change' );
+				return Status::newFatal( 'ipb_already_blocked', $block->getTargetName() );
 			}
+
+			$update = true;
+			$block = $this->configureBlock( $priorBlock );
 		}
 
 		if ( $update ) {
@@ -673,6 +670,9 @@ class BlockUser {
 				$this->logger->warning( 'Block could not be inserted. No existing block was found.' );
 				return Status::newFatal( 'ipb-block-not-found', $block->getTargetName() );
 			}
+			if ( $insertStatus['finalTargetCount'] > 1 ) {
+				$logEntry->addParameter( 'finalTargetCount', $insertStatus['finalTargetCount'] );
+			}
 		}
 		// Relate log ID to block ID (T27763)
 		$logEntry->setRelations( [ 'ipb_id' => $block->getId() ] );
diff --git a/includes/block/DatabaseBlockStore.php b/includes/block/DatabaseBlockStore.php
index cb49f0086267..3914535cd5fb 100644
--- a/includes/block/DatabaseBlockStore.php
+++ b/includes/block/DatabaseBlockStore.php
@@ -924,7 +924,9 @@ class DatabaseBlockStore {
 	 *   on the specified target. If this is zero but there is an existing
 	 *   block, the insertion will fail.
 	 * @return bool|array False on failure, assoc array on success:
-	 *      ('id' => block ID, 'autoIds' => array of autoblock IDs)
+	 *   - id: block ID
+	 *   - autoIds: array of autoblock IDs
+	 *   - finalTargetCount: The updated number of blocks for the specified target.
 	 */
 	public function insertBlock(
 		DatabaseBlock $block,
@@ -952,18 +954,30 @@ class DatabaseBlockStore {
 
 		$dbw = $this->getPrimaryDB();
 		$dbw->startAtomic( __METHOD__ );
-		$success = $this->attemptInsert( $block, $dbw, $expectedTargetCount );
+		$finalTargetCount = $this->attemptInsert( $block, $dbw, $expectedTargetCount );
+		$purgeDone = false;
 
 		// Don't collide with expired blocks.
 		// Do this after trying to insert to avoid locking.
-		if ( !$success ) {
+		if ( !$finalTargetCount ) {
 			if ( $this->purgeExpiredConflicts( $block, $dbw ) ) {
-				$success = $this->attemptInsert( $block, $dbw, $expectedTargetCount );
+				$finalTargetCount = $this->attemptInsert( $block, $dbw, $expectedTargetCount );
+				$purgeDone = true;
 			}
 		}
 		$dbw->endAtomic( __METHOD__ );
 
-		if ( $success ) {
+		if ( $finalTargetCount > 1 && !$purgeDone ) {
+			// Subtract expired blocks from the target count
+			$expiredBlockCount = $this->getExpiredConflictingBlockRows( $block, $dbw )->count();
+			if ( $expiredBlockCount >= $finalTargetCount ) {
+				$finalTargetCount = 1;
+			} else {
+				$finalTargetCount -= $expiredBlockCount;
+			}
+		}
+
+		if ( $finalTargetCount ) {
 			$autoBlockIds = $this->doRetroactiveAutoblock( $block );
 
 			if ( $this->options->get( MainConfigNames::BlockDisablesLogin ) ) {
@@ -974,7 +988,11 @@ class DatabaseBlockStore {
 				}
 			}
 
-			return [ 'id' => $block->getId( $this->wikiId ), 'autoIds' => $autoBlockIds ];
+			return [
+				'id' => $block->getId( $this->wikiId ),
+				'autoIds' => $autoBlockIds,
+				'finalTargetCount' => $finalTargetCount
+			];
 		}
 
 		return false;
@@ -987,14 +1005,14 @@ class DatabaseBlockStore {
 	 * @param DatabaseBlock $block
 	 * @param IDatabase $dbw
 	 * @param int|null $expectedTargetCount
-	 * @return bool True if block successfully inserted
+	 * @return int|false The updated number of blocks for the target, or false on failure
 	 */
 	private function attemptInsert(
 		DatabaseBlock $block,
 		IDatabase $dbw,
 		$expectedTargetCount
 	) {
-		$targetId = $this->acquireTarget( $block, $dbw, $expectedTargetCount );
+		[ $targetId, $finalCount ] = $this->acquireTarget( $block, $dbw, $expectedTargetCount );
 		if ( !$targetId ) {
 			return false;
 		}
@@ -1018,7 +1036,7 @@ class DatabaseBlockStore {
 			$this->blockRestrictionStore->insert( $restrictions );
 		}
 
-		return true;
+		return $finalCount;
 	}
 
 	/**
@@ -1032,15 +1050,31 @@ class DatabaseBlockStore {
 		DatabaseBlock $block,
 		IDatabase $dbw
 	) {
+		return (bool)$this->deleteBlockRows(
+			$this->getExpiredConflictingBlockRows( $block, $dbw )
+		);
+	}
+
+	/**
+	 * Get rows with bl_id/bl_target for expired blocks that have the same
+	 * target as the specified block.
+	 *
+	 * @param DatabaseBlock $block
+	 * @param IDatabase $dbw
+	 * @return IResultWrapper
+	 */
+	private function getExpiredConflictingBlockRows(
+		DatabaseBlock $block,
+		IDatabase $dbw
+	) {
 		$targetConds = $this->getTargetConds( $block );
-		$res = $dbw->newSelectQueryBuilder()
+		return $dbw->newSelectQueryBuilder()
 			->select( [ 'bl_id', 'bl_target' ] )
 			->from( 'block' )
 			->join( 'block_target', null, [ 'bt_id=bl_target' ] )
 			->where( $targetConds )
 			->andWhere( $dbw->expr( 'bl_expiry', '<', $dbw->timestamp() ) )
 			->caller( __METHOD__ )->fetchResultSet();
-		return (bool)$this->deleteBlockRows( $res );
 	}
 
 	/**
@@ -1061,7 +1095,7 @@ class DatabaseBlockStore {
 
 	/**
 	 * Insert a new block_target row, or update bt_count in the existing target
-	 * row for a given block, and return the target ID.
+	 * row for a given block, and return the target ID and new bt_count.
 	 *
 	 * An atomic section should be active while calling this function.
 	 *
@@ -1071,7 +1105,7 @@ class DatabaseBlockStore {
 	 *   exists, abort the insert and return null. If this is greater than zero
 	 *   and the pre-increment bt_count value does not match, abort the update
 	 *   and return null. If this is null, do not perform any conflict checks.
-	 * @return int|null
+	 * @return array{?int,int}
 	 */
 	private function acquireTarget(
 		DatabaseBlock $block,
@@ -1114,26 +1148,37 @@ class DatabaseBlockStore {
 		$numUpdatedRows = $dbw->affectedRows();
 
 		// Now that the row is locked, find the target ID
-		$ids = $dbw->newSelectQueryBuilder()
-			->select( 'bt_id' )
+		$res = $dbw->newSelectQueryBuilder()
+			->select( [ 'bt_id', 'bt_count' ] )
 			->from( 'block_target' )
 			->where( $targetConds )
 			->caller( __METHOD__ )
-			->fetchFieldValues();
-		if ( count( $ids ) > 1 ) {
+			->fetchResultSet();
+		if ( $res->numRows() > 1 ) {
+			$ids = [];
+			foreach ( $res as $row ) {
+				$ids[] = $row->bt_id;
+			}
 			throw new RuntimeException( "Duplicate block_target rows detected: " .
 				implode( ',', $ids ) );
 		}
-		$id = $ids[0] ?? false;
+		$row = $res->fetchObject();
 
-		if ( $id === false ) {
+		if ( $row ) {
+			$count = (int)$row->bt_count;
+			if ( !$numUpdatedRows ) {
+				// ID found but count update failed -- must be a conflict due to bt_count mismatch
+				return [ null, $count ];
+			}
+			$id = (int)$row->bt_id;
+		} else {
 			if ( $numUpdatedRows ) {
 				throw new RuntimeException(
 					'block_target row unexpectedly missing after we locked it' );
 			}
 			if ( $expectedTargetCount !== 0 && $expectedTargetCount !== null ) {
 				// Conflict (expectation failure)
-				return null;
+				return [ null, 0 ];
 			}
 
 			// Insert new row
@@ -1156,12 +1201,10 @@ class DatabaseBlockStore {
 				throw new RuntimeException(
 					'block_target insert ID is falsey despite unconditional insert' );
 			}
-		} elseif ( !$numUpdatedRows ) {
-			// ID found but count update failed -- must be a conflict due to bt_count mismatch
-			return null;
+			$count = 1;
 		}
 
-		return (int)$id;
+		return [ $id, $count ];
 	}
 
 	/**
@@ -1266,7 +1309,7 @@ class DatabaseBlockStore {
 		$block->setTarget( $newTarget );
 
 		$dbw->startAtomic( __METHOD__ );
-		$targetId = $this->acquireTarget( $block, $dbw, null );
+		[ $targetId, $count ] = $this->acquireTarget( $block, $dbw, null );
 		if ( !$targetId ) {
 			// This is an exotic and unlikely error -- perhaps an exception should be thrown
 			$dbw->endAtomic( __METHOD__ );
diff --git a/includes/logging/BlockLogFormatter.php b/includes/logging/BlockLogFormatter.php
index 05d53665c1aa..3f1996f55ded 100644
--- a/includes/logging/BlockLogFormatter.php
+++ b/includes/logging/BlockLogFormatter.php
@@ -338,7 +338,9 @@ class BlockLogFormatter extends LogFormatter {
 	protected function getMessageKey() {
 		$type = $this->entry->getType();
 		$subtype = $this->entry->getSubtype();
-		$sitewide = $this->entry->getParameters()['sitewide'] ?? true;
+		$params = $this->entry->getParameters();
+		$sitewide = $params['sitewide'] ?? true;
+		$count = $params['finalTargetCount'] ?? 0;
 
 		$key = "logentry-$type-$subtype";
 		if ( ( $subtype === 'block' || $subtype === 'reblock' ) && !$sitewide ) {
@@ -354,6 +356,11 @@ class BlockLogFormatter extends LogFormatter {
 				$key = "logentry-non-editing-$type-$subtype";
 			}
 		}
+		if ( $subtype === 'block' && $count > 1 ) {
+			// logentry-block-block-multi, logentry-partialblock-block-multi,
+			// logentry-non-editing-block-block-multi
+			$key .= '-multi';
+		}
 
 		return $key;
 	}
diff --git a/includes/logging/ManualLogEntry.php b/includes/logging/ManualLogEntry.php
index d0a58f58b650..d2f808f814b3 100644
--- a/includes/logging/ManualLogEntry.php
+++ b/includes/logging/ManualLogEntry.php
@@ -152,6 +152,19 @@ class ManualLogEntry extends LogEntryBase implements Taggable {
 	}
 
 	/**
+	 * Add a parameter to the list already set.
+	 *
+	 * @see setParameters
+	 * @since 1.44
+	 *
+	 * @param string $name
+	 * @param mixed $value
+	 */
+	public function addParameter( $name, $value ) {
+		$this->parameters[$name] = $value;
+	}
+
+	/**
 	 * Declare arbitrary tag/value relations to this log entry.
 	 * These will be stored in the log_search table and can be used
 	 * to filter log entries later on.
diff --git a/languages/i18n/en.json b/languages/i18n/en.json
index 571d14baf916..4806fa0088bb 100644
--- a/languages/i18n/en.json
+++ b/languages/i18n/en.json
@@ -4031,14 +4031,17 @@
 	"revdelete-restricted": "applied restrictions to administrators",
 	"revdelete-unrestricted": "removed restrictions for administrators",
 	"logentry-block-block": "$1 {{GENDER:$2|blocked}} {{GENDER:$4|$3}} with an expiration time of $5 $6",
+	"logentry-block-block-multi": "$1 {{GENDER:$2|added}} a block for {{GENDER:$4|$3}} with an expiration time of $5 $6",
 	"logentry-block-unblock": "$1 {{GENDER:$2|unblocked}} {{GENDER:$4|$3}}",
 	"logentry-block-reblock": "$1 {{GENDER:$2|changed}} block settings for {{GENDER:$4|$3}} with an expiration time of $5 $6",
 	"logentry-partialblock-block-page": "the {{PLURAL:$1|page|pages}} $2",
 	"logentry-partialblock-block-ns": "the {{PLURAL:$1|namespace|namespaces}} $2",
 	"logentry-partialblock-block-action": "the {{PLURAL:$1|action|actions}} $2",
 	"logentry-partialblock-block": "$1 {{GENDER:$2|blocked}} {{GENDER:$4|$3}} from $7 with an expiration time of $5 $6",
+	"logentry-partialblock-block-multi": "$1 {{GENDER:$2|added}} a block for {{GENDER:$4|$3}} from $7 with an expiration time of $5 $6",
 	"logentry-partialblock-reblock": "$1 {{GENDER:$2|changed}} block settings for {{GENDER:$4|$3}} blocking $7 with an expiration time of $5 $6",
 	"logentry-non-editing-block-block": "$1 {{GENDER:$2|blocked}} {{GENDER:$4|$3}} from specified non-editing actions with an expiration time of $5 $6",
+	"logentry-non-editing-block-block-multi": "$1 {{GENDER:$2|added}} a block for {{GENDER:$4|$3}} from specified non-editing actions with an expiration time of $5 $6",
 	"logentry-non-editing-block-reblock": "$1 {{GENDER:$2|changed}} block settings for {{GENDER:$4|$3}} for specified non-editing actions with an expiration time of $5 $6",
 	"logentry-suppress-block": "$1 {{GENDER:$2|blocked}} {{GENDER:$4|$3}} with an expiration time of $5 $6",
 	"logentry-suppress-reblock": "$1 {{GENDER:$2|changed}} block settings for {{GENDER:$4|$3}} with an expiration time of $5 $6",
diff --git a/languages/i18n/qqq.json b/languages/i18n/qqq.json
index bd24639a7e64..01396ed37d1c 100644
--- a/languages/i18n/qqq.json
+++ b/languages/i18n/qqq.json
@@ -4295,14 +4295,17 @@
 	"revdelete-restricted": "Used as <code>$4</code> in the following messages when setting visibility restrictions for administrators:\n* {{msg-mw|Logentry-delete-event}}\n* {{msg-mw|Logentry-delete-revision}}\n* {{msg-mw|Logentry-suppress-event}}\n* {{msg-mw|Logentry-suppress-event}}",
 	"revdelete-unrestricted": "Used as <code>$4</code> in the following messages when setting visibility restrictions for administrators:\n* {{msg-mw|Logentry-delete-event}}\n* {{msg-mw|Logentry-delete-revision}}\n* {{msg-mw|Logentry-suppress-event}}\n* {{msg-mw|Logentry-suppress-event}}",
 	"logentry-block-block": "{{Logentry|[[Special:Log/block]]}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string\n\nCf. {{msg-mw|Blocklogentry}}",
+	"logentry-block-block-multi": "Log entry used when an admin adds another block for a user who is already blocked.\n\n{{Logentry|[[Special:Log/block]]}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string",
 	"logentry-block-unblock": "{{Logentry|[[Special:Log/block]]}}\n* $4 - username for gender or empty string for autoblocks\n\nCf. {{msg-mw|Unblocklogentry}}",
 	"logentry-block-reblock": "{{Logentry|[[Special:Log/block]]}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string\n\nCf. {{msg-mw|Reblock-logentry}}",
 	"logentry-partialblock-block-page": "Page portion of {{msg-mw|logentry-partialblock-block}}\n* $1 - number of pages\n* $2 - list of pages",
 	"logentry-partialblock-block-ns": "Namespace portion of {{msg-mw|logentry-partialblock-block}}\n* $1 - number of namespaces\n* $2 - list of namespaces",
 	"logentry-partialblock-block-action": "Block action info portion of {{msg-mw|logentry-partialblock-block}}\n* $1 - number of actions\n* $2 - list of actions",
 	"logentry-partialblock-block": "{{Logentry|[[Special:Log/block]]}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string\n* $7 - Restrictions list – {{msg-mw|logentry-partialblock-block-page}}, {{msg-mw|logentry-partialblock-block-ns}} and/or {{msg-mw|logentry-partialblock-block-action}}\n\nCf. {{msg-mw|Blocklogentry}}",
+	"logentry-partialblock-block-multi": "Log entry message used when a partial block is added and there was already a block on the same user.\n\n{{Logentry|[[Special:Log/block]]}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string\n* $7 - Restrictions list – {{msg-mw|logentry-partialblock-block-page}}, {{msg-mw|logentry-partialblock-block-ns}} and/or {{msg-mw|logentry-partialblock-block-action}}\n\nCf. {{msg-mw|Blocklogentry}}",
 	"logentry-partialblock-reblock": "{{Logentry|[[Special:Log/block]]}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string\n* $7 - Restrictions list {{msg-mw|logentry-partialblock-block-page}}, {{msg-mw|logentry-partialblock-block-ns}} and/or {{msg-mw|logentry-partialblock-block-action}}\n\nCf. {{msg-mw|Reblock-logentry}}",
 	"logentry-non-editing-block-block": "{{Logentry|[[Special:Log/block]]}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string\n\nCf. {{msg-mw|Blocklogentry}}",
+	"logentry-non-editing-block-block-multi": "Log entry message used when a non-editing partial block is added and there was already a block on the same user.\n\n{{Logentry|[[Special:Log/block]]}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string\n\nCf. {{msg-mw|Blocklogentry}}",
 	"logentry-non-editing-block-reblock": "{{Logentry|[[Special:Log/block]]}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string\n\nCf. {{msg-mw|Reblock-logentry}}",
 	"logentry-suppress-block": "{{Logentry}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string",
 	"logentry-suppress-reblock": "{{Logentry}}\n* $4 - username for gender or empty string for autoblocks\n* $5 - the block duration, localized and formatted with the English tooltip\n* $6 - block detail flags or empty string",
diff --git a/tests/phpunit/includes/logging/BlockLogFormatterTest.php b/tests/phpunit/includes/logging/BlockLogFormatterTest.php
index 422575e4cb28..4274fe3e7c97 100644
--- a/tests/phpunit/includes/logging/BlockLogFormatterTest.php
+++ b/tests/phpunit/includes/logging/BlockLogFormatterTest.php
@@ -704,4 +704,45 @@ class BlockLogFormatterTest extends LogFormatterTestCase {
 	public function testPartialBlockLogDatabaseRows( $row, $extra ) {
 		$this->doTestLogFormatter( $row, $extra );
 	}
+
+	public static function provideMultiblocksDatabaseRows() {
+		return [
+			// Sitewide multiblock with expiry
+			[
+				[
+					'type' => 'block',
+					'action' => 'block',
+					'comment' => 'multiblock',
+					'user' => 0,
+					'user_text' => 'Sysop',
+					'namespace' => NS_USER,
+					'title' => 'Target',
+					'timestamp' => '20240101000000',
+					'params' => [
+						'5::duration' => '1 day',
+						'6::flags' => '',
+						'sitewide' => true,
+						'finalTargetCount' => 2,
+					]
+				],
+				[
+					'text' => 'Sysop added a block for Target with an expiration time of 1 day',
+					'api' => [
+						'duration' => '1 day',
+						'flags' => [],
+						'finalTargetCount' => 2,
+						'sitewide' => true,
+						'expiry' => '2024-01-02T00:00:00Z',
+					]
+				]
+			],
+		];
+	}
+
+	/**
+	 * @dataProvider provideMultiblocksDatabaseRows
+	 */
+	public function testMultiblocksDatabaseRows( $row, $extra ) {
+		$this->doTestLogFormatter( $row, $extra );
+	}
 }