Clean up / simplify parser-related classes

Remove unnecessary setters, injecting everything in the constructor.
These were leftovers from before the introduction of ParserFactory.
Remove public access to the conds used, include the information inside
the returned ParserStatus instead, and consequently simplify callers.

Change-Id: I0a30e044877c6c858af3ff73f819d5ec7c4cc769

includes/FilterRunner.php

@@ -4,7 +4,6 @@ namespace MediaWiki\Extension\AbuseFilter;
 
 use BadMethodCallException;
 use DeferredUpdates;
-use IBufferingStatsdDataFactory;
 use InvalidArgumentException;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Extension\AbuseFilter\ChangeTags\ChangeTagger;
@@ -60,8 +59,6 @@ class FilterRunner {
 	private $stashCache;
 	/** @var LoggerInterface */
 	private $logger;
-	/** @var IBufferingStatsdDataFactory */
-	private $statsdDataFactory;
 	/** @var VariablesManager */
 	private $varManager;
 	/** @var VariableGeneratorFactory */
@@ -110,7 +107,6 @@ class FilterRunner {
 	 * @param Watcher[] $watchers
 	 * @param EditStashCache $stashCache
 	 * @param LoggerInterface $logger
-	 * @param IBufferingStatsdDataFactory $statsdDataFactory
 	 * @param ServiceOptions $options
 	 * @param User $user
 	 * @param Title $title
@@ -132,7 +128,6 @@ class FilterRunner {
 		array $watchers,
 		EditStashCache $stashCache,
 		LoggerInterface $logger,
-		IBufferingStatsdDataFactory $statsdDataFactory,
 		ServiceOptions $options,
 		User $user,
 		Title $title,
@@ -152,7 +147,6 @@ class FilterRunner {
 		$this->watchers = $watchers;
 		$this->stashCache = $stashCache;
 		$this->logger = $logger;
-		$this->statsdDataFactory = $statsdDataFactory;
 
 		$options->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS );
 		if ( !in_array( $group, $options->get( 'AbuseFilterValidGroups' ), true ) ) {
@@ -189,7 +183,6 @@ class FilterRunner {
 		$this->vars->forFilter = true;
 		$this->vars->setVar( 'timestamp', (int)wfTimestamp( TS_UNIX ) );
 		$this->parser = $this->parserFactory->newParser( $this->vars );
-		$this->parser->setStatsd( $this->statsdDataFactory );
 	}
 
 	/**
@@ -327,22 +320,20 @@ class FilterRunner {
 	 * @return RunnerData
 	 */
 	protected function checkAllFiltersInternal(): RunnerData {
-		// Ensure that we start fresh, see T193374
-		$this->parser->resetCondCount();
 		// Ensure there's no extra time leftover
 		LazyVariableComputer::$profilingExtraTime = 0;
 
 		$data = new RunnerData();
 
 		foreach ( $this->filterLookup->getAllActiveFiltersInGroup( $this->group, false ) as $filter ) {
-			[ $status, $profiling ] = $this->checkFilter( $filter );
-			$data->record( $filter->getID(), false, $status, $profiling );
+			[ $status, $timeTaken ] = $this->checkFilter( $filter );
+			$data->record( $filter->getID(), false, $status, $timeTaken );
 		}
 
 		if ( $this->options->get( 'AbuseFilterCentralDB' ) && !$this->options->get( 'AbuseFilterIsCentral' ) ) {
 			foreach ( $this->filterLookup->getAllActiveFiltersInGroup( $this->group, true ) as $filter ) {
-				[ $status, $profiling ] = $this->checkFilter( $filter, true );
-				$data->record( $filter->getID(), true, $status, $profiling );
+				[ $status, $timeTaken ] = $this->checkFilter( $filter, true );
+				$data->record( $filter->getID(), true, $status, $timeTaken );
 			}
 		}
 
@@ -364,13 +355,12 @@ class FilterRunner {
 	 *
 	 * @param ExistingFilter $filter
 	 * @param bool $global
-	 * @return array
-	 * @phan-return array{0:ParserStatus,1:array{time:float,conds:int}}
+	 * @return array [ status, time taken ]
+	 * @phan-return array{0:ParserStatus,1:float}
 	 */
 	protected function checkFilter( ExistingFilter $filter, bool $global = false ): array {
 		$filterName = GlobalNameUtils::buildGlobalName( $filter->getID(), $global );
 
-		$startConds = $this->parser->getCondCount();
 		$startTime = microtime( true );
 		$origExtraTime = LazyVariableComputer::$profilingExtraTime;
 
@@ -379,14 +369,8 @@ class FilterRunner {
 
 		$actualExtra = LazyVariableComputer::$profilingExtraTime - $origExtraTime;
 		$timeTaken = 1000 * ( microtime( true ) - $startTime - $actualExtra );
-		$condsUsed = $this->parser->getCondCount() - $startConds;
 
-		$profiling = [
-			'time' => $timeTaken,
-			'conds' => $condsUsed,
-		];
-
-		return [ $status, $profiling ];
+		return [ $status, $timeTaken ];
 	}
 
 	/**

includes/FilterRunnerFactory.php

@@ -151,7 +151,6 @@ class FilterRunnerFactory {
 				$group
 			),
 			$this->logger,
-			$this->statsdDataFactory,
 			$this->options,
 			$user,
 			$title,

includes/Parser/FilterEvaluator.php

@@ -16,7 +16,6 @@ use MediaWiki\Extension\AbuseFilter\Parser\Exception\UserVisibleWarning;
 use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder;
 use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager;
 use MWException;
-use NullStatsdDataFactory;
 use Psr\Log\LoggerInterface;
 use Sanitizer;
 use Wikimedia\AtEase\AtEase;
@@ -105,7 +104,7 @@ class FilterEvaluator {
 	];
 
 	// Functions that affect parser state, and shouldn't be cached.
-	public const ACTIVE_FUNCTIONS = [
+	private const ACTIVE_FUNCTIONS = [
 		'funcSetVar',
 	];
 
@@ -122,71 +121,71 @@ class FilterEvaluator {
 	/**
 	 * @var bool Are we allowed to use short-circuit evaluation?
 	 */
-	public $mAllowShort;
+	private $mAllowShort;
 
 	/**
 	 * @var VariableHolder
 	 */
-	public $mVariables;
+	private $mVariables;
 	/**
 	 * @var int The current amount of conditions being consumed
 	 */
-	protected $mCondCount;
+	private $mCondCount;
 	/**
 	 * @var bool Whether the condition limit is enabled.
 	 */
-	protected $condLimitEnabled = true;
+	private $condLimitEnabled = true;
 	/**
 	 * @var string|null The ID of the filter being parsed, if available. Can also be "global-$ID"
 	 */
-	protected $mFilter;
+	private $mFilter;
 	/**
 	 * @var bool Whether we can allow retrieving _builtin_ variables not included in $this->mVariables
 	 */
-	protected $allowMissingVariables = false;
+	private $allowMissingVariables = false;
 
 	/**
 	 * @var BagOStuff Used to cache the AST and the tokens
 	 */
-	protected $cache;
+	private $cache;
 	/**
 	 * @var bool Whether the AST was retrieved from cache
 	 */
-	protected $fromCache = false;
+	private $fromCache = false;
 	/**
 	 * @var LoggerInterface Used for debugging
 	 */
-	protected $logger;
+	private $logger;
 	/**
 	 * @var Language Content language, used for language-dependent functions
 	 */
-	protected $contLang;
+	private $contLang;
 	/**
 	 * @var IBufferingStatsdDataFactory
 	 */
-	protected $statsd;
+	private $statsd;
 
 	/** @var KeywordsManager */
-	protected $keywordsManager;
+	private $keywordsManager;
 
 	/** @var VariablesManager */
-	protected $varManager;
+	private $varManager;
 
 	/** @var int */
 	private $conditionsLimit;
 
 	/** @var UserVisibleWarning[] */
-	protected $warnings = [];
+	private $warnings = [];
 
 	/**
 	 * @var array Cached results of functions
 	 */
-	protected $funcCache = [];
+	private $funcCache = [];
 
 	/**
 	 * @var Equivset
 	 */
-	protected static $equivset;
+	private static $equivset;
 
 	/**
 	 * Create a new instance
@@ -196,6 +195,7 @@ class FilterEvaluator {
 	 * @param LoggerInterface $logger Used for debugging
 	 * @param KeywordsManager $keywordsManager
 	 * @param VariablesManager $varManager
+	 * @param IBufferingStatsdDataFactory $statsdDataFactory
 	 * @param int $conditionsLimit
 	 * @param VariableHolder|null $vars
 	 */
@@ -205,13 +205,14 @@ class FilterEvaluator {
 		LoggerInterface $logger,
 		KeywordsManager $keywordsManager,
 		VariablesManager $varManager,
+		IBufferingStatsdDataFactory $statsdDataFactory,
 		int $conditionsLimit,
 		VariableHolder $vars = null
 	) {
 		$this->contLang = $contLang;
 		$this->cache = $cache;
 		$this->logger = $logger;
-		$this->statsd = new NullStatsdDataFactory;
+		$this->statsd = $statsdDataFactory;
 		$this->keywordsManager = $keywordsManager;
 		$this->varManager = $varManager;
 		$this->conditionsLimit = $conditionsLimit;
@@ -228,41 +229,6 @@ class FilterEvaluator {
 		$this->mFilter = $filter;
 	}
 
-	/**
-	 * @param BagOStuff $cache
-	 */
-	public function setCache( BagOStuff $cache ) {
-		$this->cache = $cache;
-	}
-
-	/**
-	 * @param LoggerInterface $logger
-	 */
-	public function setLogger( LoggerInterface $logger ) {
-		$this->logger = $logger;
-	}
-
-	/**
-	 * @param IBufferingStatsdDataFactory $statsd
-	 */
-	public function setStatsd( IBufferingStatsdDataFactory $statsd ) {
-		$this->statsd = $statsd;
-	}
-
-	/**
-	 * @return int
-	 */
-	public function getCondCount() {
-		return $this->mCondCount;
-	}
-
-	/**
-	 * Reset the conditions counter
-	 */
-	public function resetCondCount() {
-		$this->mCondCount = 0;
-	}
-
 	/**
 	 * For use in batch scripts and the like
 	 *
@@ -276,7 +242,7 @@ class FilterEvaluator {
 	 * @param int $val The amount to increase the conditions count of.
 	 * @throws ConditionLimitException
 	 */
-	protected function raiseCondCount( $val = 1 ) {
+	private function raiseCondCount( $val = 1 ) {
 		$this->mCondCount += $val;
 
 		if ( $this->condLimitEnabled && $this->mCondCount > $this->conditionsLimit ) {
@@ -284,13 +250,6 @@ class FilterEvaluator {
 		}
 	}
 
-	/**
-	 * Clears the array of cached function results
-	 */
-	public function clearFuncCache() {
-		$this->funcCache = [];
-	}
-
 	/**
 	 * @param VariableHolder $vars
 	 */
@@ -361,12 +320,20 @@ class FilterEvaluator {
 	 * @return ParserStatus The result indicates whether the syntax is valid
 	 */
 	public function checkSyntax( string $filter ): ParserStatus {
+		$initialConds = $this->mCondCount;
 		try {
 			$valid = $this->checkSyntaxThrow( $filter );
 		} catch ( UserVisibleException $excep ) {
 			$valid = false;
 		}
-		return new ParserStatus( $valid, $this->fromCache, $excep ?? null, $this->warnings );
+
+		return new ParserStatus(
+			$valid,
+			$this->fromCache,
+			$excep ?? null,
+			$this->warnings,
+			$this->mCondCount - $initialConds
+		);
 	}
 
 	/**
@@ -399,7 +366,7 @@ class FilterEvaluator {
 	 * @param string $code
 	 * @return AFPData
 	 */
-	public function intEval( $code ): AFPData {
+	private function intEval( $code ): AFPData {
 		$startTime = microtime( true );
 		$tree = $this->getTree( $code );
 		$res = $this->evalTree( $tree );
@@ -426,12 +393,13 @@ class FilterEvaluator {
 	 */
 	public function parseDetailed( string $code ): ParserStatus {
 		$excep = null;
+		$initialConds = $this->mCondCount;
 		try {
 			$res = $this->parse( $code );
 		} catch ( ExceptionBase $excep ) {
 			$res = false;
 		}
-		return new ParserStatus( $res, $this->fromCache, $excep, $this->warnings );
+		return new ParserStatus( $res, $this->fromCache, $excep, $this->warnings, $this->mCondCount - $initialConds );
 	}
 
 	/**
@@ -793,7 +761,7 @@ class FilterEvaluator {
 
 		if ( count( $this->funcCache ) > 1000 ) {
 			// @codeCoverageIgnoreStart
-			$this->clearFuncCache();
+			$this->funcCache = [];
 			// @codeCoverageIgnoreEnd
 		}
 		return $result;
@@ -869,7 +837,7 @@ class FilterEvaluator {
 			? VariablesManager::GET_LAX
 			// TODO: This should be GET_STRICT, but that's going to be very hard (see T230256)
 			: VariablesManager::GET_BC;
-		return $this->varManager->getVar( $this->mVariables, $var, $flags, $this->mFilter );
+		return $this->varManager->getVar( $this->mVariables, $var, $flags );
 	}
 
 	/**

includes/Parser/ParserFactory.php

@@ -3,6 +3,7 @@
 namespace MediaWiki\Extension\AbuseFilter\Parser;
 
 use BagOStuff;
+use IBufferingStatsdDataFactory;
 use Language;
 use MediaWiki\Extension\AbuseFilter\KeywordsManager;
 use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder;
@@ -27,6 +28,9 @@ class ParserFactory {
 	/** @var VariablesManager */
 	private $varManager;
 
+	/** @var IBufferingStatsdDataFactory */
+	private $statsdDataFactory;
+
 	/** @var int */
 	private $conditionsLimit;
 
@@ -36,6 +40,7 @@ class ParserFactory {
 	 * @param LoggerInterface $logger
 	 * @param KeywordsManager $keywordsManager
 	 * @param VariablesManager $varManager
+	 * @param IBufferingStatsdDataFactory $statsdDataFactory
 	 * @param int $conditionsLimit
 	 */
 	public function __construct(
@@ -44,6 +49,7 @@ class ParserFactory {
 		LoggerInterface $logger,
 		KeywordsManager $keywordsManager,
 		VariablesManager $varManager,
+		IBufferingStatsdDataFactory $statsdDataFactory,
 		int $conditionsLimit
 	) {
 		$this->contLang = $contLang;
@@ -51,6 +57,7 @@ class ParserFactory {
 		$this->logger = $logger;
 		$this->keywordsManager = $keywordsManager;
 		$this->varManager = $varManager;
+		$this->statsdDataFactory = $statsdDataFactory;
 		$this->conditionsLimit = $conditionsLimit;
 	}
 
@@ -65,6 +72,7 @@ class ParserFactory {
 			$this->logger,
 			$this->keywordsManager,
 			$this->varManager,
+			$this->statsdDataFactory,
 			$this->conditionsLimit,
 			$vars
 		);

includes/Parser/ParserStatus.php

@@ -14,18 +14,28 @@ class ParserStatus {
 	private $excep;
 	/** @var UserVisibleWarning[] */
 	private $warnings;
+	/** @var int */
+	private $condsUsed;
 
 	/**
 	 * @param bool $result A generic operation result
 	 * @param bool $warmCache Whether we retrieved the AST from cache
 	 * @param ExceptionBase|null $excep An exception thrown while parsing, or null if it parsed correctly
 	 * @param UserVisibleWarning[] $warnings
+	 * @param int $condsUsed
 	 */
-	public function __construct( bool $result, bool $warmCache, ?ExceptionBase $excep, array $warnings ) {
+	public function __construct(
+		bool $result,
+		bool $warmCache,
+		?ExceptionBase $excep,
+		array $warnings,
+		int $condsUsed
+	) {
 		$this->result = $result;
 		$this->warmCache = $warmCache;
 		$this->excep = $excep;
 		$this->warnings = $warnings;
+		$this->condsUsed = $condsUsed;
 	}
 
 	/**
@@ -56,6 +66,13 @@ class ParserStatus {
 		return $this->warnings;
 	}
 
+	/**
+	 * @return int
+	 */
+	public function getCondsUsed(): int {
+		return $this->condsUsed;
+	}
+
 	/**
 	 * Serialize data for edit stash
 	 * @return array
@@ -71,6 +88,7 @@ class ParserStatus {
 				},
 				$this->warnings
 			),
+			'condsUsed' => $this->condsUsed,
 		];
 	}
 
@@ -85,7 +103,8 @@ class ParserStatus {
 			$value['result'],
 			$value['warmCache'],
 			$excClass !== null ? call_user_func( [ $excClass, 'fromArray' ], $value['exception'] ) : null,
-			array_map( [ UserVisibleWarning::class, 'fromArray' ], $value['warnings'] )
+			array_map( [ UserVisibleWarning::class, 'fromArray' ], $value['warnings'] ),
+			$value['condsUsed']
 		);
 	}
 

includes/RunnerData.php

@@ -52,18 +52,21 @@ class RunnerData {
 	 * @param int $filterID
 	 * @param bool $global
 	 * @param ParserStatus $status
-	 * @param array $profilingData
-	 * @phan-param array{time:float,conds:int} $profilingData
+	 * @param float $timeTaken
 	 */
-	public function record( int $filterID, bool $global, ParserStatus $status, array $profilingData ): void {
+	public function record( int $filterID, bool $global, ParserStatus $status, float $timeTaken ): void {
 		$key = GlobalNameUtils::buildGlobalName( $filterID, $global );
 		if ( array_key_exists( $key, $this->matchedFilters ) ) {
 			throw new LogicException( "Filter '$key' has already been recorded" );
 		}
 		$this->matchedFilters[$key] = $status;
-		$this->profilingData[$key] = $profilingData + [ 'result' => $status->getResult() ];
-		$this->totalRuntime += $profilingData['time'];
-		$this->totalConditions += $profilingData['conds'];
+		$this->profilingData[$key] = [
+			'time' => $timeTaken,
+			'conds' => $status->getCondsUsed(),
+			'result' => $status->getResult()
+		];
+		$this->totalRuntime += $timeTaken;
+		$this->totalConditions += $status->getCondsUsed();
 	}
 
 	/**

includes/ServiceWiring.php

@@ -117,6 +117,7 @@ return [
 			LoggerFactory::getInstance( 'AbuseFilter' ),
 			$services->getService( KeywordsManager::SERVICE_NAME ),
 			$services->get( VariablesManager::SERVICE_NAME ),
+			$services->getStatsdDataFactory(),
 			$services->getMainConfig()->get( 'AbuseFilterConditionLimit' )
 		);
 	},
@@ -335,8 +336,7 @@ return [
 	VariablesManager::SERVICE_NAME => static function ( MediaWikiServices $services ): VariablesManager {
 		return new VariablesManager(
 			$services->get( KeywordsManager::SERVICE_NAME ),
-			$services->get( LazyVariableComputer::SERVICE_NAME ),
-			LoggerFactory::getInstance( 'AbuseFilter' )
+			$services->get( LazyVariableComputer::SERVICE_NAME )
 		);
 	},
 	VariableGeneratorFactory::SERVICE_NAME => static function (

includes/Variables/VariablesManager.php

@@ -5,8 +5,6 @@ namespace MediaWiki\Extension\AbuseFilter\Variables;
 use LogicException;
 use MediaWiki\Extension\AbuseFilter\KeywordsManager;
 use MediaWiki\Extension\AbuseFilter\Parser\AFPData;
-use Psr\Log\LoggerInterface;
-use RuntimeException;
 
 /**
  * Service that allows manipulating a VariableHolder
@@ -25,22 +23,17 @@ class VariablesManager {
 	private $keywordsManager;
 	/** @var LazyVariableComputer */
 	private $lazyComputer;
-	/** @var LoggerInterface */
-	private $logger;
 
 	/**
 	 * @param KeywordsManager $keywordsManager
 	 * @param LazyVariableComputer $lazyComputer
-	 * @param LoggerInterface $logger
 	 */
 	public function __construct(
 		KeywordsManager $keywordsManager,
-		LazyVariableComputer $lazyComputer,
-		LoggerInterface $logger
+		LazyVariableComputer $lazyComputer
 	) {
 		$this->keywordsManager = $keywordsManager;
 		$this->lazyComputer = $lazyComputer;
-		$this->logger = $logger;
 	}
 
 	/**
@@ -68,14 +61,12 @@ class VariablesManager {
 	 *  - GET_STRICT -> In the future, this will throw an exception. For now it returns a DUNDEFINED and logs a warning
 	 *  - GET_LAX -> Return a DUNDEFINED AFPData
 	 *  - GET_BC -> Return a DNULL AFPData (this should only be used for BC, see T230256)
-	 * @param string|null $tempFilter Filter ID, if available; only used for debugging (temporarily)
 	 * @return AFPData
 	 */
 	public function getVar(
 		VariableHolder $holder,
 		string $varName,
-		$mode = self::GET_STRICT,
-		$tempFilter = null
+		$mode = self::GET_STRICT
 	): AFPData {
 		$varName = strtolower( $varName );
 		if ( $holder->varIsSet( $varName ) ) {
@@ -102,16 +93,7 @@ class VariablesManager {
 		// The variable is not set.
 		switch ( $mode ) {
 			case self::GET_STRICT:
-				$this->logger->warning(
-					__METHOD__ . ": requested unset variable {varname} in strict mode, filter: {filter}",
-					[
-						'varname' => $varName,
-						'exception' => new RuntimeException(),
-						'filter' => $tempFilter ?? 'unavailable'
-					]
-				);
-				// @todo change the line below to throw an exception in a future MW version
-				return new AFPData( AFPData::DUNDEFINED );
+				throw new UnsetVariableException( $varName );
 			case self::GET_LAX:
 				return new AFPData( AFPData::DUNDEFINED );
 			case self::GET_BC:

tests/phpunit/integration/Api/CheckMatchTest.php

@@ -44,8 +44,8 @@ class CheckMatchTest extends ApiTestCase {
 	 */
 	public function testExecute_Ok( bool $expected ) {
 		$filter = 'sampleFilter';
-		$checkStatus = new ParserStatus( true, false, null, [] );
-		$resultStatus = new ParserStatus( $expected, false, null, [] );
+		$checkStatus = new ParserStatus( true, false, null, [], 1 );
+		$resultStatus = new ParserStatus( $expected, false, null, [], 1 );
 		$parser = $this->createMock( FilterEvaluator::class );
 		$parser->expects( $this->once() )
 			->method( 'checkSyntax' )->with( $filter )
@@ -79,7 +79,7 @@ class CheckMatchTest extends ApiTestCase {
 	public function testExecute_error() {
 		$this->setExpectedApiException( 'apierror-abusefilter-badsyntax', 'badsyntax' );
 		$filter = 'sampleFilter';
-		$status = new ParserStatus( false, false, null, [] );
+		$status = new ParserStatus( false, false, null, [], 1 );
 		$parser = $this->createMock( FilterEvaluator::class );
 		$parser->expects( $this->once() )
 			->method( 'checkSyntax' )->with( $filter )

tests/phpunit/integration/Api/CheckSyntaxTest.php

@@ -36,7 +36,7 @@ class CheckSyntaxTest extends ApiTestCase {
 	 */
 	public function testExecute_Ok() {
 		$input = 'sampleFilter';
-		$status = new ParserStatus( true, false, null, [] );
+		$status = new ParserStatus( true, false, null, [], 1 );
 		$parser = $this->createMock( FilterEvaluator::class );
 		$parser->method( 'checkSyntax' )->with( $input )
 			->willReturn( $status );
@@ -64,7 +64,7 @@ class CheckSyntaxTest extends ApiTestCase {
 			new UserVisibleWarning( 'exception-1', 3, [] ),
 			new UserVisibleWarning( 'exception-2', 8, [ 'param' ] ),
 		];
-		$status = new ParserStatus( true, false, null, $warnings );
+		$status = new ParserStatus( true, false, null, $warnings, 1 );
 		$parser = $this->createMock( FilterEvaluator::class );
 		$parser->method( 'checkSyntax' )->with( $input )
 			->willReturn( $status );
@@ -110,7 +110,7 @@ class CheckSyntaxTest extends ApiTestCase {
 	public function testExecute_error() {
 		$input = 'sampleFilter';
 		$exception = new UserVisibleException( 'error-id', 4, [] );
-		$status = new ParserStatus( false, false, $exception, [] );
+		$status = new ParserStatus( false, false, $exception, [], 1 );
 		$parser = $this->createMock( FilterEvaluator::class );
 		$parser->method( 'checkSyntax' )->with( $input )
 			->willReturn( $status );

tests/phpunit/integration/Api/EvalExpressionTest.php

@@ -36,7 +36,7 @@ class EvalExpressionTest extends ApiTestCase {
 	public function testExecute_error() {
 		$this->setExpectedApiException( 'abusefilter-tools-syntax-error' );
 		$expression = 'sampleExpression';
-		$status = new ParserStatus( false, false, null, [] );
+		$status = new ParserStatus( false, false, null, [], 1 );
 		$parser = $this->createMock( FilterEvaluator::class );
 		$parser->method( 'checkSyntax' )->with( $expression )
 			->willReturn( $status );
@@ -54,7 +54,7 @@ class EvalExpressionTest extends ApiTestCase {
 	 */
 	public function testExecute_Ok() {
 		$expression = 'sampleExpression';
-		$status = new ParserStatus( true, false, null, [] );
+		$status = new ParserStatus( true, false, null, [], 1 );
 		$parser = $this->createMock( FilterEvaluator::class );
 		$parser->method( 'checkSyntax' )->with( $expression )
 			->willReturn( $status );
@@ -86,7 +86,7 @@ class EvalExpressionTest extends ApiTestCase {
 	 */
 	public function testExecute_OkAndPrettyPrint() {
 		$expression = 'sampleExpression';
-		$status = new ParserStatus( true, false, null, [] );
+		$status = new ParserStatus( true, false, null, [], 1 );
 		$parser = $this->createMock( FilterEvaluator::class );
 		$parser->method( 'checkSyntax' )->with( $expression )
 			->willReturn( $status );

tests/phpunit/integration/Parser/ParserEquivsetTest.php

@@ -31,6 +31,7 @@ use MediaWiki\Extension\AbuseFilter\Tests\Unit\Parser\ParserTest;
 use MediaWiki\Extension\AbuseFilter\Variables\LazyVariableComputer;
 use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager;
 use MediaWikiIntegrationTestCase;
+use NullStatsdDataFactory;
 
 /**
  * Tests that require Equivset, separated from the parser unit tests.
@@ -62,8 +63,7 @@ class ParserEquivsetTest extends MediaWikiIntegrationTestCase {
 		$keywordsManager = AbuseFilterServices::getKeywordsManager();
 		$varManager = new VariablesManager(
 			$keywordsManager,
-			$this->createMock( LazyVariableComputer::class ),
-			$logger
+			$this->createMock( LazyVariableComputer::class )
 		);
 
 		$evaluator = new FilterEvaluator(
@@ -72,6 +72,7 @@ class ParserEquivsetTest extends MediaWikiIntegrationTestCase {
 			$logger,
 			$keywordsManager,
 			$varManager,
+			new NullStatsdDataFactory(),
 			1000
 		);
 		$evaluator->toggleConditionLimit( false );

tests/phpunit/unit/EditStashCacheTest.php

@@ -11,7 +11,6 @@ use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager;
 use MediaWikiUnitTestCase;
 use NullStatsdDataFactory;
 use Psr\Log\LoggerInterface;
-use Psr\Log\NullLogger;
 use TitleValue;
 
 /**
@@ -23,8 +22,7 @@ class EditStashCacheTest extends MediaWikiUnitTestCase {
 	private function getVariablesManager(): VariablesManager {
 		return new VariablesManager(
 			$this->createMock( KeywordsManager::class ),
-			$this->createMock( LazyVariableComputer::class ),
-			new NullLogger()
+			$this->createMock( LazyVariableComputer::class )
 		);
 	}
 

tests/phpunit/unit/FilterRunnerTest.php

@@ -2,7 +2,6 @@
 
 namespace MediaWiki\Extension\AbuseFilter\Tests\Unit;
 
-use IBufferingStatsdDataFactory;
 use InvalidArgumentException;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\Extension\AbuseFilter\AbuseLoggerFactory;
@@ -63,7 +62,7 @@ class FilterRunnerTest extends MediaWikiUnitTestCase {
 			$this->createMock( FilterProfiler::class ),
 			$changeTagger ?? $this->createMock( ChangeTagger::class ),
 			$this->createMock( FilterLookup::class ),
-			$parserFactory ?? $this->createMock( ParserFactory::class ),
+			$this->createMock( ParserFactory::class ),
 			$this->createMock( ConsequencesExecutorFactory::class ),
 			$this->createMock( AbuseLoggerFactory::class ),
 			$this->createMock( VariablesManager::class ),
@@ -72,7 +71,6 @@ class FilterRunnerTest extends MediaWikiUnitTestCase {
 			[],
 			$cache,
 			new NullLogger(),
-			$this->createMock( IBufferingStatsdDataFactory::class ),
 			$opts,
 			$this->createMock( User::class ),
 			$this->createMock( Title::class ),

tests/phpunit/unit/FilterValidatorTest.php

@@ -43,7 +43,7 @@ class FilterValidatorTest extends MediaWikiUnitTestCase {
 		if ( !$parser ) {
 			$parser = $this->createMock( FilterEvaluator::class );
 			$parser->method( 'checkSyntax' )->willReturn(
-				new ParserStatus( true, true, null, [] )
+				new ParserStatus( true, true, null, [], 1 )
 			);
 		}
 		$parserFactory = $this->createMock( ParserFactory::class );
@@ -102,7 +102,7 @@ class FilterValidatorTest extends MediaWikiUnitTestCase {
 	 */
 	public function testCheckValidSyntax( bool $valid, ?ExceptionBase $excep, ?string $expected, ?array $expParams ) {
 		$parser = $this->createMock( FilterEvaluator::class );
-		$syntaxStatus = new ParserStatus( $valid, true, $excep, [] );
+		$syntaxStatus = new ParserStatus( $valid, true, $excep, [], 1 );
 		$parser->method( 'checkSyntax' )->willReturn( $syntaxStatus );
 		$validator = $this->getFilterValidator( null, $parser );
 
@@ -390,7 +390,7 @@ class FilterValidatorTest extends MediaWikiUnitTestCase {
 		$noopFilter->method( 'isEnabled' )->willReturn( true );
 
 		$parser = $this->createMock( FilterEvaluator::class );
-		$syntaxStatus = new ParserStatus( false, true, $this->createMock( UserVisibleException::class ), [] );
+		$syntaxStatus = new ParserStatus( false, true, $this->createMock( UserVisibleException::class ), [], 1 );
 		$parser->method( 'checkSyntax' )->willReturn( $syntaxStatus );
 		yield 'invalid syntax' => [ $noopFilter, 'abusefilter-edit-badsyntax', null, $parser ];
 

tests/phpunit/unit/Parser/ParserFactoryTest.php

@@ -9,6 +9,7 @@ use MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator;
 use MediaWiki\Extension\AbuseFilter\Parser\ParserFactory;
 use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager;
 use MediaWikiUnitTestCase;
+use NullStatsdDataFactory;
 use Psr\Log\NullLogger;
 
 /**
@@ -30,6 +31,7 @@ class ParserFactoryTest extends MediaWikiUnitTestCase {
 			new NullLogger(),
 			$this->createMock( KeywordsManager::class ),
 			$this->createMock( VariablesManager::class ),
+			new NullStatsdDataFactory(),
 			1000
 		);
 		$this->assertInstanceOf( FilterEvaluator::class, $factory->newParser() );

tests/phpunit/unit/Parser/ParserStatusTest.php

@@ -24,17 +24,20 @@ class ParserStatusTest extends MediaWikiUnitTestCase {
 	 * @covers ::getWarmCache
 	 * @covers ::getException
 	 * @covers ::getWarnings
+	 * @covers ::getCondsUsed
 	 */
 	public function testGetters() {
 		$result = true;
 		$warm = false;
 		$exc = $this->createMock( UserVisibleException::class );
 		$warnings = [ new UserVisibleWarning( 'foo', 1, [] ) ];
-		$status = new ParserStatus( $result, $warm, $exc, $warnings );
+		$condsUsed = 42;
+		$status = new ParserStatus( $result, $warm, $exc, $warnings, $condsUsed );
 		$this->assertSame( $result, $status->getResult() );
 		$this->assertSame( $warm, $status->getWarmCache() );
 		$this->assertSame( $exc, $status->getException() );
 		$this->assertSame( $warnings, $status->getWarnings() );
+		$this->assertSame( $condsUsed, $status->getCondsUsed() );
 	}
 
 	public function provideToArrayException() {
@@ -52,7 +55,8 @@ class ParserStatusTest extends MediaWikiUnitTestCase {
 			true,
 			false,
 			$exception,
-			[ new UserVisibleWarning( 'foo', 1, [] ) ]
+			[ new UserVisibleWarning( 'foo', 1, [] ) ],
+			42
 		);
 		$newStatus = ParserStatus::fromArray( $status->toArray() );
 		$this->assertSame( $status->getResult(), $newStatus->getResult() );
@@ -63,5 +67,6 @@ class ParserStatusTest extends MediaWikiUnitTestCase {
 			$this->assertNull( $newStatus->getException() );
 		}
 		$this->assertContainsOnlyInstancesOf( UserVisibleWarning::class, $newStatus->getWarnings() );
+		$this->assertSame( $status->getCondsUsed(), $newStatus->getCondsUsed() );
 	}
 }

tests/phpunit/unit/Parser/ParserTest.php

@@ -23,17 +23,13 @@
 
 namespace MediaWiki\Extension\AbuseFilter\Tests\Unit\Parser;
 
-use BagOStuff;
 use Generator;
-use IBufferingStatsdDataFactory;
 use MediaWiki\Extension\AbuseFilter\Hooks\AbuseFilterHookRunner;
 use MediaWiki\Extension\AbuseFilter\KeywordsManager;
 use MediaWiki\Extension\AbuseFilter\Parser\AbuseFilterTokenizer;
 use MediaWiki\Extension\AbuseFilter\Parser\Exception\UserVisibleException;
 use MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator;
 use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder;
-use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager;
-use Psr\Log\NullLogger;
 use TestLogger;
 use Wikimedia\TestingAccessWrapper;
 
@@ -161,12 +157,7 @@ class ParserTest extends ParserTestCase {
 	 * @dataProvider condCountCases
 	 */
 	public function testCondCount( $rule, $expected ) {
-		$parser = $this->getParser();
-		$countBefore = $parser->getCondCount();
-		$parser->parse( $rule );
-		$countAfter = $parser->getCondCount();
-		$actual = $countAfter - $countBefore;
-		$this->assertEquals( $expected, $actual, "Rule: $rule" );
+		$this->assertEquals( $expected, $this->getParser()->parseDetailed( $rule )->getCondsUsed(), "Rule: $rule" );
 	}
 
 	/**
@@ -786,10 +777,9 @@ class ParserTest extends ParserTestCase {
 		// Set it under the new name, and check that the old name points to it
 		$vars = VariableHolder::newFromArray( [ $new => 'value' ] );
 
-		$parser = $this->getParser();
 		$loggerMock = new TestLogger();
 		$loggerMock->setCollect( true );
-		$parser->setLogger( $loggerMock );
+		$parser = $this->getParser( $loggerMock );
 
 		$parser->setVariables( $vars );
 		$actual = $parser->parse( "$old === $new" );
@@ -1154,21 +1144,6 @@ class ParserTest extends ParserTestCase {
 		$this->assertSame( $funcs, $argsCount );
 	}
 
-	/**
-	 * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator::__construct
-	 */
-	public function testConstructorInitsVars() {
-		$lang = $this->getLanguageMock();
-		$cache = $this->createMock( BagOStuff::class );
-		$logger = new NullLogger();
-		$keywordsManager = $this->createMock( KeywordsManager::class );
-		$varManager = $this->createMock( VariablesManager::class );
-		$vars = new VariableHolder();
-
-		$parser = new FilterEvaluator( $lang, $cache, $logger, $keywordsManager, $varManager, 1000, $vars );
-		$this->assertSame( $vars, $parser->mVariables, 'Variables should be initialized' );
-	}
-
 	/**
 	 * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator::setFilter
 	 */
@@ -1180,50 +1155,6 @@ class ParserTest extends ParserTestCase {
 		$this->assertSame( $filter, $parser->mFilter );
 	}
 
-	/**
-	 * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator::setCache
-	 */
-	public function testSetCache() {
-		$parser = TestingAccessWrapper::newFromObject( $this->getParser() );
-		$cache = $this->createMock( BagOStuff::class );
-		$parser->setCache( $cache );
-		$this->assertSame( $cache, $parser->cache );
-	}
-
-	/**
-	 * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator::setLogger
-	 */
-	public function testSetLogger() {
-		$parser = TestingAccessWrapper::newFromObject( $this->getParser() );
-		$logger = new NullLogger();
-		$parser->setLogger( $logger );
-		$this->assertSame( $logger, $parser->logger );
-	}
-
-	/**
-	 * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator::setStatsd
-	 */
-	public function testSetStatsd() {
-		$parser = TestingAccessWrapper::newFromObject( $this->getParser() );
-		$statsd = $this->createMock( IBufferingStatsdDataFactory::class );
-		$parser->setStatsd( $statsd );
-		$this->assertSame( $statsd, $parser->statsd );
-	}
-
-	/**
-	 * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator::getCondCount
-	 * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator::resetCondCount
-	 */
-	public function testCondCountMethods() {
-		$parser = TestingAccessWrapper::newFromObject( $this->getParser() );
-		$this->assertSame( 0, $parser->mCondCount, 'precondition' );
-		$val = 42;
-		$parser->mCondCount = $val;
-		$this->assertSame( $val, $parser->getCondCount(), 'after set' );
-		$parser->resetCondCount( $val );
-		$this->assertSame( 0, $parser->getCondCount(), 'after reset' );
-	}
-
 	/**
 	 * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator::toggleConditionLimit
 	 */
@@ -1237,18 +1168,6 @@ class ParserTest extends ParserTestCase {
 		$this->assertTrue( $parser->condLimitEnabled );
 	}
 
-	/**
-	 * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator::clearFuncCache
-	 */
-	public function testClearFuncCache() {
-		$parser = TestingAccessWrapper::newFromObject( $this->getParser() );
-
-		$parser->funcCache = [ 1, 2, 3 ];
-
-		$parser->clearFuncCache();
-		$this->assertSame( [], $parser->funcCache );
-	}
-
 	/**
 	 * @covers \MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator::setVariables
 	 */

tests/phpunit/unit/Parser/ParserTestCase.php

@@ -32,26 +32,28 @@ use MediaWiki\Extension\AbuseFilter\Parser\FilterEvaluator;
 use MediaWiki\Extension\AbuseFilter\Variables\LazyVariableComputer;
 use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager;
 use MediaWikiUnitTestCase;
+use NullStatsdDataFactory;
 use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
 
 /**
  * Helper for parser-related tests
  */
 abstract class ParserTestCase extends MediaWikiUnitTestCase {
 	/**
+	 * @param LoggerInterface|null $logger
 	 * @return FilterEvaluator
 	 */
-	protected function getParser() {
+	protected function getParser( LoggerInterface $logger = null ) {
 		// We're not interested in caching or logging; tests should call respectively setCache
 		// and setLogger if they want to test any of those.
 		$contLang = $this->getLanguageMock();
 		$cache = new EmptyBagOStuff();
-		$logger = new \Psr\Log\NullLogger();
+		$logger = $logger ?? new \Psr\Log\NullLogger();
 		$keywordsManager = new KeywordsManager( $this->createMock( AbuseFilterHookRunner::class ) );
 		$varManager = new VariablesManager(
 			$keywordsManager,
-			$this->createMock( LazyVariableComputer::class ),
-			$logger
+			$this->createMock( LazyVariableComputer::class )
 		);
 
 		$evaluator = new FilterEvaluator(
@@ -60,6 +62,7 @@ abstract class ParserTestCase extends MediaWikiUnitTestCase {
 			$logger,
 			$keywordsManager,
 			$varManager,
+			new NullStatsdDataFactory(),
 			1000
 		);
 		$evaluator->toggleConditionLimit( false );

tests/phpunit/unit/RunnerDataTest.php

@@ -39,13 +39,13 @@ class RunnerDataTest extends MediaWikiUnitTestCase {
 		$runnerData = new RunnerData();
 		$runnerData->record(
 			1, false,
-			new ParserStatus( true, false, null, [] ),
-			[ 'time' => 12.3, 'conds' => 7 ]
+			new ParserStatus( true, false, null, [], 7 ),
+			12.3
 		);
 		$runnerData->record(
 			1, true,
-			new ParserStatus( false, false, null, [] ),
-			[ 'time' => 23.4, 'conds' => 5 ]
+			new ParserStatus( false, false, null, [], 5 ),
+			23.4
 		);
 
 		$this->assertArrayEquals(
@@ -74,14 +74,14 @@ class RunnerDataTest extends MediaWikiUnitTestCase {
 		$runnerData = new RunnerData();
 		$runnerData->record(
 			1, false,
-			new ParserStatus( true, false, null, [] ),
-			[ 'time' => 12.3, 'conds' => 7 ]
+			new ParserStatus( true, false, null, [], 7 ),
+			12.3
 		);
 		$this->expectException( LogicException::class );
 		$runnerData->record(
 			1, false,
-			new ParserStatus( false, false, null, [] ),
-			[ 'time' => 23.4, 'conds' => 5 ]
+			new ParserStatus( false, false, null, [], 5 ),
+			23.4
 		);
 	}
 
@@ -98,14 +98,15 @@ class RunnerDataTest extends MediaWikiUnitTestCase {
 				true,
 				false,
 				null,
-				[ new UserVisibleWarning( 'match-empty-regex', 3, [] ) ]
+				[ new UserVisibleWarning( 'match-empty-regex', 3, [] ) ],
+				7
 			),
-			[ 'time' => 12.3, 'conds' => 7 ]
+			12.3
 		);
 		$runnerData->record(
 			1, true,
-			new ParserStatus( false, false, null, [] ),
-			[ 'time' => 23.4, 'conds' => 5 ]
+			new ParserStatus( false, false, null, [], 5 ),
+			23.4
 		);
 		$newData = RunnerData::fromArray( $runnerData->toArray() );
 		$this->assertSame( $runnerData->getTotalConditions(), $newData->getTotalConditions() );
@@ -122,23 +123,23 @@ class RunnerDataTest extends MediaWikiUnitTestCase {
 		$runnerData = new RunnerData();
 		$runnerData->record(
 			1, false,
-			new ParserStatus( true, false, null, [] ),
-			[ 'time' => 12.3, 'conds' => 7 ]
+			new ParserStatus( true, false, null, [], 7 ),
+			12.3
 		);
 		$runnerData->record(
 			1, true,
-			new ParserStatus( false, false, null, [] ),
-			[ 'time' => 23.4, 'conds' => 5 ]
+			new ParserStatus( false, false, null, [], 5 ),
+			23.4
 		);
 		$runnerData->record(
 			3, false,
-			new ParserStatus( false, false, null, [] ),
-			[ 'time' => 12.3, 'conds' => 7 ]
+			new ParserStatus( false, false, null, [], 7 ),
+			12.3
 		);
 		$runnerData->record(
 			3, true,
-			new ParserStatus( true, false, null, [] ),
-			[ 'time' => 23.4, 'conds' => 5 ]
+			new ParserStatus( true, false, null, [], 5 ),
+			23.4
 		);
 
 		$this->assertArrayEquals(

tests/phpunit/unit/Variables/VariablesManagerTest.php

@@ -9,11 +9,10 @@ use MediaWiki\Extension\AbuseFilter\KeywordsManager;
 use MediaWiki\Extension\AbuseFilter\Parser\AFPData;
 use MediaWiki\Extension\AbuseFilter\Variables\LazyLoadedVariable;
 use MediaWiki\Extension\AbuseFilter\Variables\LazyVariableComputer;
+use MediaWiki\Extension\AbuseFilter\Variables\UnsetVariableException;
 use MediaWiki\Extension\AbuseFilter\Variables\VariableHolder;
 use MediaWiki\Extension\AbuseFilter\Variables\VariablesManager;
 use MediaWikiUnitTestCase;
-use Psr\Log\LoggerInterface;
-use Psr\Log\NullLogger;
 
 /**
  * @group Test
@@ -24,18 +23,15 @@ class VariablesManagerTest extends MediaWikiUnitTestCase {
 	/**
 	 * @param LazyVariableComputer|null $lazyComputer
 	 * @param KeywordsManager|null $keywordsManager
-	 * @param LoggerInterface|null $logger
 	 * @return VariablesManager
 	 */
 	private function getManager(
 		LazyVariableComputer $lazyComputer = null,
-		KeywordsManager $keywordsManager = null,
-		LoggerInterface $logger = null
+		KeywordsManager $keywordsManager = null
 	): VariablesManager {
 		return new VariablesManager(
 			$keywordsManager ?? new KeywordsManager( $this->createMock( AbuseFilterHookRunner::class ) ),
-			$lazyComputer ?? $this->createMock( LazyVariableComputer::class ),
-			$logger ?? new NullLogger()
+			$lazyComputer ?? $this->createMock( LazyVariableComputer::class )
 		);
 	}
 
@@ -187,7 +183,7 @@ class VariablesManagerTest extends MediaWikiUnitTestCase {
 	 * @param VariableHolder $holder
 	 * @param string $name
 	 * @param int $flags
-	 * @param AFPData $expected
+	 * @param AFPData|string $expected String if expecting an exception
 	 * @covers ::getVar
 	 *
 	 * @dataProvider provideGetVar
@@ -197,9 +193,14 @@ class VariablesManagerTest extends MediaWikiUnitTestCase {
 		VariableHolder $holder,
 		string $name,
 		int $flags,
-		AFPData $expected
+		$expected
 	) {
-		$this->assertEquals( $expected, $manager->getVar( $holder, $name, $flags ) );
+		if ( is_string( $expected ) ) {
+			$this->expectException( $expected );
+			$manager->getVar( $holder, $name, $flags );
+		} else {
+			$this->assertEquals( $expected, $manager->getVar( $holder, $name, $flags ) );
+		}
 	}
 
 	/**
@@ -239,8 +240,13 @@ class VariablesManagerTest extends MediaWikiUnitTestCase {
 		$name = 'not-set';
 		$expected = new AFPData( AFPData::DUNDEFINED );
 		yield 'unset, lax' => [ $this->getManager(), $vars, $name, VariablesManager::GET_LAX, $expected ];
-		// For now, strict is the same as lax.
-		yield 'unset, strict' => [ $this->getManager(), $vars, $name, VariablesManager::GET_STRICT, $expected ];
+		yield 'unset, strict' => [
+			$this->getManager(),
+			$vars,
+			$name,
+			VariablesManager::GET_STRICT,
+			UnsetVariableException::class
+		];
 		yield 'unset, bc' => [
 			$this->getManager(),
 			$vars,
@@ -300,8 +306,7 @@ class VariablesManagerTest extends MediaWikiUnitTestCase {
 			VariablesManager::class,
 			new VariablesManager(
 				$this->createMock( KeywordsManager::class ),
-				$this->createMock( LazyVariableComputer::class ),
-				new NullLogger()
+				$this->createMock( LazyVariableComputer::class )
 			)
 		);
 	}