diff --git a/includes/ParamValidator/TypeDef/UserDef.php b/includes/ParamValidator/TypeDef/UserDef.php index 0be2b3d46cf..772510d3461 100644 --- a/includes/ParamValidator/TypeDef/UserDef.php +++ b/includes/ParamValidator/TypeDef/UserDef.php @@ -8,7 +8,6 @@ use MediaWiki\User\UserIdentity; use MediaWiki\User\UserIdentityLookup; use MediaWiki\User\UserIdentityValue; use MediaWiki\User\UserNameUtils; -use RequestContext; use TitleParser; use Wikimedia\IPUtils; use Wikimedia\Message\MessageValue; @@ -164,26 +163,19 @@ class UserDef extends TypeDef { private function processUser( string $value ): array { // A user ID? if ( preg_match( '/^#(\d+)$/D', $value, $m ) ) { - // We match the behavior of trying to create a User object - // based on the id. The name is: - // * the IP address of the current request, if the id is 0 - // * the name of the relevant user in the database, if one exists - // * the name 'Unknown user' if it does not exist + // This used to use the IP address of the current request if the + // id was 0, to match the behavior of User objects, but was switched + // to "Unknown user" because the former behavior is likely unexpected. + // If the id corresponds to a user in the database, use that user, otherwise + // return a UserIdentityValue with id 0 (regardless of the input id) and + // the name "Unknown user" $userId = (int)$m[1]; - if ( $userId === 0 ) { - // TODO we probably shouldn't be falling back to the request IP, - // but if we really want to we shouldn't be doing it by accessing - // the global state like this - $currentIP = RequestContext::getMain()->getRequest()->getIP(); - return [ - 'id', - new UserIdentityValue( 0, IPUtils::sanitizeIP( $currentIP ) ) - ]; - } - // Check the database. - $userIdentity = $this->userIdentityLookup->getUserIdentityByUserId( $userId ); - if ( $userIdentity ) { - return [ 'id', $userIdentity ]; + if ( $userId !== 0 ) { + // Check the database. + $userIdentity = $this->userIdentityLookup->getUserIdentityByUserId( $userId ); + if ( $userIdentity ) { + return [ 'id', $userIdentity ]; + } } // Fall back to "Unknown user" return [ diff --git a/tests/phpunit/includes/ParamValidator/TypeDef/UserDefTest.php b/tests/phpunit/includes/ParamValidator/TypeDef/UserDefTest.php index 102b33190cc..ba7ca63a4b8 100644 --- a/tests/phpunit/includes/ParamValidator/TypeDef/UserDefTest.php +++ b/tests/phpunit/includes/ParamValidator/TypeDef/UserDefTest.php @@ -7,8 +7,6 @@ use MediaWiki\MediaWikiServices; use MediaWiki\User\UserIdentity; use MediaWiki\User\UserIdentityLookup; use MediaWiki\User\UserIdentityValue; -use RequestContext; -use WebRequest; use Wikimedia\Message\DataMessageValue; use Wikimedia\ParamValidator\ParamValidator; use Wikimedia\ParamValidator\SimpleCallbacks; @@ -413,35 +411,12 @@ class UserDefTest extends TypeDefTestCase { $this->assertSame( $expectName, $actual->getName() ); } - public function testProcessUser_id0() { - // User created by id, when that id is 0, falls back to context ip - // Use try-finally so that we don't interfere with other tests - $oldRequest = RequestContext::getMain()->getRequest(); - try { - $mockRequest = $this->createMock( WebRequest::class ); - $mockRequest->method( 'getIP' )->willReturn( '200.1.2.3' ); - RequestContext::getMain()->setRequest( $mockRequest ); - $userDef = $this->getInstance( new SimpleCallbacks( [] ), [] ); - $res = $userDef->validate( - '', // $name, unused here - '#0', - [ - UserDef::PARAM_ALLOWED_USER_TYPES => [ 'id' ], - UserDef::PARAM_RETURN_OBJECT => true, - ], // $settings - [] // $options, unused here - ); - $this->assertUserIdentity( $res, 0, '200.1.2.3' ); - } finally { - RequestContext::getMain()->setRequest( $oldRequest ); - } - } - - public function testProcessUser_missingId() { + /** + * @dataProvider provideMissingId + */ + public function testProcessUser_missingId( $missingId ) { // User created by id, does not exist, falls back to "Unknown user" // See our mock UserIdentityLookup for which ids and names exist - $missingId = 6; - $userDef = $this->getInstance( new SimpleCallbacks( [] ), [] ); $res = $userDef->validate( '', // $name, unused here @@ -457,6 +432,11 @@ class UserDefTest extends TypeDefTestCase { $this->assertUserIdentity( $res, 0, "Unknown user" ); } + public function provideMissingId() { + yield "0 no longer matches request ip" => [ 0 ]; + yield "Id with no user" => [ 6 ]; + } + public function testProcessUser_validId() { // User created by id, does exist // See our mock UserIdentityLookup for which ids and names exist