Clean up dependency injection in RevertedTagUpdate

- Use ICP instead of ILB (T330641) - Use ChangeTagsStore instead of ChangeTags static (T245964) - Clean up all of the weirdness in phpunit testing because of lack of DI in ChangeTags Change-Id: I674b9f7450425c089324cf3b11447e0078941a33
2023-06-21 23:18:33 +02:00 · 2023-06-21 23:18:33 +02:00 · f97280cf2f
commit f97280cf2f
parent bcd6c5eaac
3 changed files with 93 additions and 173 deletions
--- a/includes/Storage/RevertedTagUpdate.php
+++ b/includes/Storage/RevertedTagUpdate.php
@ -23,12 +23,13 @@ namespace MediaWiki\Storage;
 use ChangeTags;
 use DeferrableUpdate;
 use FormatJson;
+use MediaWiki\ChangeTags\ChangeTagsStore;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\MainConfigNames;
 use MediaWiki\Revision\RevisionRecord;
 use MediaWiki\Revision\RevisionStore;
 use Psr\Log\LoggerInterface;
-use Wikimedia\Rdbms\ILoadBalancer;
+use Wikimedia\Rdbms\IConnectionProvider;

 /**
 * Adds the mw-reverted tag to reverted edits after a revert is made.
@ -51,11 +52,8 @@ class RevertedTagUpdate implements DeferrableUpdate {
 	/** @var LoggerInterface */
 	private $logger;

-	/** @var string[] */
-	private $softwareTags;
-
-	/** @var ILoadBalancer */
-	private $loadBalancer;
+	/** @var IConnectionProvider */
+	private $dbProvider;

 	/** @var ServiceOptions */
 	private $options;
@ -74,13 +72,13 @@ class RevertedTagUpdate implements DeferrableUpdate {

 	/** @var RevisionRecord|null */
 	private $oldestRevertedRevision;
+	private ChangeTagsStore $changeTagsStore;

 	/**
 	 * @param RevisionStore $revisionStore
 	 * @param LoggerInterface $logger
-	 * @param string[] $softwareTags Array of currently enabled software change tags. Can be
-	 *        obtained from ChangeTags::getSoftwareTags()
-	 * @param ILoadBalancer $loadBalancer
+	 * @param ChangeTagsStore $changeTagsStore
+	 * @param IConnectionProvider $dbProvider
 	 * @param ServiceOptions $serviceOptions
 	 * @param int $revertId ID of the revert
 	 * @param EditResult $editResult EditResult object of this revert
@ -88,8 +86,8 @@ class RevertedTagUpdate implements DeferrableUpdate {
 	public function __construct(
 		RevisionStore $revisionStore,
 		LoggerInterface $logger,
-		array $softwareTags,
-		ILoadBalancer $loadBalancer,
+		ChangeTagsStore $changeTagsStore,
+		IConnectionProvider $dbProvider,
 		ServiceOptions $serviceOptions,
 		int $revertId,
 		EditResult $editResult
@ -98,11 +96,11 @@ class RevertedTagUpdate implements DeferrableUpdate {

 		$this->revisionStore = $revisionStore;
 		$this->logger = $logger;
-		$this->softwareTags = $softwareTags;
-		$this->loadBalancer = $loadBalancer;
+		$this->dbProvider = $dbProvider;
 		$this->options = $serviceOptions;
 		$this->revertId = $revertId;
 		$this->editResult = $editResult;
+		$this->changeTagsStore = $changeTagsStore;
 	}

 	/**
@ -156,10 +154,12 @@ class RevertedTagUpdate implements DeferrableUpdate {
 				// See: T265312
 				continue;
 			}
-
-			$this->markAsReverted(
+			$this->changeTagsStore->addTags(
+				[ ChangeTags::TAG_REVERTED ],
+				null,
 				$revId,
-				$extraParams
+				null,
+				FormatJson::encode( $extraParams )
 			);
 		}
 	}
@ -171,7 +171,10 @@ class RevertedTagUpdate implements DeferrableUpdate {
 	 */
 	private function shouldExecute(): bool {
 		$maxDepth = $this->options->get( MainConfigNames::RevertedTagMaxDepth );
-		if ( !in_array( ChangeTags::TAG_REVERTED, $this->softwareTags ) || $maxDepth <= 0 ) {
+		if (
+			!in_array( ChangeTags::TAG_REVERTED, $this->changeTagsStore->getSoftwareTags() ) ||
+			$maxDepth <= 0
+		) {
 			return false;
 		}

@ -223,7 +226,11 @@ class RevertedTagUpdate implements DeferrableUpdate {
 			return false;
 		}

-		$changeTagsOnRevert = $this->getChangeTags( $this->revertId );
+		$changeTagsOnRevert = $this->changeTagsStore->getTags(
+			$this->dbProvider->getReplicaDatabase(),
+			null,
+			$this->revertId
+		);
 		if ( in_array( ChangeTags::TAG_REVERTED, $changeTagsOnRevert ) ) {
 			// This is already marked as reverted, which means the update was delayed
 			// until the edit is approved. Apparently, the edit was not approved, as
@ -272,56 +279,14 @@ class RevertedTagUpdate implements DeferrableUpdate {
 			// is executed.
 			return true;
 		}
-
-		$this->markAsReverted(
-			// @phan-suppress-next-line PhanTypeMismatchArgumentNullable revertedRevision is already checked
-			$this->editResult->getOldestRevertedRevisionId(),
-			$this->getTagExtraParams()
-		);
-		return true;
-	}
-
-	/**
-	 * Protected function calling static ChangeTags class to allow for unit testing of this
-	 * deferrable update.
-	 *
-	 * This class is not stable for extending, this is just to make the class testable.
-	 *
-	 * ChangeTags should be passed by dependency injection when that becomes possible.
-	 * See: T245964
-	 *
-	 * @param int $revisionId ID of the revision to mark as reverted
-	 * @param array $extraParams Params to put in the ct_params field of table 'change_tag'
-	 */
-	protected function markAsReverted( int $revisionId, array $extraParams ) {
-		ChangeTags::addTags(
+		$this->changeTagsStore->addTags(
 			[ ChangeTags::TAG_REVERTED ],
 			null,
-			$revisionId,
+			$this->editResult->getOldestRevertedRevisionId(),
 			null,
-			FormatJson::encode( $extraParams )
-		);
-	}
-
-	/**
-	 * Protected function calling static ChangeTags class to allow for unit testing of this
-	 * deferrable update.
-	 *
-	 * This class is not stable for extending, this is just to make the class testable.
-	 *
-	 * ChangeTags should be passed by dependency injection when that becomes possible.
-	 * See: T245964
-	 *
-	 * @param int $revisionId
-	 *
-	 * @return string[]
-	 */
-	protected function getChangeTags( int $revisionId ) {
-		return ChangeTags::getTags(
-			$this->loadBalancer->getConnectionRef( DB_REPLICA ),
-			null,
-			$revisionId
+			FormatJson::encode( $this->getTagExtraParams() )
 		);
+		return true;
 	}

 	/**
--- a/includes/jobqueue/jobs/RevertedTagUpdateJob.php
+++ b/includes/jobqueue/jobs/RevertedTagUpdateJob.php
@ -77,8 +77,8 @@ class RevertedTagUpdateJob extends Job implements GenericParameterJob {
 		$update = new RevertedTagUpdate(
 			$services->getRevisionStore(),
 			LoggerFactory::getInstance( 'RevertedTagUpdate' ),
-			ChangeTags::getSoftwareTags(),
-			$services->getDBLoadBalancer(),
+			$services->getChangeTagsStore(),
+			$services->getDBLoadBalancerFactory(),
 			new ServiceOptions(
 				RevertedTagUpdate::CONSTRUCTOR_OPTIONS,
 				$services->getMainConfig()
--- a/tests/phpunit/unit/includes/Storage/RevertedTagUpdateTest.php
+++ b/tests/phpunit/unit/includes/Storage/RevertedTagUpdateTest.php
@ -2,6 +2,9 @@

 namespace MediaWiki\Tests\Storage;

+use ChangeTags;
+use FormatJson;
+use MediaWiki\ChangeTags\ChangeTagsStore;
 use MediaWiki\Config\ServiceOptions;
 use MediaWiki\MainConfigNames;
 use MediaWiki\Revision\MutableRevisionRecord;
@ -15,7 +18,7 @@ use PHPUnit\Framework\MockObject\Stub\ReturnCallback;
 use Psr\Log\LoggerInterface;
 use Psr\Log\LogLevel;
 use TestLogger;
-use Wikimedia\Rdbms\ILoadBalancer;
+use Wikimedia\Rdbms\IConnectionProvider;

 /**
 * @covers \MediaWiki\Storage\RevertedTagUpdate
@ -25,12 +28,9 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 	use MockTitleTrait;

 	/**
-	 * Convenience function for creating a RevertedTagUpdate that does not use
-	 * the ChangeTags static class. Instead, a mock for a future ChangeTags-like
-	 * object should be provided.
-	 * TODO: clean this up once T245964 is resolved
+	 * Convenience function for creating a RevertedTagUpdate object
 	 *
-	 * @param FutureChangeTags $futureChangeTags
+	 * @param ChangeTagsStore $changeTagsStore
 	 * @param RevisionStore $revisionStore
 	 * @param LoggerInterface $logger
 	 * @param string[] $softwareTags
@ -41,7 +41,7 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 	 * @return RevertedTagUpdate
 	 */
 	private function newRevertedTagUpdate(
-		$futureChangeTags,
+		$changeTagsStore,
 		RevisionStore $revisionStore,
 		LoggerInterface $logger,
 		array $softwareTags,
@ -49,62 +49,24 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 		int $revertId,
 		EditResult $editResult
 	): RevertedTagUpdate {
-		// LoadBalancer is never used in unit tests because getTags is overridden
-		$loadBalancer = $this->createNoOpMock( ILoadBalancer::class );
+		$dbProvider = $this->createMock( IConnectionProvider::class );
+		$changeTagsStore->method( 'getSoftwareTags' )
+			->willReturn( $softwareTags );

-		return new class(
-			$futureChangeTags,
+		$serviceOptions = new ServiceOptions(
+			RevertedTagUpdate::CONSTRUCTOR_OPTIONS,
+			[ MainConfigNames::RevertedTagMaxDepth => $revertedTagMaxDepth ]
+		);
+
+		return new RevertedTagUpdate(
 			$revisionStore,
 			$logger,
-			$softwareTags,
-			$loadBalancer,
-			$revertedTagMaxDepth,
+			$changeTagsStore,
+			$dbProvider,
+			$serviceOptions,
 			$revertId,
 			$editResult
-		) extends RevertedTagUpdate {
-
-			protected $futureChangeTags;
-
-			public function __construct(
-				$futureChangeTags,
-				RevisionStore $revisionStore,
-				LoggerInterface $logger,
-				array $softwareTags,
-				ILoadBalancer $loadBalancer,
-				int $revertedTagMaxDepth,
-				int $revertId,
-				EditResult $editResult
-			) {
-				$serviceOptions = new ServiceOptions(
-					RevertedTagUpdate::CONSTRUCTOR_OPTIONS,
-					[ MainConfigNames::RevertedTagMaxDepth => $revertedTagMaxDepth ]
-				);
-
-				parent::__construct(
-					$revisionStore,
-					$logger,
-					$softwareTags,
-					$loadBalancer,
-					$serviceOptions,
-					$revertId,
-					$editResult
-				);
-				$this->futureChangeTags = $futureChangeTags;
-			}
-
-			protected function markAsReverted( int $revisionId, array $extraParams ) {
-				$this->futureChangeTags->addTags(
-					$revisionId,
-					$extraParams
-				);
-			}
-
-			protected function getChangeTags( int $revisionId ) {
-				return $this->futureChangeTags->getTags(
-					$revisionId
-				);
-			}
-		};
+		);
 	}

 	/**
@ -137,36 +99,38 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 	}

 	/**
-	 * Returns a closure that determines the return value of RevertedTagUpdate::markAsReverted()
+	 * Returns a closure that determines the return value of ChangeTagsStore::addTags()
 	 *
 	 * @param int $revertedRevisionId
 	 * @param int $newRevisionId
 	 * @param EditResult $editResult
 	 * @return callable
 	 */
-	private function getFutureChangeTagsReturnCallback(
+	private function getChangeTagsReturnCallback(
 		int $revertedRevisionId,
 		int $newRevisionId,
 		EditResult $editResult
 	): callable {
 		return function (
-			int $revisionId,
-			array $extraParams
+			$tags, $rc_id, $rev_id,
+			$log_id, $params
 		) use ( $newRevisionId, $revertedRevisionId, $editResult ) {
 			$this->assertSame(
-				$revertedRevisionId,
-				$revisionId,
+				[ ChangeTags::TAG_REVERTED ],
+				$tags,
 				'RevertedTagUpdate::markAsReverted() $revisionId'
 			);
-			$this->assertArrayEquals(
-				array_merge(
+			$this->assertSame(
+				$revertedRevisionId,
+				$rev_id,
+				'RevertedTagUpdate::markAsReverted() $revisionId'
+			);
+			$this->assertSame(
+				FormatJson::encode( array_merge(
 					[ 'revertId' => $newRevisionId ],
 					$editResult->jsonSerialize()
-				),
-				$extraParams,
-				false,
-				true,
-				'RevertedTagUpdate::markAsReverted()'
+				) ),
+				$params
 			);
 		};
 	}
@ -186,7 +150,7 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 		int $revertedTagMaxDepth
 	) {
 		$update = $this->newRevertedTagUpdate(
-			$this->createNoOpMock( FutureChangeTags::class ),
+			$this->createMock( ChangeTagsStore::class ),
 			$this->createNoOpMock( RevisionStore::class ),
 			new TestLogger(),
 			$softwareChangeTags,
@ -231,7 +195,7 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 	public function testInvalidEditResult( EditResult $editResult ) {
 		$logger = new TestLogger( true );
 		$update = $this->newRevertedTagUpdate(
-			$this->createNoOpMock( FutureChangeTags::class ),
+			$this->createMock( ChangeTagsStore::class ),
 			$this->createNoOpMock( RevisionStore::class ),
 			$logger,
 			[ 'mw-reverted' ],
@ -272,7 +236,7 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 		);

 		$update = $this->newRevertedTagUpdate(
-			$this->createNoOpMock( FutureChangeTags::class ),
+			$this->createMock( ChangeTagsStore::class ),
 			$revisionStore,
 			$logger,
 			[ 'mw-reverted' ],
@ -317,7 +281,7 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 		);

 		$update = $this->newRevertedTagUpdate(
-			$this->createNoOpMock( FutureChangeTags::class ),
+			$this->createMock( ChangeTagsStore::class ),
 			$revisionStore,
 			$logger,
 			[ 'mw-reverted' ],
@ -383,7 +347,7 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 		);

 		$update = $this->newRevertedTagUpdate(
-			$this->createNoOpMock( FutureChangeTags::class ),
+			$this->createMock( ChangeTagsStore::class ),
 			$revisionStore,
 			$logger,
 			[ 'mw-reverted' ],
@ -425,7 +389,7 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 		);

 		$update = $this->newRevertedTagUpdate(
-			$this->createNoOpMock( FutureChangeTags::class ),
+			$this->createMock( ChangeTagsStore::class ),
 			$revisionStore,
 			$logger,
 			[ 'mw-reverted' ],
@ -455,10 +419,10 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {

 		$logger = new TestLogger( true );

-		$futureChangeTags = $this->createNoOpMock( FutureChangeTags::class, [ 'getTags' ] );
-		$futureChangeTags->expects( $this->once() )
+		$changeTagsStore = $this->createMock( ChangeTagsStore::class );
+		$changeTagsStore->expects( $this->once() )
 			->method( 'getTags' )
-			->with( 300 )
+			->with( $this->anything(), null, 300 )
 			->willReturn( [ 'mw-reverted' ] );

 		$editResult = new EditResult(
@ -473,7 +437,7 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 		);

 		$update = $this->newRevertedTagUpdate(
-			$futureChangeTags,
+			$changeTagsStore,
 			$revisionStore,
 			$logger,
 			[ 'mw-reverted' ],
@ -510,18 +474,18 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 			[ 'mw-rollback' ]
 		);

-		$futureChangeTags = $this->createMock( FutureChangeTags::class );
-		$futureChangeTags->expects( $this->once() )
+		$changeTagsStore = $this->createMock( ChangeTagsStore::class );
+		$changeTagsStore->expects( $this->once() )
 			->method( 'getTags' )
 			->willReturn( [] );
-		$futureChangeTags->expects( $this->once() )
+		$changeTagsStore->expects( $this->once() )
 			->method( 'addTags' )
 			->willReturnCallback(
-				$this->getFutureChangeTagsReturnCallback( 123, 124, $editResult )
+				$this->getChangeTagsReturnCallback( 123, 124, $editResult )
 			);

 		$update = $this->newRevertedTagUpdate(
-			$futureChangeTags,
+			$changeTagsStore,
 			$revisionStore,
 			new TestLogger(),
 			[ 'mw-reverted' ],
@ -556,15 +520,15 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 			[ 'mw-undo' ]
 		);

-		$futureChangeTags = $this->createNoOpMock( FutureChangeTags::class, [ 'getTags' ] );
-		$futureChangeTags->expects( $this->once() )
+		$changeTagsStore = $this->createMock( ChangeTagsStore::class, [ 'getTags' ] );
+		$changeTagsStore->expects( $this->once() )
 			->method( 'getTags' )
 			->willReturn( [] );

 		$logger = new TestLogger( true );

 		$update = $this->newRevertedTagUpdate(
-			$futureChangeTags,
+			$changeTagsStore,
 			$revisionStore,
 			$logger,
 			[ 'mw-reverted' ],
@ -615,28 +579,28 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 			[ 'mw-undo' ]
 		);

-		$futureChangeTags = $this->createMock( FutureChangeTags::class );
+		$changeTagsStore = $this->createMock( ChangeTagsStore::class );

 		// Revision 125 has the same content as 124, so it should not be marked
 		// as reverted. See: T265312
 		$reallyRevertedRevs = [ 123, 124, 126 ];
-		$futureChangeTagsRetCallbacks = [];
+		$changeTagsStoreRetCallbacks = [];
 		for ( $i = 0; $i <= 2; $i++ ) {
-			$futureChangeTagsRetCallbacks[] = new ReturnCallback(
-				$this->getFutureChangeTagsReturnCallback( $reallyRevertedRevs[$i], 130, $editResult )
+			$changeTagsStoreRetCallbacks[] = new ReturnCallback(
+				$this->getChangeTagsReturnCallback( $reallyRevertedRevs[$i], 130, $editResult )
 			);
 		}
-		$futureChangeTags
+		$changeTagsStore
 			->method( 'addTags' )
-			->willReturnOnConsecutiveCalls( ...$futureChangeTagsRetCallbacks );
-		$futureChangeTags->expects( $this->exactly( 3 ) )
+			->willReturnOnConsecutiveCalls( ...$changeTagsStoreRetCallbacks );
+		$changeTagsStore->expects( $this->exactly( 3 ) )
 			->method( 'addTags' );
-		$futureChangeTags->expects( $this->once() )
+		$changeTagsStore->expects( $this->once() )
 			->method( 'getTags' )
 			->willReturn( [] );

 		$update = $this->newRevertedTagUpdate(
-			$futureChangeTags,
+			$changeTagsStore,
 			$revisionStore,
 			new TestLogger(),
 			[ 'mw-reverted' ],
@ -647,12 +611,3 @@ class RevertedTagUpdateTest extends MediaWikiUnitTestCase {
 		$update->doUpdate();
 	}
 }
-
-// phpcs:ignore Generic.Files.OneObjectStructurePerFile.MultipleFound
-class FutureChangeTags {
-	public function addTags( ...$args ) {
-	}
-
-	public function getTags() {
-	}
-}