diff --git a/includes/editpage/Constraint/EditConstraintFactory.php b/includes/editpage/Constraint/EditConstraintFactory.php index 035c8b269b4..70e6a2c281c 100644 --- a/includes/editpage/Constraint/EditConstraintFactory.php +++ b/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 diff --git a/includes/editpage/Constraint/EditConstraintRunner.php b/includes/editpage/Constraint/EditConstraintRunner.php index 0987423fd7c..dfd9e52ce83 100644 --- a/includes/editpage/Constraint/EditConstraintRunner.php +++ b/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 * diff --git a/includes/editpage/Constraint/PageSizeConstraint.php b/includes/editpage/Constraint/PageSizeConstraint.php index 1938c01db73..0996864c701 100644 --- a/includes/editpage/Constraint/PageSizeConstraint.php +++ b/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; + } + } diff --git a/includes/editpage/Constraint/SpamRegexConstraint.php b/includes/editpage/Constraint/SpamRegexConstraint.php index 9e6b63fc478..3043a4383f7 100644 --- a/includes/editpage/Constraint/SpamRegexConstraint.php +++ b/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; } diff --git a/includes/editpage/Constraint/UserRateLimitConstraint.php b/includes/editpage/Constraint/UserRateLimitConstraint.php index 66dfa9d851d..91d3c510173 100644 --- a/includes/editpage/Constraint/UserRateLimitConstraint.php +++ b/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() diff --git a/tests/phpunit/unit/includes/editpage/Constraint/EditConstraintRunnerTest.php b/tests/phpunit/unit/includes/editpage/Constraint/EditConstraintRunnerTest.php index fe7d4176a02..570f1973d5a 100644 --- a/tests/phpunit/unit/includes/editpage/Constraint/EditConstraintRunnerTest.php +++ b/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 ) + ); + } + } diff --git a/tests/phpunit/unit/includes/editpage/Constraint/SpamRegexConstraintTest.php b/tests/phpunit/unit/includes/editpage/Constraint/SpamRegexConstraintTest.php index c5512d21cd1..2a1d66908bc 100644 --- a/tests/phpunit/unit/includes/editpage/Constraint/SpamRegexConstraintTest.php +++ b/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();