API: Refactor to match modern code standards

I've moved all API classes into a separate folder,
as I felt like grouping modules improves readability.
APIs themselves had storage logic which I extracted and put
in another folder. I was originally going with an interface
to the storage to allow for other storage methods than
log entries, but the code was too tightly coupled with it,
so I've left that for another day. Added dependency injection
for all services and used ServiceOptions for config vars.

Bug: T337002
Change-Id: Ie8a1f435d635e1d0e1286f673bfe96cc4fdfe4fe

ServiceWiring.php

@@ -1,5 +1,7 @@
 <?php
 
+use MediaWiki\Config\ServiceOptions;
+use MediaWiki\Extension\Thanks\Storage\LogStore;
 use MediaWiki\Extension\Thanks\ThanksQueryHelper;
 use MediaWiki\MediaWikiServices;
 
@@ -12,4 +14,14 @@ return [
 				$services->getDBLoadBalancer()
 			);
 	},
+	'LogStore' => static function ( MediaWikiServices $services ): LogStore {
+		return new LogStore(
+			$services->getDBLoadBalancerFactory(),
+			$services->getActorNormalization(),
+			new ServiceOptions(
+				LogStore::CONSTRUCTOR_OPTIONS,
+				$services->getMainConfig()
+			)
+		);
+	}
 ];

extension.json

@@ -41,7 +41,15 @@
 		"thanks/*": "MediaWiki\\Extension\\Thanks\\ThanksLogFormatter"
 	},
 	"APIModules": {
-		"thank": "MediaWiki\\Extension\\Thanks\\ApiCoreThank"
+		"thank": {
+			"class": "MediaWiki\\Extension\\Thanks\\Api\\ApiCoreThank",
+			"services": [
+				"PermissionManager",
+				"RevisionStore",
+				"UserFactory",
+				"LogStore"
+			]
+		}
 	},
 	"MessagesDirs": {
 		"Thanks": [

includes/Api/ApiCoreThank.php

@@ -1,14 +1,20 @@
 <?php
 
-namespace MediaWiki\Extension\Thanks;
+namespace MediaWiki\Extension\Thanks\Api;
 
 use ApiBase;
+use ApiMain;
 use DatabaseLogEntry;
 use EchoDiscussionParser;
 use EchoEvent;
 use LogEntry;
-use MediaWiki\MediaWikiServices;
+use MediaWiki\Extension\Thanks\Storage\Exceptions\InvalidLogType;
+use MediaWiki\Extension\Thanks\Storage\Exceptions\LogDeleted;
+use MediaWiki\Extension\Thanks\Storage\LogStore;
+use MediaWiki\Permissions\PermissionManager;
 use MediaWiki\Revision\RevisionRecord;
+use MediaWiki\Revision\RevisionStore;
+use MediaWiki\User\UserFactory;
 use MediaWiki\User\UserIdentity;
 use Title;
 use User;
@@ -22,6 +28,26 @@ use Wikimedia\ParamValidator\TypeDef\IntegerDef;
  * @ingroup Extensions
  */
 class ApiCoreThank extends ApiThank {
+	protected RevisionStore $revisionStore;
+	protected UserFactory $userFactory;
+
+	public function __construct(
+		ApiMain $main,
+		$action,
+		PermissionManager $permissionManager,
+		RevisionStore $revisionStore,
+		UserFactory $userFactory,
+		LogStore $storage
+	) {
+		parent::__construct(
+			$main,
+			$action,
+			$permissionManager,
+			$storage
+		);
+		$this->revisionStore = $revisionStore;
+		$this->userFactory = $userFactory;
+	}
 
 	/**
 	 * Perform the API request.
@@ -75,8 +101,7 @@ class ApiCoreThank extends ApiThank {
 			$recipientUsername = $revision->getUser()->getName();
 
 			// If there is no parent revid of this revision, it's a page creation.
-			$store = MediaWikiServices::getInstance()->getRevisionStore();
-			if ( !(bool)$store->getPreviousRevision( $revision ) ) {
+			if ( !$this->revisionStore->getPreviousRevision( $revision ) ) {
 				$revcreation = true;
 			}
 		}
@@ -115,8 +140,7 @@ class ApiCoreThank extends ApiThank {
 	}
 
 	private function getRevisionFromId( $revId ) {
-		$store = MediaWikiServices::getInstance()->getRevisionStore();
-		$revision = $store->getRevisionById( $revId );
+		$revision = $this->revisionStore->getRevisionById( $revId );
 		// Revision ID 1 means an invalid argument was passed in.
 		if ( !$revision || $revision->getId() === 1 ) {
 			$this->dieWithError( 'thanks-error-invalidrevision', 'invalidrevision' );
@@ -131,26 +155,20 @@ class ApiCoreThank extends ApiThank {
 	 * @param int $logId The log entry ID.
 	 * @return DatabaseLogEntry
 	 */
-	protected function getLogEntryFromId( $logId ) {
-		$logEntry = DatabaseLogEntry::newFromId( $logId, wfGetDB( DB_REPLICA ) );
+	protected function getLogEntryFromId( $logId ): DatabaseLogEntry {
+		$logEntry = null;
+		try {
+			$logEntry = $this->storage->getLogEntryFromId( $logId );
+		} catch ( InvalidLogType $e ) {
+			$err = $this->msg( 'thanks-error-invalid-log-type', $e->getLogType() );
+			$this->dieWithError( $err, 'thanks-error-invalid-log-type' );
+		} catch ( LogDeleted $e ) {
+			$this->dieWithError( 'thanks-error-log-deleted', 'thanks-error-log-deleted' );
+		}
 
 		if ( !$logEntry ) {
 			$this->dieWithError( 'thanks-error-invalid-log-id', 'thanks-error-invalid-log-id' );
 		}
-
-		// Make sure this log type is allowed.
-		$allowedLogTypes = $this->getConfig()->get( 'ThanksAllowedLogTypes' );
-		if ( !in_array( $logEntry->getType(), $allowedLogTypes )
-			&& !in_array( $logEntry->getType() . '/' . $logEntry->getSubtype(), $allowedLogTypes ) ) {
-			$err = $this->msg( 'thanks-error-invalid-log-type', $logEntry->getType() );
-			$this->dieWithError( $err, 'thanks-error-invalid-log-type' );
-		}
-
-		// Don't permit thanks if any part of the log entry is deleted.
-		if ( $logEntry->getDeleted() ) {
-			$this->dieWithError( 'thanks-error-log-deleted', 'thanks-error-log-deleted' );
-		}
-
 		// @phan-suppress-next-line PhanTypeMismatchReturnNullable T240141
 		return $logEntry;
 	}
@@ -185,7 +203,7 @@ class ApiCoreThank extends ApiThank {
 		if ( !$recipient ) {
 			$this->dieWithError( 'thanks-error-invalidrecipient', 'invalidrecipient' );
 		}
-		return User::newFromId( $recipient );
+		return $this->userFactory->newFromId( $recipient );
 	}
 
 	/**
@@ -194,7 +212,7 @@ class ApiCoreThank extends ApiThank {
 	 */
 	private function getUserFromLog( LogEntry $logEntry ) {
 		$recipient = $logEntry->getPerformerIdentity();
-		return MediaWikiServices::getInstance()->getUserFactory()->newFromUserIdentity( $recipient );
+		return $this->userFactory->newFromUserIdentity( $recipient );
 	}
 
 	/**

includes/Api/ApiFlowThank.php

@@ -1,6 +1,6 @@
 <?php
 
-namespace MediaWiki\Extension\Thanks;
+namespace MediaWiki\Extension\Thanks\Api;
 
 use ApiBase;
 use EchoEvent;
@@ -25,6 +25,7 @@ use Wikimedia\ParamValidator\ParamValidator;
  */
 
 class ApiFlowThank extends ApiThank {
+
 	public function execute() {
 		$user = $this->getUser();
 		$this->dieOnBadUser( $user );

includes/Api/ApiThank.php

@@ -1,12 +1,11 @@
 <?php
 
-namespace MediaWiki\Extension\Thanks;
+namespace MediaWiki\Extension\Thanks\Api;
 
 use ApiBase;
-use ExtensionRegistry;
-use ManualLogEntry;
-use MediaWiki\CheckUser\Hooks;
-use MediaWiki\MediaWikiServices;
+use ApiMain;
+use MediaWiki\Extension\Thanks\Storage\LogStore;
+use MediaWiki\Permissions\PermissionManager;
 use Title;
 use User;
 
@@ -17,6 +16,21 @@ use User;
  * @ingroup Extensions
  */
 abstract class ApiThank extends ApiBase {
+
+	protected PermissionManager $permissionManager;
+	protected LogStore $storage;
+
+	public function __construct(
+		ApiMain $main,
+		$action,
+		PermissionManager $permissionManager,
+		LogStore $storage
+	) {
+		parent::__construct( $main, $action );
+		$this->permissionManager = $permissionManager;
+		$this->storage = $storage;
+	}
+
 	protected function dieOnBadUser( User $user ) {
 		if ( $user->isAnon() ) {
 			$this->dieWithError( 'thanks-error-notloggedin', 'notloggedin' );
@@ -36,8 +50,7 @@ abstract class ApiThank extends ApiBase {
 	 * @param Title $title
 	 */
 	protected function dieOnUserBlockedFromTitle( User $user, Title $title ) {
-		$permissionManager = MediaWikiServices::getInstance()->getPermissionManager();
-		if ( $permissionManager->isBlockedFrom( $user, $title ) ) {
+		if ( $this->permissionManager->isBlockedFrom( $user, $title ) ) {
 			// Block should definitely exist
 			// @phan-suppress-next-line PhanTypeMismatchArgumentNullable
 			$this->dieBlocked( $user->getBlock() );
@@ -77,53 +90,12 @@ abstract class ApiThank extends ApiBase {
 		] );
 	}
 
-	/**
-	 * This checks the log_search data.
-	 *
-	 * @param User $thanker The user sending the thanks.
-	 * @param string $uniqueId The identifier for the thanks.
-	 * @return bool Whether thanks has already been sent
-	 */
 	protected function haveAlreadyThanked( User $thanker, $uniqueId ) {
-		$dbw = wfGetDB( DB_PRIMARY );
-		$thankerActor = MediaWikiServices::getInstance()->getActorNormalization()
-			->acquireActorId( $thanker, $dbw );
-		return (bool)$dbw->selectRow(
-			[ 'log_search', 'logging' ],
-			[ 'ls_value' ],
-			[
-				'log_actor' => $thankerActor,
-				'ls_field' => 'thankid',
-				'ls_value' => $uniqueId,
-			],
-			__METHOD__,
-			[],
-			[ 'logging' => [ 'INNER JOIN', 'ls_log_id=log_id' ] ]
-		);
+		return $this->storage->haveThanked( $thanker, $uniqueId );
 	}
 
-	/**
-	 * @param User $user The user performing the thanks (and the log entry).
-	 * @param User $recipient The target of the thanks (and the log entry).
-	 * @param string $uniqueId A unique Id to identify the event being thanked for, to use
-	 *                         when checking for duplicate thanks
-	 */
 	protected function logThanks( User $user, User $recipient, $uniqueId ) {
-		if ( !$this->getConfig()->get( 'ThanksLogging' ) ) {
-			return;
-		}
-		$logEntry = new ManualLogEntry( 'thanks', 'thank' );
-		$logEntry->setPerformer( $user );
-		$logEntry->setRelations( [ 'thankid' => $uniqueId ] );
-		$target = $recipient->getUserPage();
-		$logEntry->setTarget( $target );
-		$logId = $logEntry->insert();
-		$logEntry->publish( $logId, 'udp' );
-
-		if ( ExtensionRegistry::getInstance()->isLoaded( 'CheckUser' ) ) {
-			$recentChange = $logEntry->getRecentChange();
-			Hooks::updateCheckUserData( $recentChange );
-		}
+		$this->storage->thank( $user, $recipient, $uniqueId );
 	}
 
 	public function needsToken() {

includes/Hooks.php

@@ -16,6 +16,7 @@ use Html;
 use ImagePage;
 use LogEventsList;
 use LogPage;
+use MediaWiki\Extension\Thanks\Api\ApiFlowThank;
 use MediaWiki\Linker\LinkTarget;
 use MediaWiki\MediaWikiServices;
 use MediaWiki\Revision\RevisionRecord;
@@ -405,7 +406,13 @@ class Hooks {
 			$moduleManager->addModule(
 				'flowthank',
 				'action',
-				ApiFlowThank::class
+				[
+					"class" => ApiFlowThank::class,
+					"services" => [
+						"PermissionManager",
+						"LogStore"
+					]
+				]
 			);
 		}
 	}

includes/Storage/Exceptions/InvalidLogType.php

@@ -0,0 +1,18 @@
+<?php
+
+namespace MediaWiki\Extension\Thanks\Storage\Exceptions;
+
+use Exception;
+
+class InvalidLogType extends Exception {
+	private string $logType;
+
+	public function __construct( string $logType ) {
+		parent::__construct();
+		$this->logType = $logType;
+	}
+
+	public function getLogType(): string {
+		return $this->logType;
+	}
+}

includes/Storage/Exceptions/LogDeleted.php

@@ -0,0 +1,8 @@
+<?php
+
+namespace MediaWiki\Extension\Thanks\Storage\Exceptions;
+
+use Exception;
+
+class LogDeleted extends Exception {
+}

includes/Storage/LogStore.php

@@ -0,0 +1,114 @@
+<?php
+
+namespace MediaWiki\Extension\Thanks\Storage;
+
+use DatabaseLogEntry;
+use ExtensionRegistry;
+use ManualLogEntry;
+use MediaWiki\CheckUser\Hooks;
+use MediaWiki\Config\ServiceOptions;
+use MediaWiki\Extension\Thanks\Storage\Exceptions\InvalidLogType;
+use MediaWiki\Extension\Thanks\Storage\Exceptions\LogDeleted;
+use MediaWiki\User\ActorNormalization;
+use User;
+use Wikimedia\Rdbms\IConnectionProvider;
+
+/**
+ * Manages the storage for Thank events.
+ * Thanks are stored as logs with relations between users.
+ * Each thank action should have an unique ID generated by callers.
+ */
+class LogStore {
+
+	protected IConnectionProvider $conn;
+	protected ActorNormalization $actorNormalization;
+	public const CONSTRUCTOR_OPTIONS = [ 'ThanksLogging', 'ThanksAllowedLogTypes' ];
+	protected ServiceOptions $serviceOptions;
+
+	public function __construct(
+		IConnectionProvider $conn,
+		ActorNormalization $actorNormalization,
+		ServiceOptions $serviceOptions
+	) {
+		$this->conn = $conn;
+		$this->actorNormalization = $actorNormalization;
+		$serviceOptions->assertRequiredOptions( self::CONSTRUCTOR_OPTIONS );
+		$this->serviceOptions = $serviceOptions;
+	}
+
+	/**
+	 * @param User $user The user performing the thanks (and the log entry).
+	 * @param User $recipient The target of the thanks (and the log entry).
+	 * @param string $uniqueId A unique Id to identify the event being thanked for, to use
+	 *                         when checking for duplicate thanks
+	 */
+	public function thank( User $user, User $recipient, string $uniqueId ): void {
+		if ( !$this->serviceOptions->get( 'ThanksLogging' ) ) {
+			return;
+		}
+		$logEntry = new ManualLogEntry( 'thanks', 'thank' );
+		$logEntry->setPerformer( $user );
+		$logEntry->setRelations( [ 'thankid' => $uniqueId ] );
+		$target = $recipient->getUserPage();
+		$logEntry->setTarget( $target );
+		$logId = $logEntry->insert();
+		$logEntry->publish( $logId, 'udp' );
+
+		if ( ExtensionRegistry::getInstance()->isLoaded( 'CheckUser' ) ) {
+			$recentChange = $logEntry->getRecentChange();
+			// TODO: This should be done in a separate hook handler
+			Hooks::updateCheckUserData( $recentChange );
+		}
+	}
+
+	/**
+	 * This checks the log_search data.
+	 *
+	 * @param User $thanker The user sending the thanks.
+	 * @param string $uniqueId The identifier for the thanks.
+	 * @return bool Whether thanks has already been sent
+	 */
+	public function haveThanked( User $thanker, string $uniqueId ): bool {
+		// TODO: Figure out why it's not getting the data from a replica
+		$dbw = $this->conn->getPrimaryDatabase();
+		$thankerActor = $this->actorNormalization->acquireActorId( $thanker, $dbw );
+		return (bool)$dbw->newSelectQueryBuilder()
+			->select( 'ls_value' )
+			->from( 'log_search' )
+			->join( 'logging', null, [ 'ls_log_id=log_id' ] )
+			->where(
+				[
+					'log_actor' => $thankerActor,
+					'ls_field' => 'thankid',
+					'ls_value' => $uniqueId,
+				]
+			)
+			->caller( __METHOD__ )
+			->fetchRow();
+	}
+
+	/**
+	 * @throws InvalidLogType
+	 * @throws LogDeleted
+	 */
+	public function getLogEntryFromId( int $logId ): ?DatabaseLogEntry {
+		$logEntry = DatabaseLogEntry::newFromId( $logId, $this->conn->getPrimaryDatabase() );
+
+		if ( !$logEntry ) {
+			return null;
+		}
+
+		// Make sure this log type is allowed.
+		$allowedLogTypes = $this->serviceOptions->get( 'ThanksAllowedLogTypes' );
+		if ( !in_array( $logEntry->getType(), $allowedLogTypes )
+			 && !in_array( $logEntry->getType() . '/' . $logEntry->getSubtype(), $allowedLogTypes ) ) {
+			throw new InvalidLogType( $logEntry->getType() );
+		}
+
+		// Don't permit thanks if any part of the log entry is deleted.
+		if ( $logEntry->getDeleted() ) {
+			throw new LogDeleted();
+		}
+		return $logEntry;
+	}
+}

tests/phpunit/ApiCoreThankIntegrationTest.php

@@ -6,7 +6,7 @@ use MediaWiki\Revision\SlotRecord;
 /**
  * Integration tests for the Thanks API module
  *
- * @covers \MediaWiki\Extension\Thanks\ApiCoreThank
+ * @covers \MediaWiki\Extension\Thanks\Api\ApiCoreThank
  *
  * @group Thanks
  * @group Database

tests/phpunit/ApiCoreThankUnitTest.php

@@ -1,7 +1,7 @@
 <?php
 
 use MediaWiki\Block\DatabaseBlock;
-use MediaWiki\Extension\Thanks\ApiCoreThank;
+use MediaWiki\Extension\Thanks\Api\ApiCoreThank;
 use MediaWiki\User\UserIdentityValue;
 
 /**
@@ -15,7 +15,15 @@ use MediaWiki\User\UserIdentityValue;
 class ApiCoreThankUnitTest extends ApiTestCase {
 
 	protected function getModule() {
-		return new ApiCoreThank( new ApiMain(), 'thank' );
+		$services = $this->getServiceContainer();
+		return new ApiCoreThank(
+			new ApiMain(),
+			'thank',
+			$services->getPermissionManager(),
+			$services->getRevisionStore(),
+			$services->getUserFactory(),
+			$services->getService( 'LogStore' )
+		);
 	}
 
 	private function createBlock( $options ) {
@@ -31,8 +39,8 @@ class ApiCoreThankUnitTest extends ApiTestCase {
 
 	/**
 	 * @dataProvider provideDieOnBadUser
-	 * @covers \MediaWiki\Extension\Thanks\ApiThank::dieOnBadUser
-	 * @covers \MediaWiki\Extension\Thanks\ApiThank::dieOnUserBlockedFromThanks
+	 * @covers \MediaWiki\Extension\Thanks\Api\ApiThank::dieOnBadUser
+	 * @covers \MediaWiki\Extension\Thanks\Api\ApiThank::dieOnUserBlockedFromThanks
 	 */
 	public function testDieOnBadUser( $user, $dieMethod, $expectedError ) {
 		$module = $this->getModule();

tests/phpunit/ApiFlowThankIntegrationTest.php

@@ -8,7 +8,7 @@ use Flow\Model\Workflow;
 /**
  * Integration tests for the Thanks Flow api module
  *
- * @covers \MediaWiki\Extension\Thanks\ApiFlowThank
+ * @covers \MediaWiki\Extension\Thanks\Api\ApiFlowThank
  *
  * @group Thanks
  * @group Database