From 95ed38a0f379f1869b8690aba4cbc4566204f5b0 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Fri, 27 May 2016 13:58:34 -0700
Subject: [PATCH] Clean up setVisibility() log type logic

The log type used should be based on whether any item
was (un)suppressed, not just the last log entry. Otherwise,
unsuppression could end up in the wrong log if the last
item in the list was not unsuppressed but others were.

Change-Id: I7b6af524cc45a1d83b2b719bfa00138531455e35
---
 includes/revisiondelete/RevDelList.php | 56 ++++++++++++++------------
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/includes/revisiondelete/RevDelList.php b/includes/revisiondelete/RevDelList.php
index b555592471..79d66a9d8a 100644
--- a/includes/revisiondelete/RevDelList.php
+++ b/includes/revisiondelete/RevDelList.php
@@ -125,6 +125,7 @@ abstract class RevDelList extends RevisionListBase {
 			$status->itemStatuses = [];
 		}
 
+		$logType = 'delete';
 		// @codingStandardsIgnoreStart Generic.CodeAnalysis.ForLoopWithTestFunctionCall.NotAllowed
 		for ( $this->reset(); $this->current(); $this->next() ) {
 			// @codingStandardsIgnoreEnd
@@ -144,7 +145,8 @@ abstract class RevDelList extends RevisionListBase {
 			$newBits = RevisionDeleter::extractBitfield( $bitPars, $oldBits );
 
 			if ( $oldBits == $newBits ) {
-				$itemStatus->warning( 'revdelete-no-change', $item->formatDate(), $item->formatTime() );
+				$itemStatus->warning(
+					'revdelete-no-change', $item->formatDate(), $item->formatTime() );
 				$status->failCount++;
 				continue;
 			} elseif ( $oldBits == 0 && $newBits != 0 ) {
@@ -157,21 +159,21 @@ abstract class RevDelList extends RevisionListBase {
 
 			if ( $item->isHideCurrentOp( $newBits ) ) {
 				// Cannot hide current version text
-				$itemStatus->error( 'revdelete-hide-current', $item->formatDate(), $item->formatTime() );
+				$itemStatus->error(
+					'revdelete-hide-current', $item->formatDate(), $item->formatTime() );
 				$status->failCount++;
 				continue;
-			}
-			if ( !$item->canView() ) {
+			} elseif ( !$item->canView() ) {
 				// Cannot access this revision
 				$msg = ( $opType == 'show' ) ?
 					'revdelete-show-no-access' : 'revdelete-modify-no-access';
 				$itemStatus->error( $msg, $item->formatDate(), $item->formatTime() );
 				$status->failCount++;
 				continue;
-			}
 			// Cannot just "hide from Sysops" without hiding any fields
-			if ( $newBits == Revision::DELETED_RESTRICTED ) {
-				$itemStatus->warning( 'revdelete-only-restricted', $item->formatDate(), $item->formatTime() );
+			} elseif ( $newBits == Revision::DELETED_RESTRICTED ) {
+				$itemStatus->warning(
+					'revdelete-only-restricted', $item->formatDate(), $item->formatTime() );
 				$status->failCount++;
 				continue;
 			}
@@ -181,6 +183,11 @@ abstract class RevDelList extends RevisionListBase {
 
 			if ( $ok ) {
 				$idsForLog[] = $item->getId();
+				// If any item field was suppressed or unsupressed
+				if ( ( $oldBits | $newBits ) & $this->getSuppressBit() ) {
+					$logType = 'suppress';
+				}
+
 				$status->successCount++;
 				if ( $item->getAuthorId() > 0 ) {
 					$authorIds[] = $item->getAuthorId();
@@ -188,7 +195,8 @@ abstract class RevDelList extends RevisionListBase {
 					$authorIPs[] = $item->getAuthorName();
 				}
 			} else {
-				$itemStatus->error( 'revdelete-concurrent-change', $item->formatDate(), $item->formatTime() );
+				$itemStatus->error(
+					'revdelete-concurrent-change', $item->formatDate(), $item->formatTime() );
 				$status->failCount++;
 			}
 		}
@@ -221,16 +229,19 @@ abstract class RevDelList extends RevisionListBase {
 
 		// Log it
 		// @FIXME: $newBits/$oldBits set in for loop, makes IDE warnings too
-		$this->updateLog( [
-			'title' => $this->title,
-			'count' => $successCount,
-			'newBits' => $newBits,
-			'oldBits' => $oldBits,
-			'comment' => $comment,
-			'ids' => $idsForLog,
-			'authorIds' => $authorIds,
-			'authorIPs' => $authorIPs
-		] );
+		$this->updateLog(
+			$logType,
+			[
+				'title' => $this->title,
+				'count' => $successCount,
+				'newBits' => $newBits,
+				'oldBits' => $oldBits,
+				'comment' => $comment,
+				'ids' => $idsForLog,
+				'authorIds' => $authorIds,
+				'authorIPs' => $authorIPs
+			]
+		);
 
 		// Clear caches
 		$that = $this;
@@ -254,6 +265,7 @@ abstract class RevDelList extends RevisionListBase {
 
 	/**
 	 * Record a log entry on the action
+	 * @param string $logType One of (delete,suppress)
 	 * @param array $params Associative array of parameters:
 	 *     newBits:         The new value of the *_deleted bitfield
 	 *     oldBits:         The old value of the *_deleted bitfield.
@@ -264,18 +276,12 @@ abstract class RevDelList extends RevisionListBase {
 	 *     authorsIPs:      The array of the IP/anon user offenders
 	 * @throws MWException
 	 */
-	protected function updateLog( $params ) {
+	private function updateLog( $logType, $params ) {
 		// Get the URL param's corresponding DB field
 		$field = RevisionDeleter::getRelationType( $this->getType() );
 		if ( !$field ) {
 			throw new MWException( "Bad log URL param type!" );
 		}
-		// Put things hidden from sysops in the suppression log
-		if ( ( $params['newBits'] | $params['oldBits'] ) & $this->getSuppressBit() ) {
-			$logType = 'suppress';
-		} else {
-			$logType = 'delete';
-		}
 		// Add params for affected page and ids
 		$logParams = $this->getLogParams( $params );
 		// Actually add the deletion log entry
-- 
2.20.1