diff --git a/includes/Consequences/Consequence/Block.php b/includes/Consequences/Consequence/Block.php index 0c84c9a20..188d88cce 100644 --- a/includes/Consequences/Consequence/Block.php +++ b/includes/Consequences/Consequence/Block.php @@ -4,7 +4,6 @@ namespace MediaWiki\Extension\AbuseFilter\Consequences\Consequence; use ManualLogEntry; use MediaWiki\Block\BlockUserFactory; -use MediaWiki\Block\DatabaseBlock; use MediaWiki\Block\DatabaseBlockStore; use MediaWiki\Extension\AbuseFilter\Consequences\Parameters; use MediaWiki\Extension\AbuseFilter\FilterUser; @@ -21,6 +20,8 @@ class Block extends BlockingConsequence implements ReversibleConsequence { private $preventsTalkEdit; /** @var DatabaseBlockStore */ private $databaseBlockStore; + /** @var callable */ + private $blockFactory; /** * @param Parameters $params @@ -28,6 +29,7 @@ class Block extends BlockingConsequence implements ReversibleConsequence { * @param bool $preventTalkEdit * @param BlockUserFactory $blockUserFactory * @param DatabaseBlockStore $databaseBlockStore + * @param callable $blockFactory Should take a user name and return a DatabaseBlock or null. * @param FilterUser $filterUser * @param MessageLocalizer $messageLocalizer */ @@ -37,12 +39,14 @@ class Block extends BlockingConsequence implements ReversibleConsequence { bool $preventTalkEdit, BlockUserFactory $blockUserFactory, DatabaseBlockStore $databaseBlockStore, + callable $blockFactory, FilterUser $filterUser, MessageLocalizer $messageLocalizer ) { parent::__construct( $params, $expiry, $blockUserFactory, $filterUser, $messageLocalizer ); $this->databaseBlockStore = $databaseBlockStore; $this->preventsTalkEdit = $preventTalkEdit; + $this->blockFactory = $blockFactory; } /** @@ -67,8 +71,8 @@ class Block extends BlockingConsequence implements ReversibleConsequence { * @todo This could use UnblockUser, but we need to check if the block was performed by the AF user */ public function revert( $info, UserIdentity $performer, string $reason ): bool { - // TODO: DI once T255433 is resolved - $block = DatabaseBlock::newFromTarget( $this->parameters->getUser()->getName() ); + // TODO: Proper DI once T255433 is resolved + $block = ( $this->blockFactory )( $this->parameters->getUser()->getName() ); if ( !( $block && $block->getBy() === $this->filterUser->getUser()->getId() ) ) { // Not blocked by abuse filter return false; @@ -80,12 +84,11 @@ class Block extends BlockingConsequence implements ReversibleConsequence { $logEntry->setTarget( new TitleValue( NS_USER, $this->parameters->getUser()->getName() ) ); $logEntry->setComment( $reason ); $logEntry->setPerformer( $performer ); - $id = $logEntry->insert(); if ( !defined( 'MW_PHPUNIT_TEST' ) ) { // This has a bazillion of static dependencies all around the place, and a nightmare to deal with in tests // TODO: Remove this check once T253717 is resolved // @codeCoverageIgnoreStart - $logEntry->publish( $id ); + $logEntry->publish( $logEntry->insert() ); // @codeCoverageIgnoreEnd } return true; diff --git a/includes/Consequences/ConsequencesFactory.php b/includes/Consequences/ConsequencesFactory.php index b1df7679b..5c07c8788 100644 --- a/includes/Consequences/ConsequencesFactory.php +++ b/includes/Consequences/ConsequencesFactory.php @@ -4,6 +4,7 @@ namespace MediaWiki\Extension\AbuseFilter\Consequences; use BagOStuff; use MediaWiki\Block\BlockUserFactory; +use MediaWiki\Block\DatabaseBlock; use MediaWiki\Block\DatabaseBlockStore; use MediaWiki\Config\ServiceOptions; use MediaWiki\Extension\AbuseFilter\BlockAutopromoteStore; @@ -143,6 +144,8 @@ class ConsequencesFactory { $preventsTalk, $this->blockUserFactory, $this->databaseBlockStore, + // FIXME This is a hack until DI is possible here. + [ DatabaseBlock::class, 'newFromTarget' ], $this->filterUser, $this->messageLocalizer ); diff --git a/tests/phpunit/AbuseFilterBlockTest.php b/tests/phpunit/AbuseFilterBlockTest.php index 284e3ce74..657aa6756 100644 --- a/tests/phpunit/AbuseFilterBlockTest.php +++ b/tests/phpunit/AbuseFilterBlockTest.php @@ -2,14 +2,13 @@ use MediaWiki\Block\BlockUser; use MediaWiki\Block\BlockUserFactory; +use MediaWiki\Block\DatabaseBlock; use MediaWiki\Block\DatabaseBlockStore; use MediaWiki\Extension\AbuseFilter\Consequences\Consequence\Block; use MediaWiki\Extension\AbuseFilter\Consequences\Parameters; use MediaWiki\Extension\AbuseFilter\FilterUser; use MediaWiki\User\UserIdentity; use MediaWiki\User\UserIdentityValue; -use Wikimedia\Rdbms\IDatabase; -use Wikimedia\Rdbms\ILoadBalancer; /** * @coversDefaultClass \MediaWiki\Extension\AbuseFilter\Consequences\Consequence\Block @@ -77,6 +76,9 @@ class AbuseFilterBlockTest extends MediaWikiIntegrationTestCase { $preventsTalkEdit = true, $blockUserFactory, $this->createMock( DatabaseBlockStore::class ), + function () { + return null; + }, $this->getFilterUser(), $this->getMsgLocalizer() ); @@ -94,6 +96,9 @@ class AbuseFilterBlockTest extends MediaWikiIntegrationTestCase { false, $this->createMock( BlockUserFactory::class ), $this->createMock( DatabaseBlockStore::class ), + function () { + return null; + }, $this->createMock( FilterUser::class ), $this->getMsgLocalizer() ); @@ -103,70 +108,24 @@ class AbuseFilterBlockTest extends MediaWikiIntegrationTestCase { public function provideRevert() { yield 'no block to revert' => [ null, null, false ]; - $getMockRow = function ( UserIdentity $performer ) { - return new class( $performer ) extends stdClass { - private $performer; - - public function __construct( UserIdentity $performer ) { - $this->performer = $performer; - } - - public function __get( string $prop ) { - switch ( $prop ) { - case 'ipb_by': - return $this->performer->getId(); - case 'ipb_by_text': - return $this->performer->getName(); - case 'ipb_by_actor': - return $this->performer->getActorId(); - case 'ipb_auto': - return false; - default: - return 'foo'; - } - } - }; - }; - $randomUser = new UserIdentityValue( 1234, 'Some other user', 3456 ); - yield 'not blocked by AF user' => [ $getMockRow( $randomUser ), null, false ]; + yield 'not blocked by AF user' => [ new DatabaseBlock( [ 'by' => $randomUser->getUserId() ] ), null, false ]; - $filterUserIdentity = $this->getFilterUser()->getUser(); + $blockByFilter = new DatabaseBlock( [ 'by' => $this->getFilterUser()->getUser()->getUserId() ] ); $failBlockStore = $this->createMock( DatabaseBlockStore::class ); $failBlockStore->expects( $this->once() )->method( 'deleteBlock' )->willReturn( false ); - yield 'cannot delete block' => [ $getMockRow( $filterUserIdentity ), $failBlockStore, false ]; + yield 'cannot delete block' => [ $blockByFilter, $failBlockStore, false ]; $succeedBlockStore = $this->createMock( DatabaseBlockStore::class ); $succeedBlockStore->expects( $this->once() )->method( 'deleteBlock' )->willReturn( true ); - yield 'succeed' => [ $getMockRow( $filterUserIdentity ), $succeedBlockStore, true ]; + yield 'succeed' => [ $blockByFilter, $succeedBlockStore, true ]; } /** * @covers ::revert * @dataProvider provideRevert - * @todo This sucks. Clean it up once T255433 and T253717 are resolved */ - public function testRevert( ?stdClass $blockRow, ?DatabaseBlockStore $blockStore, bool $expected ) { - // Unset all hook handlers per T272124 - $this->setService( 'HookContainer', $this->createHookContainer() ); - $db = $this->createMock( IDatabase::class ); - $db->method( 'select' )->willReturnCallback( function ( $tables ) use ( $blockRow ) { - return in_array( 'ipblocks', $tables, true ) && $blockRow ? [ $blockRow ] : []; - } ); - $lb = $this->createMock( ILoadBalancer::class ); - $lb->method( 'getMaintenanceConnectionRef' )->willReturn( $db ); - $this->setService( 'DBLoadBalancer', $lb ); - $commentStore = $this->createMock( CommentStore::class ); - $migrationData = [ 'tables' => [], 'fields' => [], 'joins' => [] ]; - $commentStore->method( 'getJoin' )->willReturn( $migrationData ); - $commentStore->method( 'insert' )->willReturn( [] ); - $this->setService( 'CommentStore', $commentStore ); - $actorMigration = $this->createMock( ActorMigration::class ); - $actorMigration->method( 'getInsertValues' )->willReturn( [] ); - $actorMigration->method( 'getJoin' )->willReturn( $migrationData ); - $this->setService( 'ActorMigration', $actorMigration ); - $this->setService( 'LinkCache', $this->createMock( LinkCache::class ) ); - + public function testRevert( ?DatabaseBlock $block, ?DatabaseBlockStore $blockStore, bool $expected ) { $params = $this->createMock( Parameters::class ); $params->method( 'getUser' )->willReturn( new UserIdentityValue( 1, 'Foobar', 2 ) ); $block = new Block( @@ -175,6 +134,9 @@ class AbuseFilterBlockTest extends MediaWikiIntegrationTestCase { false, $this->createMock( BlockUserFactory::class ), $blockStore ?? $this->createMock( DatabaseBlockStore::class ), + function () use ( $block ) { + return $block; + }, $this->getFilterUser(), $this->getMsgLocalizer() );