Minor tweaks to edit constraints

- EditConstraintRunner debug logging uses `info` for the failed constraint
- Differentiate between the two PageSizeConstraints in debug logs
- Add or tweak some comments
- Normalize SpamRegexConstraint logs

Bug: T157658
Change-Id: I9d0e8d48a104320f29831357b45a4003942b04fd

includes/editpage/Constraint/EditConstraintFactory.php

@@ -66,7 +66,11 @@ class EditConstraintFactory {
 	 * that need dependencies injected.
 	 *
 	 * The checks in EditPage use wfDebugLog and logged to different channels, hence the need
-	 * for multiple loggers retrieved from the Spi. TODO can they be combined into the same channel?
+	 * for multiple loggers retrieved from the Spi. The channels used are:
+	 * - SimpleAntiSpam (in SimpleAntiSpamConstraint)
+	 * - SpamRegex (in SpamRegexConstraint)
+	 *
+	 * TODO can they be combined into the same channel?
 	 *
 	 * @param ServiceOptions $options
 	 * @param Spi $loggerFactory

includes/editpage/Constraint/EditConstraintRunner.php

@@ -86,23 +86,50 @@ class EditConstraintRunner {
 	public function checkConstraints() : bool {
 		foreach ( $this->constraints as $constraint ) {
 			$result = $constraint->checkConstraint();
-			$fullClassName = explode( '\\', get_class( $constraint ) );
-			$this->logger->debug(
-				'Checked {class}, got result: {result}',
-				[
-					'class' => end( $fullClassName ),
-					'result' => $result
-				]
-			);
-
 			if ( $result !== IEditConstraint::CONSTRAINT_PASSED ) {
+				// Use `info` instead of `debug` for the one constraint that failed
+				$this->logger->info(
+					'Checked {name}, got result: {result}',
+					[
+						'name' => $this->getConstraintName( $constraint ),
+						'result' => $result
+					]
+				);
+
 				$this->failedConstraint = $constraint;
 				return false;
 			}
+
+			// Pass, log at `debug` level
+			$this->logger->debug(
+				'Checked {name}, got result: {result}',
+				[
+					'name' => $this->getConstraintName( $constraint ),
+					'result' => $result
+				]
+			);
 		}
 		return true;
 	}
 
+	/**
+	 * @param IEditConstraint $constraint
+	 * @return string
+	 */
+	private function getConstraintName( IEditConstraint $constraint ) : string {
+		// Used for debug logging
+		$fullClassName = explode( '\\', get_class( $constraint ) );
+		$constraintName = end( $fullClassName );
+		if ( $constraint instanceof PageSizeConstraint ) {
+			// TODO "When you need to do this instanceof, it's a code smell"
+			// Convert IEditConstraint to abstract class with a getName method
+			// once the initial migration to edit constraints is complete
+			// PageSizeConstraint is used twice, make sure they can be told apart
+			$constraintName .= ' ' . $constraint->getType();
+		}
+		return $constraintName;
+	}
+
 	/**
 	 * Get the constraint that failed
 	 *

includes/editpage/Constraint/PageSizeConstraint.php

@@ -27,8 +27,7 @@ use StatusValue;
  * Verify the page isn't larger than the maximum
  *
  * This is used for both checking the size //before// merging the edit, and checking the size
- * //after// applying the edit, and the result codes they return are different, hence the need
- * to pass the error code in the constructor to avoid duplication
+ * //after// applying the edit, and the result codes they return are different.
  *
  * @since 1.36
  * @internal
@@ -52,6 +51,9 @@ class PageSizeConstraint implements IEditConstraint {
 	/** @var int */
 	private $errorCode;
 
+	/** @var string */
+	private $type;
+
 	/**
 	 * @param int $maxSize In kilobytes, from $wgMaxArticleSize
 	 * @param int $contentSize
@@ -72,6 +74,8 @@ class PageSizeConstraint implements IEditConstraint {
 		} else {
 			throw new InvalidArgumentException( "Invalid type: $type" );
 		}
+
+		$this->type = $type;
 	}
 
 	public function checkConstraint() : string {
@@ -90,4 +94,15 @@ class PageSizeConstraint implements IEditConstraint {
 		return $statusValue;
 	}
 
+	/**
+	 * Get the type, so that the two different uses of this constraint can be told
+	 * apart in debug logs.
+	 * @internal
+	 * @codeCoverageIgnore
+	 * @return string
+	 */
+	public function getType() : string {
+		return $this->type;
+	}
+
 }

includes/editpage/Constraint/SpamRegexConstraint.php

@@ -104,12 +104,14 @@ class SpamRegexConstraint implements IEditConstraint {
 		}
 
 		$this->match = $match;
-		$ip = $this->reqIP;
-		$prefixedDBkey = $this->title->getPrefixedDBkey();
-
-		// TODO use context instead of concatenation for easier grepping
-		$match = str_replace( "\n", '', $match );
-		$this->logger->debug( "$ip spam regex hit [[$prefixedDBkey]]: \"$match\"" );
+		$this->logger->debug(
+			'{ip} spam regex hit [[{title}]]: "{match}"',
+			[
+				'ip' => $this->reqIP,
+				'title' => $this->title->getPrefixedDBkey(),
+				'match' => str_replace( "\n", '', $match )
+			]
+		);
 		return self::CONSTRAINT_FAILED;
 	}
 

includes/editpage/Constraint/UserRateLimitConstraint.php

@@ -54,6 +54,7 @@ class UserRateLimitConstraint implements IEditConstraint {
 	}
 
 	public function checkConstraint() : string {
+		// TODO inject and use a ThrottleStore once available, see T261744
 		// Checking if the user is rate limited increments the counts, so we cannot perform
 		// the check again when getting the status; thus, store the result
 		if ( $this->user->pingLimiter()

tests/phpunit/unit/includes/editpage/Constraint/EditConstraintRunnerTest.php

@@ -20,7 +20,9 @@
 
 use MediaWiki\EditPage\Constraint\EditConstraintRunner;
 use MediaWiki\EditPage\Constraint\IEditConstraint;
+use MediaWiki\EditPage\Constraint\PageSizeConstraint;
 use Wikimedia\Assert\PreconditionException;
+use Wikimedia\TestingAccessWrapper;
 
 /**
  * Tests the EditConstraintRunner
@@ -83,4 +85,17 @@ class EditConstraintRunnerTest extends MediaWikiUnitTestCase {
 		$runner->getFailedConstraint();
 	}
 
+	public function testGetConstraintName() {
+		$runner = TestingAccessWrapper::newFromObject( new EditConstraintRunner() );
+		$pageSizeConstraint = new PageSizeConstraint(
+			1,
+			512,
+			PageSizeConstraint::BEFORE_MERGE
+		);
+		$this->assertSame(
+			'PageSizeConstraint ' . PageSizeConstraint::BEFORE_MERGE,
+			$runner->getConstraintName( $pageSizeConstraint )
+		);
+	}
+
 }

tests/phpunit/unit/includes/editpage/Constraint/SpamRegexConstraintTest.php

@@ -104,7 +104,7 @@ class SpamRegexConstraintTest extends MediaWikiUnitTestCase {
 		$this->assertSame( [
 			[
 				LogLevel::DEBUG,
-				"Request-IP spam regex hit [[$prefixedDBKey]]: \"$matchingText\""
+				'{ip} spam regex hit [[{title}]]: "{match}"'
 			],
 		], $logger->getBuffer() );
 		$logger->clearBuffer();