Rewrite the VariableHolder code to translate deprecated variables

The current code was more of a subpar, temporary solution. However, we
need a stable solution in case more variables will be deprecated in the
future (T213006 fixes the problem for the past deprecation round). So,
instead of setting a hacky property, directly translate all variables
when loading the var dump. This is not only stable, but has a couple
micro-performance advantages:
 - Calling getDeprecatedVariables happens only once when loading the
   dump, and not every time a variable is accessed
 - No checks are needed when retrieving a variable,
   because names can always assumed to be new

Some simple benchmarks reveals a runtime reduction of 8-15% compared to
the old code (8% when it had varsVersion = 2, 15% for varsVersion = 1),
which comes at no cost together with increased readability and
stability. It ain't much, but it's honest work.

Change-Id: Ib32a92c4ad939790633aa63eb3ef8d4629488bea

includes/AbuseFilter.php

@@ -528,11 +528,7 @@ class AbuseFilter {
 		) {
 			$vars = $obj;
 			$obj = AbuseFilterVariableHolder::newFromArray( $vars );
-			$deprecatedVars = AbuseFilterServices::getKeywordsManager()->getDeprecatedVariables();
-			// If old variable names are used, make sure to keep them
-			if ( array_intersect_key( $deprecatedVars, $obj->getVars() ) ) {
-				$obj->mVarsVersion = 1;
-			}
+			$obj->translateDeprecatedVars();
 		}
 
 		return $obj;

includes/AbuseFilterVariableHolder.php

@@ -29,12 +29,6 @@ class AbuseFilterVariableHolder {
 	/** @var bool Whether this object is being used for an ongoing action being filtered */
 	public $forFilter = false;
 
-	/** @var int 2 is the default and means that new variables names (from T173889) should be used.
-	 *    1 means that the old ones should be used, e.g. if this object is constructed from an
-	 *    afl_var_dump which still bears old variables.
-	 */
-	public $mVarsVersion = 2;
-
 	/**
 	 * @param KeywordsManager|null $keywordsManager Optional for BC
 	 */
@@ -69,6 +63,20 @@ class AbuseFilterVariableHolder {
 		return $ret;
 	}
 
+	/**
+	 * Checks whether any deprecated variable is stored with the old name, and replaces it with
+	 * the new name. This should normally only happen when a DB dump is retrieved from the DB.
+	 */
+	public function translateDeprecatedVars() : void {
+		$deprecatedVars = $this->keywordsManager->getDeprecatedVariables();
+		foreach ( $this->mVars as $name => $value ) {
+			if ( array_key_exists( $name, $deprecatedVars ) ) {
+				$this->mVars[ $deprecatedVars[$name] ] = $value;
+				unset( $this->mVars[$name] );
+			}
+		}
+	}
+
 	/**
 	 * @param string $variable
 	 * @param mixed $datum
@@ -124,13 +132,6 @@ class AbuseFilterVariableHolder {
 	 */
 	public function getVar( $varName, $mode = self::GET_STRICT, $tempFilter = null ) : AFPData {
 		$varName = strtolower( $varName );
-		$deprecatedVars = $this->keywordsManager->getDeprecatedVariables();
-		if ( $this->mVarsVersion === 1 && in_array( $varName, $deprecatedVars ) ) {
-			// Variables are stored with old names, but the parser has given us
-			// a new name. Translate it back.
-			$varName = array_search( $varName, $deprecatedVars );
-		}
-
 		if ( $this->varIsSet( $varName ) ) {
 			/** @var $variable AFComputedVariable|AFPData */
 			$variable = $this->mVars[$varName];

tests/phpunit/AbuseFilterDBTest.php

@@ -1,6 +1,5 @@
 <?php
 
-use MediaWiki\Extension\AbuseFilter\AbuseFilterServices;
 use MediaWiki\Revision\MutableRevisionRecord;
 use MediaWiki\Revision\RevisionRecord;
 use MediaWiki\Revision\SlotRecord;
@@ -50,18 +49,16 @@ class AbuseFilterDBTest extends MediaWikiTestCase {
 	 * Test storing and loading the var dump. See also AbuseFilterConsequencesTest::testVarDump
 	 *
 	 * @param array $variables Map of [ name => value ] to build an AbuseFilterVariableHolder with
+	 * @param ?array $expectedValues Null to use $variables
 	 * @covers AbuseFilter::storeVarDump
 	 * @covers AbuseFilter::loadVarDump
 	 * @covers AbuseFilterVariableHolder::dumpAllVars
 	 * @dataProvider provideVariables
 	 */
-	public function testVarDump( $variables ) {
+	public function testVarDump( array $variables, array $expectedValues = null ) {
 		global $wgCompressRevisions, $wgDefaultExternalStore;
 
 		$holder = AbuseFilterVariableHolder::newFromArray( $variables );
-		if ( array_intersect_key( AbuseFilterServices::getKeywordsManager()->getDeprecatedVariables(), $variables ) ) {
-			$holder->mVarsVersion = 1;
-		}
 
 		$insertID = AbuseFilter::storeVarDump( $holder );
 
@@ -86,7 +83,8 @@ class AbuseFilterDBTest extends MediaWikiTestCase {
 		$this->assertEquals( $expectedFlags, $flags, 'The var dump does not have the correct flags' );
 
 		$dump = AbuseFilter::loadVarDump( "stored-text:$insertID" );
-		$this->assertEquals( $holder, $dump, 'The var dump is not saved correctly' );
+		$expected = $expectedValues ? AbuseFilterVariableHolder::newFromArray( $expectedValues ) : $holder;
+		$this->assertEquals( $expected, $dump, 'The var dump is not saved correctly' );
 	}
 
 	/**
@@ -119,6 +117,13 @@ class AbuseFilterDBTest extends MediaWikiTestCase {
 					'new_wikitext' => 'New text',
 					'article_articleid' => 11745,
 					'article_first_contributor' => 'Good guy'
+				],
+				[
+					'action' => 'edit',
+					'old_wikitext' => 'Old text',
+					'new_wikitext' => 'New text',
+					'page_id' => 11745,
+					'page_first_contributor' => 'Good guy'
 				]
 			],
 			'Move action' => [

tests/phpunit/unit/AbuseFilterVariableHolderTest.php

@@ -85,6 +85,28 @@ class AbuseFilterVariableHolderTest extends MediaWikiUnitTestCase {
 		$this->assertArrayHasKey( 'foo', $vars->getVars(), 'var should be lowercase' );
 	}
 
+	/**
+	 * @covers AbuseFilterVariableHolder::translateDeprecatedVars
+	 */
+	public function testTranslateDeprecatedVars() {
+		$varsMap = [
+			'timestamp' => new AFPData( AFPData::DSTRING, '123' ),
+			'added_lines' => new AFPData( AFPData::DSTRING, 'foobar' ),
+			'article_text' => new AFPData( AFPData::DSTRING, 'FOO' ),
+			'article_articleid' => new AFPData( AFPData::DINT, 42 )
+		];
+		$translatedVarsMap = [
+			'timestamp' => $varsMap['timestamp'],
+			'added_lines' => $varsMap['added_lines'],
+			'page_title' => $varsMap['article_text'],
+			'page_id' => $varsMap['article_articleid']
+		];
+		$keywordsManager = new KeywordsManager( $this->createMock( AbuseFilterHookRunner::class ) );
+		$holder = AbuseFilterVariableHolder::newFromArray( $varsMap, $keywordsManager );
+		$holder->translateDeprecatedVars();
+		$this->assertEquals( $translatedVarsMap, $holder->getVars() );
+	}
+
 	/**
 	 * @param string $name
 	 * @param mixed $val
@@ -167,11 +189,6 @@ class AbuseFilterVariableHolderTest extends MediaWikiUnitTestCase {
 		// For now, strict is the same as lax.
 		yield 'unset, strict' => [ $vars, $name, AbuseFilterVariableHolder::GET_STRICT, $expected ];
 		yield 'unset, bc' => [ $vars, $name, AbuseFilterVariableHolder::GET_BC, new AFPData( AFPData::DNULL ) ];
-
-		$vars->mVarsVersion = 1;
-		$expected = new AFPData( AFPData::DSTRING, 'foo' );
-		$vars->setVar( 'article_text', $expected );
-		yield 'deprecated, with new name' => [ $vars, 'page_title', 0, $expected ];
 	}
 
 	/**