ManualLogEntry: Check RecentChanges bot flag before POSTSEND

Why:
* ManualLogEntry::publish causes an entry to be added to the
  recentchanges table as well as other publishing actions for
  the log entry.
* This is done in a DeferredUpdate to avoid writes that block
  the response to the user unnecessarily.
* However, doing this means that if we assign a temporary user
  right during the course of the POST request (such as in the
  case of an extension wanting to make lots of log entries)
  this is not seen in the RecentChanges object created at the
  POSTSEND stage.
* Creating the RecentChanges entry at the time of calling ::publish
  allows temporary user rights, such as 'bot', to take effect
  and mark the log entries as bot actions.
** We still put the writes to the DB POSTSEND as we only need to
   create the RecentChanges object to get the bot flag set
   correctly.

What:
* Update ManualLogEntry::publish to create the RecentChanges
  object outside the POSTSEND DeferredUpdate.
* Add tests for ManualLogEntry which was previously completely
  untested (though likely indirectly tested through code which
  uses this class).
** Adding the tests allows verification that the change performed
   actually does what we want it to do.

Bug: T387659
Change-Id: I80d906bbc1cad180a815477477d9081f9775a197

2 files changed, 117 insertions, 13 deletions

diff --git a/includes/logging/ManualLogEntry.php b/includes/logging/ManualLogEntry.php
index 46a29a9133c0..d2ea485c1072 100644
--- a/includes/logging/ManualLogEntry.php
+++ b/includes/logging/ManualLogEntry.php
@@ -438,14 +438,17 @@ class ManualLogEntry extends LogEntryBase implements Taggable {
 			$canAddTags = false;
 		}
 
-		DeferredUpdates::addCallableUpdate(
-			function () use ( $newId, $to, $canAddTags ) {
-				$log = new LogPage( $this->getType() );
-				if ( !$log->isRestricted() ) {
-					( new HookRunner( MediaWikiServices::getInstance()->getHookContainer() ) )
-						->onManualLogEntryBeforePublish( $this );
-					$rc = $this->getRecentChange( $newId );
-
+		$log = new LogPage( $this->getType() );
+		if ( !$log->isRestricted() ) {
+			// We need to generate a RecentChanges object now so that we can have the rc_bot attribute set based
+			// on any temporary user rights assigned to the user as part of the creation of this log entry.
+			// We do not attempt to save it to the DB until POSTSEND to avoid writes blocking a response (T127852).
+			( new HookRunner( MediaWikiServices::getInstance()->getHookContainer() ) )
+				->onManualLogEntryBeforePublish( $this );
+			$rc = $this->getRecentChange( $newId );
+
+			DeferredUpdates::addCallableUpdate(
+				function () use ( $newId, $to, $canAddTags, $rc ) {
 					if ( $to === 'rc' || $to === 'rcandudp' ) {
 						// save RC, passing tags so they are applied there
 						$rc->addTags( $this->getTags() );
@@ -466,11 +469,11 @@ class ManualLogEntry extends LogEntryBase implements Taggable {
 					if ( $to === 'udp' || $to === 'rcandudp' ) {
 						$rc->notifyRCFeeds();
 					}
-				}
-			},
-			DeferredUpdates::POSTSEND,
-			MediaWikiServices::getInstance()->getConnectionProvider()->getPrimaryDatabase()
-		);
+				},
+				DeferredUpdates::POSTSEND,
+				MediaWikiServices::getInstance()->getConnectionProvider()->getPrimaryDatabase()
+			);
+		}
 	}
 
 	/**
diff --git a/tests/phpunit/integration/includes/logging/ManualLogEntryTest.php b/tests/phpunit/integration/includes/logging/ManualLogEntryTest.php
new file mode 100644
index 000000000000..80995a63449c
--- /dev/null
+++ b/tests/phpunit/integration/includes/logging/ManualLogEntryTest.php
@@ -0,0 +1,101 @@
+<?php
+
+namespace MediaWiki\Tests\Integration\Logging;
+
+use MediaWiki\Deferred\DeferredUpdates;
+use MediaWiki\Logging\LogEntryBase;
+use MediaWiki\Logging\LogPage;
+use MediaWiki\Logging\ManualLogEntry;
+use MediaWiki\RecentChanges\RecentChange;
+use MediaWikiIntegrationTestCase;
+use Wikimedia\ScopedCallback;
+
+/**
+ * @group Database
+ * @covers \MediaWiki\Logging\ManualLogEntry
+ */
+class ManualLogEntryTest extends MediaWikiIntegrationTestCase {
+	/** @dataProvider provideCreationAndPublishingToRecentChanges */
+	public function testCreationAndPublishingToRecentChanges( bool $userHasBotRight ) {
+		$performer = $this->getTestUser()->getUser();
+		$target = $this->getExistingTestPage()->getTitle();
+		$logParams = [ '4::test' => 'testabc1234', 'testing' => 'testingabc' ];
+
+		// Give the performer the bot right temporarily to allow testing that ::publish
+		// (if specified by this test case), so that we can test that ::publish sets the RecentChanges bot flag
+		// outside any DeferredUpdates to prevent issues such as described in T387659.
+		if ( $userHasBotRight ) {
+			$userRightScope = $this->getServiceContainer()->getPermissionManager()
+				->addTemporaryUserRights( $performer, 'bot' );
+		}
+
+		// Create a log entry and publish it to just RecentChanges
+		$logEntry = new ManualLogEntry( 'phpunit', 'test' );
+		$logEntry->setPerformer( $performer );
+		$logEntry->setTarget( $target );
+		$logEntry->setComment( 'A very good reason' );
+		$logEntry->setTimestamp( '20300405060708' );
+		$logEntry->setParameters( $logParams );
+		$logEntry->setDeleted( LogPage::DELETED_ACTION );
+		$logId = $logEntry->insert();
+		$logEntry->publish( $logId, 'rc' );
+		ScopedCallback::consume( $userRightScope );
+
+		// Actually cause the writes to RecentChanges, as they are queued using DeferredUpdates.
+		DeferredUpdates::doUpdates();
+
+		// Assert that the log entry exists and that it matches what we provided above.
+		$this->newSelectQueryBuilder()
+			->select( [
+				'log_type', 'log_action', 'log_timestamp', 'log_namespace', 'log_title', 'log_deleted',
+				'log_actor', 'comment_text'
+			] )
+			->from( 'logging' )
+			->join( 'comment', null, 'log_comment_id=comment_id' )
+			->where( [ 'log_id' => $logId ] )
+			->caller( __METHOD__ )
+			->assertRowValue( [
+				'phpunit', 'test', $this->getDb()->timestamp( '20300405060708' ), $target->getNamespace(),
+				$target->getDBkey(), LogPage::DELETED_ACTION, $performer->getActorId(), 'A very good reason',
+			] );
+		$this->assertArrayEquals(
+			$logParams,
+			LogEntryBase::extractParams(
+				$this->newSelectQueryBuilder()
+				->select( 'log_params' )
+				->from( 'logging' )
+				->caller( __METHOD__ )
+				->where( [ 'log_id' => $logId ] )
+				->fetchField()
+			),
+			false,
+			true
+		);
+
+		// Assert that the entry was sent to RecentChanges
+		$actualRecentChangeObject = RecentChange::newFromConds( [ 'rc_logid' => $logId ] );
+		$actualRecentChangeTitle = $actualRecentChangeObject->getPage();
+		$this->assertNotNull( $actualRecentChangeTitle );
+		$this->assertTrue( $target->isSamePageAs( $actualRecentChangeTitle ) );
+		$this->assertTrue( $performer->equals( $actualRecentChangeObject->getPerformerIdentity() ) );
+		$this->assertArrayContains(
+			[
+				// DB stores booleans as integers and we get back that integer as a string.
+				'rc_bot' => (string)intval( $userHasBotRight ),
+				'rc_log_type' => 'phpunit',
+				'rc_log_action' => 'test',
+				// The RecentChanges object stores the timestamp as TS_MW, even if DB stores it in a different
+				// format.
+				'rc_timestamp' => '20300405060708',
+			],
+			$actualRecentChangeObject->getAttributes()
+		);
+	}
+
+	public static function provideCreationAndPublishingToRecentChanges() {
+		return [
+			'User does not have bot right' => [ false ],
+			'User temporarily has the bot right' => [ true ],
+		];
+	}
+}