diff --git a/ServiceWiring.php b/ServiceWiring.php index 232edb30..eb5c8f20 100644 --- a/ServiceWiring.php +++ b/ServiceWiring.php @@ -1,5 +1,7 @@ getDBLoadBalancer() ); }, + 'LogStore' => static function ( MediaWikiServices $services ): LogStore { + return new LogStore( + $services->getDBLoadBalancerFactory(), + $services->getActorNormalization(), + new ServiceOptions( + LogStore::CONSTRUCTOR_OPTIONS, + $services->getMainConfig() + ) + ); + } ]; diff --git a/extension.json b/extension.json index e43e01ee..db6bd619 100644 --- a/extension.json +++ b/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": [ diff --git a/includes/ApiCoreThank.php b/includes/Api/ApiCoreThank.php similarity index 85% rename from includes/ApiCoreThank.php rename to includes/Api/ApiCoreThank.php index 3727475d..4a09b70f 100644 --- a/includes/ApiCoreThank.php +++ b/includes/Api/ApiCoreThank.php @@ -1,14 +1,20 @@ 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 ); } /** diff --git a/includes/ApiFlowThank.php b/includes/Api/ApiFlowThank.php similarity index 99% rename from includes/ApiFlowThank.php rename to includes/Api/ApiFlowThank.php index d7db60a3..4c7fed97 100644 --- a/includes/ApiFlowThank.php +++ b/includes/Api/ApiFlowThank.php @@ -1,6 +1,6 @@ getUser(); $this->dieOnBadUser( $user ); diff --git a/includes/ApiThank.php b/includes/Api/ApiThank.php similarity index 57% rename from includes/ApiThank.php rename to includes/Api/ApiThank.php index 34f749d0..0498e488 100644 --- a/includes/ApiThank.php +++ b/includes/Api/ApiThank.php @@ -1,12 +1,11 @@ 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() { diff --git a/includes/Hooks.php b/includes/Hooks.php index 3389630b..fe78e0c1 100644 --- a/includes/Hooks.php +++ b/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" + ] + ] ); } } diff --git a/includes/Storage/Exceptions/InvalidLogType.php b/includes/Storage/Exceptions/InvalidLogType.php new file mode 100644 index 00000000..e2c3c108 --- /dev/null +++ b/includes/Storage/Exceptions/InvalidLogType.php @@ -0,0 +1,18 @@ +logType = $logType; + } + + public function getLogType(): string { + return $this->logType; + } +} diff --git a/includes/Storage/Exceptions/LogDeleted.php b/includes/Storage/Exceptions/LogDeleted.php new file mode 100644 index 00000000..16b0a2c9 --- /dev/null +++ b/includes/Storage/Exceptions/LogDeleted.php @@ -0,0 +1,8 @@ +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; + } +} diff --git a/tests/phpunit/ApiCoreThankIntegrationTest.php b/tests/phpunit/ApiCoreThankIntegrationTest.php index d6d22327..a6aad826 100644 --- a/tests/phpunit/ApiCoreThankIntegrationTest.php +++ b/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 diff --git a/tests/phpunit/ApiCoreThankUnitTest.php b/tests/phpunit/ApiCoreThankUnitTest.php index d7eda133..4285247f 100644 --- a/tests/phpunit/ApiCoreThankUnitTest.php +++ b/tests/phpunit/ApiCoreThankUnitTest.php @@ -1,7 +1,7 @@ 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(); diff --git a/tests/phpunit/ApiFlowThankIntegrationTest.php b/tests/phpunit/ApiFlowThankIntegrationTest.php index 37132a28..d66cf832 100644 --- a/tests/phpunit/ApiFlowThankIntegrationTest.php +++ b/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