Move some misplaced AbuseFilterParser entry points

These methods had no reals reason to be static and belong to the
AbuseFilter class. Most of them were moved to Parser class as common
variations of the existing entry points. One was specific to the
EvalExpression API module and was moved there.

This change comes at no cost, and will make it possible to inject a
parser where needed.

Change-Id: Ifd169cfc99df8a5eb4ca94ac330f301ca28a2442

includes/AbuseFilter.php

@@ -338,74 +338,6 @@ class AbuseFilter {
 		return $generator->addGenericVars()->getVariableHolder();
 	}
 
-	/**
-	 * @param string $filter
-	 * @return true|array True when successful, otherwise a two-element array with exception message
-	 *  and character position of the syntax error
-	 */
-	public static function checkSyntax( $filter ) {
-		try {
-			$res = self::getDefaultParser()->checkSyntax( $filter );
-		} catch ( AFPUserVisibleException $excep ) {
-			$res = [ $excep->getMessageObj()->text(), $excep->mPosition ];
-		}
-		return $res;
-	}
-
-	/**
-	 * @param string $expr
-	 * @return Status
-	 */
-	public static function evaluateExpression( $expr ) {
-		if ( self::checkSyntax( $expr ) !== true ) {
-			return Status::newFatal( 'abusefilter-tools-syntax-error' );
-		}
-
-		$vars = new AbuseFilterVariableHolder();
-		// Generic vars are the only ones available
-		$generator = new VariableGenerator( $vars );
-		$vars = $generator->addGenericVars()->getVariableHolder();
-		$vars->setVar( 'timestamp', wfTimestamp( TS_UNIX ) );
-		$parser = self::getDefaultParser( $vars );
-
-		return Status::newGood( $parser->evaluateExpression( $expr ) );
-	}
-
-	/**
-	 * @param string $conds
-	 * @param AbuseFilterParser $parser The parser instance to use.
-	 * @param bool $ignoreError
-	 * @param string|null $filter The ID of the filter being parsed
-	 * @return bool
-	 * @throws Exception
-	 */
-	public static function checkConditions(
-		$conds, AbuseFilterParser $parser, $ignoreError = true, $filter = null
-	) {
-		try {
-			$result = $parser->parse( $conds );
-		} catch ( Exception $excep ) {
-			$result = false;
-
-			if ( $excep instanceof AFPUserVisibleException ) {
-				$msg = $excep->getMessageForLogs();
-				$excep->setLocalizedMessage();
-			} else {
-				$msg = $excep->getMessage();
-			}
-
-			$logger = LoggerFactory::getInstance( 'AbuseFilter' );
-			$extraInfo = $filter !== null ? " for filter $filter" : '';
-			$logger->warning( "AbuseFilter parser error$extraInfo: $msg" );
-
-			if ( !$ignoreError ) {
-				throw $excep;
-			}
-		}
-
-		return $result;
-	}
-
 	/**
 	 * Returns an associative array of filters which were tripped
 	 *
@@ -1197,7 +1129,7 @@ class AbuseFilter {
 		$user = $context->getUser();
 
 		// Check the syntax
-		$syntaxerr = self::checkSyntax( $request->getVal( 'wpFilterRules' ) );
+		$syntaxerr = self::getDefaultParser()->checkSyntax( $request->getVal( 'wpFilterRules' ) );
 		if ( $syntaxerr !== true ) {
 			$validationStatus->error( 'abusefilter-edit-badsyntax', $syntaxerr[0] );
 			return $validationStatus;

includes/AbuseFilterRunner.php

@@ -433,7 +433,7 @@ class AbuseFilterRunner {
 
 		$pattern = trim( $row->af_pattern );
 		$this->parser->setFilter( $filterName );
-		$result = AbuseFilter::checkConditions( $pattern, $this->parser, true, $filterName );
+		$result = $this->parser->checkConditions( $pattern, true, $filterName );
 
 		$actualExtra = AFComputedVariable::$profilingExtraTime - $origExtraTime;
 		$timeTaken = 1000 * ( microtime( true ) - $startTime - $actualExtra );

includes/Views/AbuseFilterViewTestBatch.php

@@ -157,8 +157,10 @@ class AbuseFilterViewTestBatch extends AbuseFilterView {
 	public function doTest() {
 		// Quick syntax check.
 		$out = $this->getOutput();
-		$result = AbuseFilter::checkSyntax( $this->testPattern );
-		if ( $result !== true ) {
+		$parser = AbuseFilter::getDefaultParser();
+
+		$validSyntax = $parser->checkSyntax( $this->testPattern );
+		if ( $validSyntax !== true ) {
 			$out->addWikiMsg( 'abusefilter-test-syntaxerr' );
 			return;
 		}
@@ -215,6 +217,7 @@ class AbuseFilterViewTestBatch extends AbuseFilterView {
 		$counter = 1;
 
 		$contextUser = $this->getUser();
+		$parser->toggleConditionLimit( false );
 		foreach ( $res as $row ) {
 			$vars = new AbuseFilterVariableHolder();
 			$rc = RecentChange::newFromRow( $row );
@@ -225,9 +228,8 @@ class AbuseFilterViewTestBatch extends AbuseFilterView {
 				continue;
 			}
 
-			$parser = AbuseFilter::getDefaultParser( $vars );
-			$parser->toggleConditionLimit( false );
-			$result = AbuseFilter::checkConditions( $this->testPattern, $parser );
+			$parser->setVariables( $vars );
+			$result = $parser->checkConditions( $this->testPattern );
 
 			if ( $result || $this->mShowNegative ) {
 				// Stash result in RC item

includes/api/ApiAbuseFilterCheckMatch.php

@@ -45,15 +45,19 @@ class ApiAbuseFilterCheckMatch extends ApiBase {
 
 			$vars = AbuseFilter::loadVarDump( $row->afl_var_dump );
 		}
+		if ( $vars === null ) {
+			throw new LogicException( 'Impossible.' );
+		}
 
-		if ( AbuseFilter::checkSyntax( $params[ 'filter' ] ) !== true ) {
+		$parser = AbuseFilter::getDefaultParser();
+		if ( $parser->checkSyntax( $params[ 'filter' ] ) !== true ) {
 			$this->dieWithError( 'apierror-abusefilter-badsyntax', 'badsyntax' );
 		}
 
-		$parser = AbuseFilter::getDefaultParser( $vars );
+		$parser->setVariables( $vars );
 		$result = [
 			ApiResult::META_BC_BOOLS => [ 'result' ],
-			'result' => AbuseFilter::checkConditions( $params['filter'], $parser ),
+			'result' => $parser->checkConditions( $params['filter'] ),
 		];
 
 		$this->getResult()->addValue(

includes/api/ApiAbuseFilterCheckSyntax.php

@@ -12,7 +12,7 @@ class ApiAbuseFilterCheckSyntax extends ApiBase {
 		}
 
 		$params = $this->extractRequestParams();
-		$result = AbuseFilter::checkSyntax( $params[ 'filter' ] );
+		$result = AbuseFilter::getDefaultParser()->checkSyntax( $params[ 'filter' ] );
 
 		$r = [];
 		if ( $result === true ) {

includes/api/ApiAbuseFilterEvalExpression.php

@@ -1,5 +1,7 @@
 <?php
 
+use MediaWiki\Extension\AbuseFilter\VariableGenerator\VariableGenerator;
+
 class ApiAbuseFilterEvalExpression extends ApiBase {
 	/**
 	 * @see ApiBase::execute()
@@ -12,7 +14,7 @@ class ApiAbuseFilterEvalExpression extends ApiBase {
 
 		$params = $this->extractRequestParams();
 
-		$status = AbuseFilter::evaluateExpression( $params['expression'] );
+		$status = $this->evaluateExpression( $params['expression'] );
 		if ( !$status->isGood() ) {
 			$this->dieWithError( $status->getErrors()[0] );
 		} else {
@@ -26,6 +28,26 @@ class ApiAbuseFilterEvalExpression extends ApiBase {
 		}
 	}
 
+	/**
+	 * @param string $expr
+	 * @return Status
+	 */
+	private function evaluateExpression( string $expr ) : Status {
+		$parser = AbuseFilter::getDefaultParser();
+		if ( $parser->checkSyntax( $expr ) !== true ) {
+			return Status::newFatal( 'abusefilter-tools-syntax-error' );
+		}
+
+		$vars = new AbuseFilterVariableHolder();
+		// Generic vars are the only ones available
+		$generator = new VariableGenerator( $vars );
+		$vars = $generator->addGenericVars()->getVariableHolder();
+		$vars->setVar( 'timestamp', wfTimestamp( TS_UNIX ) );
+		$parser->setVariables( $vars );
+
+		return Status::newGood( $parser->evaluateExpression( $expr ) );
+	}
+
 	/**
 	 * @see ApiBase::getAllowedParams()
 	 * @return array

includes/parser/AbuseFilterParser.php

@@ -206,11 +206,12 @@ class AbuseFilterParser extends AFPTransitionBase {
 	}
 
 	/**
+	 * Check the syntax of $filter, throwing an exception if invalid
 	 * @param string $filter
 	 * @return true When successful
 	 * @throws AFPUserVisibleException
 	 */
-	public function checkSyntax( $filter ) {
+	public function checkSyntaxThrow( string $filter ) {
 		$this->allowMissingVariables = true;
 		$origAS = $this->mAllowShort;
 		try {
@@ -224,6 +225,57 @@ class AbuseFilterParser extends AFPTransitionBase {
 		return true;
 	}
 
+	/**
+	 * Check the syntax of $filter, without throwing
+	 *
+	 * @param string $filter
+	 * @return true|array True when successful, otherwise a two-element array with exception message
+	 *  and character position of the syntax error
+	 */
+	public function checkSyntax( string $filter ) {
+		try {
+			$res = $this->checkSyntaxThrow( $filter );
+		} catch ( AFPUserVisibleException $excep ) {
+			$res = [ $excep->getMessageObj()->text(), $excep->mPosition ];
+		}
+		return $res;
+	}
+
+	/**
+	 * This is the main entry point. It checks the given conditions and returns whether
+	 * they match. In case of bad syntax, this is always logged, and $ignoreError can
+	 * be used to determine whether this method should throw.
+	 *
+	 * @param string $conds
+	 * @param bool $ignoreError
+	 * @param string|null $filter The ID of the filter being parsed
+	 * @return bool
+	 * @throws Exception
+	 */
+	public function checkConditions( string $conds, $ignoreError = true, $filter = null ) : bool {
+		try {
+			$result = $this->parse( $conds );
+		} catch ( Exception $excep ) {
+			$result = false;
+
+			if ( $excep instanceof AFPUserVisibleException ) {
+				$msg = $excep->getMessageForLogs();
+				$excep->setLocalizedMessage();
+			} else {
+				$msg = $excep->getMessage();
+			}
+
+			$extraInfo = $filter !== null ? " for filter $filter" : '';
+			$this->logger->warning( "AbuseFilter parser error$extraInfo: $msg" );
+
+			if ( !$ignoreError ) {
+				throw $excep;
+			}
+		}
+
+		return $result;
+	}
+
 	/**
 	 * Move to the next token
 	 */

tests/phpunit/unit/AbuseFilterParserTestCase.php

@@ -60,7 +60,7 @@ abstract class AbuseFilterParserTestCase extends MediaWikiUnitTestCase {
 			try {
 				if ( $skippedBlock ) {
 					// Skipped blocks are, well, skipped when actually parsing.
-					$parser->checkSyntax( $expr );
+					$parser->checkSyntaxThrow( $expr );
 				} else {
 					$parser->parse( $expr );
 				}