From 1fb76fefbb4d2a17622e18ee3c0307f9b3dae99b Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Fri, 22 Jan 2021 19:00:26 -0600 Subject: [PATCH] Change RevisionStoreCacheRecord callback signature In order to convert the tests into tru unit tests. Bug: T272669 Change-Id: I6055b43381fdcc6dcdf69e9311979db0dccdb16b --- includes/Revision/RevisionStore.php | 17 +++++++-- .../Revision/RevisionStoreCacheRecord.php | 35 +++++-------------- tests/common/TestsAutoLoader.php | 1 - .../Revision/RevisionStoreCacheRecordTest.php | 29 ++++++++------- 4 files changed, 39 insertions(+), 43 deletions(-) rename tests/phpunit/{ => unit}/includes/Revision/RevisionStoreCacheRecordTest.php (79%) diff --git a/includes/Revision/RevisionStore.php b/includes/Revision/RevisionStore.php index 94c0d67b025..938eaedfe57 100644 --- a/includes/Revision/RevisionStore.php +++ b/includes/Revision/RevisionStore.php @@ -1574,19 +1574,30 @@ class RevisionStore if ( !$row && !( $queryFlags & self::READ_LATEST ) ) { // If we found no slots, try looking on the master database (T259738) $this->logger->info( - 'RevisionStoreCacheRecord refresh callback falling back to READ_LATEST.', + 'RevisionStoreCacheRecord refresh callback falling back to READ_LATEST.', [ 'revid' => $revId, 'trace' => wfBacktrace( true ) ] ); $dbw = $this->getDBConnectionRefForQueryFlags( self::READ_LATEST ); - return $this->fetchRevisionRowFromConds( + $row = $this->fetchRevisionRowFromConds( $dbw, [ 'rev_id' => intval( $revId ) ] ); } - return $row; + if ( !$row ) { + return [ null, null ]; + } + return [ + $row->rev_deleted, + User::newFromAnyId( + $row->rev_user ?? null, + $row->rev_user_text ?? null, + $row->rev_actor ?? null, + $this->dbDomain + ) + ]; }, $title, $user, $comment, $row, $slots, $this->dbDomain ); diff --git a/includes/Revision/RevisionStoreCacheRecord.php b/includes/Revision/RevisionStoreCacheRecord.php index 46b5f81c57c..e7873ee45ce 100644 --- a/includes/Revision/RevisionStoreCacheRecord.php +++ b/includes/Revision/RevisionStoreCacheRecord.php @@ -23,23 +23,21 @@ namespace MediaWiki\Revision; use CommentStoreComment; -use InvalidArgumentException; use MediaWiki\Page\PageIdentity; use MediaWiki\Permissions\Authority; use MediaWiki\User\UserIdentity; -use MediaWiki\User\UserIdentityValue; -use User; /** * A cached RevisionStoreRecord. Ensures that changes performed "behind the back" * of the cache do not cause the revision record to deliver stale data. * + * @internal * @since 1.33 */ class RevisionStoreCacheRecord extends RevisionStoreRecord { /** - * @var callable + * @var callable ( int $revId ): [ int $rev_deleted, UserIdentity $user ] */ private $mCallback; @@ -47,7 +45,8 @@ class RevisionStoreCacheRecord extends RevisionStoreRecord { * @note Avoid calling this constructor directly. Use the appropriate methods * in RevisionStore instead. * - * @param callable $callback Callback for loading data. Signature: function ( $id ): \stdClass + * @param callable $callback Callback for loading data. + * Signature: function ( int $revId ): [ int $rev_deleted, UserIdentity $user ] * @param PageIdentity $page The page this Revision is associated with. * @param UserIdentity $user * @param CommentStoreComment $comment @@ -57,7 +56,7 @@ class RevisionStoreCacheRecord extends RevisionStoreRecord { * @param bool|string $dbDomain DB domain of the relevant wiki or false for the current one. */ public function __construct( - $callback, + callable $callback, PageIdentity $page, UserIdentity $user, CommentStoreComment $comment, @@ -102,31 +101,15 @@ class RevisionStoreCacheRecord extends RevisionStoreRecord { * @throws RevisionAccessException if the row could not be loaded */ private function loadFreshRow() { - $freshRow = call_user_func( $this->mCallback, $this->mId ); + list( $freshRevDeleted, $freshUser ) = call_user_func( $this->mCallback, $this->mId ); // Set to null to ensure we do not make unnecessary queries for subsequent getter calls, // and to allow the closure to be freed. $this->mCallback = null; - if ( $freshRow ) { - $this->mDeleted = intval( $freshRow->rev_deleted ); - - try { - $this->mUser = User::newFromAnyId( - $freshRow->rev_user ?? null, - $freshRow->rev_user_text ?? null, - $freshRow->rev_actor ?? null - ); - } catch ( InvalidArgumentException $ex ) { - wfWarn( - __METHOD__ - . ': ' - . $this->mPage - . ': ' - . $ex->getMessage() - ); - $this->mUser = new UserIdentityValue( 0, 'Unknown user', 0 ); - } + if ( $freshRevDeleted !== null && $freshUser !== null ) { + $this->mDeleted = intval( $freshRevDeleted ); + $this->mUser = $freshUser; } else { throw new RevisionAccessException( 'Unable to load fresh row for rev_id: ' . $this->mId diff --git a/tests/common/TestsAutoLoader.php b/tests/common/TestsAutoLoader.php index 9229a874b96..d98e5456141 100644 --- a/tests/common/TestsAutoLoader.php +++ b/tests/common/TestsAutoLoader.php @@ -177,7 +177,6 @@ $wgAutoloadClasses += [ # tests/phpunit/includes/Revision 'MediaWiki\Tests\Revision\RevisionSlotsTest' => "$testDir/phpunit/includes/Revision/RevisionSlotsTest.php", 'MediaWiki\Tests\Revision\RevisionStoreDbTestBase' => "$testDir/phpunit/includes/Revision/RevisionStoreDbTestBase.php", - 'MediaWiki\Tests\Revision\RevisionStoreRecordTest' => "$testDir/phpunit/includes/Revision/RevisionStoreRecordTest.php", # test/phpunit/includes/user 'UserOptionsLookupTest' => "$testDir/phpunit/includes/user/UserOptionsLookupTest.php", diff --git a/tests/phpunit/includes/Revision/RevisionStoreCacheRecordTest.php b/tests/phpunit/unit/includes/Revision/RevisionStoreCacheRecordTest.php similarity index 79% rename from tests/phpunit/includes/Revision/RevisionStoreCacheRecordTest.php rename to tests/phpunit/unit/includes/Revision/RevisionStoreCacheRecordTest.php index c91ada3cb46..6e5e481af9e 100644 --- a/tests/phpunit/includes/Revision/RevisionStoreCacheRecordTest.php +++ b/tests/phpunit/unit/includes/Revision/RevisionStoreCacheRecordTest.php @@ -1,17 +1,17 @@ resetArticleID( 17 ); + $title = new PageIdentityValue( 17, NS_MAIN, 'Dummy', PageIdentity::LOCAL ); $user = new UserIdentityValue( 11, 'Tester', 0 ); $comment = CommentStoreComment::newUnsavedComment( 'Hello World' ); @@ -41,7 +40,7 @@ class RevisionStoreCacheRecordTest extends RevisionStoreRecordTest { $row = [ 'rev_id' => '7', - 'rev_page' => strval( $title->getArticleID() ), + 'rev_page' => '17', 'rev_timestamp' => '20200101000000', 'rev_deleted' => 0, 'rev_minor_edit' => 0, @@ -56,7 +55,11 @@ class RevisionStoreCacheRecordTest extends RevisionStoreRecordTest { if ( !$callback ) { $callback = function ( $revId ) use ( $row ) { - return (object)$row; + $this->assertSame( 7, $revId ); + return [ + $row['rev_deleted'], + new UserIdentityValue( intval( $row['rev_user'] ), 'Bla', 10 ) + ]; }; } @@ -74,14 +77,14 @@ class RevisionStoreCacheRecordTest extends RevisionStoreRecordTest { // Provide a callback that returns non-default values. Asserting the revision returns // these values confirms callback execution and behavior. Also confirm the callback // is only invoked once, even for multiple getter calls. - $rowOverrides = [ - 'rev_deleted' => RevisionRecord::DELETED_TEXT, - 'rev_user' => 12, - ]; $callbackInvoked = 0; - $callback = function ( $revId ) use ( &$callbackInvoked, $rowOverrides ) { + $callback = function ( $revId ) use ( &$callbackInvoked ) { + $this->assertSame( 7, $revId ); $callbackInvoked++; - return (object)$rowOverrides; + return [ + RevisionRecord::DELETED_TEXT, + new UserIdentityValue( 12, 'Lalala', 24 ) + ]; }; $rev = $this->newRevision( [], $callback );