From 5adf116196ea97987e7bdb2085b73fdafc053b03 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Thu, 3 Jun 2021 07:06:08 -0700 Subject: [PATCH] Convert Skin::relevantUser to UserIdentity This is a breaking change, but I've reviewed all usages and there's nothing that requires full User object to be returned. Maybe it's too bold, following the deprecation process is not too hard either. Change-Id: Id22580945f594d7e0de533226f6c374a7a97fe1e --- RELEASE-NOTES-1.37 | 3 ++ includes/skins/Skin.php | 52 ++++++++++++----------- includes/specials/SpecialUserrights.php | 5 ++- tests/phpunit/includes/skins/SkinTest.php | 15 ++----- 4 files changed, 37 insertions(+), 38 deletions(-) diff --git a/RELEASE-NOTES-1.37 b/RELEASE-NOTES-1.37 index 9d905192cb9..431ee763c37 100644 --- a/RELEASE-NOTES-1.37 +++ b/RELEASE-NOTES-1.37 @@ -303,6 +303,9 @@ because of Phabricator reports. now returns Authority instead of User. Also, a number of it's @stable to override methods now accept Authority instead of User as $audience parameter. +* Skin::getRelevantUser now returns an instance of UserIdentity, not necessarily + a User object. There is no known usages in MediaWiki ecosystem that were not + satisfied with UserIdentity. * … === Deprecations in 1.37 === diff --git a/includes/skins/Skin.php b/includes/skins/Skin.php index 7c38b1d2ae9..ef0bc1e2afd 100644 --- a/includes/skins/Skin.php +++ b/includes/skins/Skin.php @@ -24,6 +24,8 @@ use MediaWiki\HookContainer\ProtectedHookAccessorTrait; use MediaWiki\MediaWikiServices; use MediaWiki\Revision\RevisionLookup; use MediaWiki\Revision\RevisionStore; +use MediaWiki\User\UserIdentity; +use MediaWiki\User\UserIdentityValue; use Wikimedia\WrappedString; use Wikimedia\WrappedStringList; @@ -54,9 +56,9 @@ abstract class Skin extends ContextSource { protected $mRelevantTitle = null; /** - * @var User|null + * @var UserIdentity|null|false */ - protected $mRelevantUser = null; + private $mRelevantUser = false; /** * @var string Stylesheets set to use. Subdirectory in skins/ where various stylesheets are @@ -419,9 +421,9 @@ abstract class Skin extends ContextSource { /** * @see self::getRelevantUser() - * @param User $u + * @param UserIdentity|null $u */ - public function setRelevantUser( User $u ) { + public function setRelevantUser( ?UserIdentity $u ) { $this->mRelevantUser = $u; } @@ -432,24 +434,21 @@ abstract class Skin extends ContextSource { * things like the toolbox can display the information they usually are only * able to display on a user's userpage and talkpage. * - * @return User|null Null if there's no relevant user or the viewer cannot view it. + * @return UserIdentity|null Null if there's no relevant user or the viewer cannot view it. */ - public function getRelevantUser() { - if ( $this->mRelevantUser === null ) { + public function getRelevantUser(): ?UserIdentity { + if ( $this->mRelevantUser === false ) { + $this->mRelevantUser = null; // false indicates we never attempted to load it. $title = $this->getRelevantTitle(); if ( $title->hasSubjectNamespace( NS_USER ) ) { - $rootUser = $title->getRootText(); $services = MediaWikiServices::getInstance(); + $rootUser = $title->getRootText(); $userNameUtils = $services->getUserNameUtils(); if ( $userNameUtils->isIP( $rootUser ) ) { - $this->mRelevantUser = User::newFromName( $rootUser, false ); + $this->mRelevantUser = UserIdentityValue::newAnonymous( $rootUser ); } else { - $user = User::newFromName( $rootUser, false ); - - if ( $user ) { - $user->load( User::READ_NORMAL ); - $this->mRelevantUser = $user->isRegistered() ? $user : null; - } + $user = $services->getUserIdentityLookup()->getUserIdentityByName( $rootUser ); + $this->mRelevantUser = $user && $user->isRegistered() ? $user : null; } } } @@ -457,12 +456,15 @@ abstract class Skin extends ContextSource { // The relevant user should only be set if it exists. However, if it exists but is hidden, // and the viewer cannot see hidden users, this exposes the fact that the user exists; // pretend like the user does not exist in such cases, by setting it to null. T120883 - if ( $this->mRelevantUser - && $this->mRelevantUser->isRegistered() - && $this->mRelevantUser->isHidden() - && !$this->getAuthority()->isAllowed( 'hideuser' ) - ) { - return null; + if ( $this->mRelevantUser && $this->mRelevantUser->isRegistered() ) { + $userBlock = MediaWikiServices::getInstance() + ->getBlockManager() + ->getUserBlock( $this->mRelevantUser, null, true ); + if ( $userBlock && $userBlock->getHideName() && + !$this->getAuthority()->isAllowed( 'hideuser' ) + ) { + $this->mRelevantUser = null; + } } return $this->mRelevantUser; @@ -1265,12 +1267,12 @@ abstract class Skin extends ContextSource { } /** - * @param User|int $id + * @param UserIdentity|int $id * @return bool */ public function showEmailUser( $id ) { - if ( $id instanceof User ) { - $targetUser = $id; + if ( $id instanceof UserIdentity ) { + $targetUser = User::newFromIdentity( $id ); } else { $targetUser = User::newFromId( $id ); } @@ -1663,7 +1665,7 @@ abstract class Skin extends ContextSource { ]; } - if ( !$user->isAnon() ) { + if ( $user->isRegistered() ) { if ( $this->getUser()->isRegistered() && $this->getConfig()->get( 'EnableSpecialMute' ) ) { $nav_urls['mute'] = [ 'text' => $this->msg( 'mute-preferences' )->text(), diff --git a/includes/specials/SpecialUserrights.php b/includes/specials/SpecialUserrights.php index 96d20fa9298..506611a268c 100644 --- a/includes/specials/SpecialUserrights.php +++ b/includes/specials/SpecialUserrights.php @@ -24,6 +24,7 @@ use MediaWiki\MediaWikiServices; use MediaWiki\User\UserGroupManager; use MediaWiki\User\UserGroupManagerFactory; +use MediaWiki\User\UserIdentity; use MediaWiki\User\UserNamePrefixSearch; use MediaWiki\User\UserNameUtils; @@ -83,13 +84,13 @@ class UserrightsPage extends SpecialPage { /** * Check whether the current user (from context) can change the target user's rights. * - * @param User $targetUser User whose rights are being changed + * @param UserIdentity $targetUser User whose rights are being changed * @param bool $checkIfSelf If false, assume that the current user can add/remove groups defined * in $wgGroupsAddToSelf / $wgGroupsRemoveFromSelf, without checking if it's the same as target * user * @return bool */ - public function userCanChangeRights( $targetUser, $checkIfSelf = true ) { + public function userCanChangeRights( UserIdentity $targetUser, $checkIfSelf = true ) { $isself = $this->getUser()->equals( $targetUser ); $available = $this->changeableGroups(); diff --git a/tests/phpunit/includes/skins/SkinTest.php b/tests/phpunit/includes/skins/SkinTest.php index d38e074156b..4e3d92a4f34 100644 --- a/tests/phpunit/includes/skins/SkinTest.php +++ b/tests/phpunit/includes/skins/SkinTest.php @@ -260,35 +260,28 @@ class SkinTest extends MediaWikiIntegrationTestCase { public function outputPage() { } }; - $relevantUser = User::newFromIdentity( - UserIdentityValue::newRegistered( 1, '123.123.123.123' ) - ); + $relevantUser = UserIdentityValue::newRegistered( 1, '123.123.123.123' ); $skin->setRelevantUser( $relevantUser ); $this->assertSame( $relevantUser, $skin->getRelevantUser() ); $blockManagerMock = $this->createNoOpMock( BlockManager::class, [ 'getUserBlock' ] ); $blockManagerMock->method( 'getUserBlock' ) - // ->with( $relevantUser ) + ->with( $relevantUser ) ->willReturn( new DatabaseBlock( [ 'address' => $relevantUser, 'by' => UserIdentityValue::newAnonymous( '123.123.123.123' ), 'hideName' => true ] ) ); - $relevantUser = User::newFromIdentity( - UserIdentityValue::newRegistered( 1, '123.123.123.123' ) - ); $this->setService( 'BlockManager', $blockManagerMock ); $ctx = RequestContext::getMain(); $ctx->setAuthority( $this->mockAnonNullAuthority() ); $skin->setContext( $ctx ); - $skin->setRelevantUser( $relevantUser ); $this->assertNull( $skin->getRelevantUser() ); - $relevantUser = User::newFromIdentity( - UserIdentityValue::newRegistered( 1, '123.123.123.123' ) - ); + $ctx->setAuthority( $this->mockAnonUltimateAuthority() ); $skin->setContext( $ctx ); $skin->setRelevantUser( $relevantUser ); + $skin->setRelevantUser( $relevantUser ); $this->assertSame( $relevantUser, $skin->getRelevantUser() ); }