Add options to UserDef to avoid potentially dangerous fallbacks
Open, Needs TriagePublic
Actions

Assigned To

None

Authored By

	Daimona
	Apr 28 2022, 5:50 PM

Description

UserDef::processUser has two fallbacks that are in my opinion dangerous and unwanted in some code.

If a user ID was given but no user was found, it will return

new UserIdentityValue( 0, "Unknown user" )

If given a valid user name but no user with that name exists, it returns

new UserIdentityValue( 0, $canonicalName )

The first case is awkward to handle for the caller, the second one is a bit better but still surprising. I believe there should be a way to make it return null or throw an exception in those cases. Possibly, the current behaviour would be removed altogether if nobody is relying on it.

These were introduced in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/703416 and haven't changed since then.

Related Objects

Mentioned Here: rMW825064fa759c: Add regression tests for UserDef::processUser()

Event Timeline

Daimona created this task.Apr 28 2022, 5:50 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 28 2022, 5:50 PM

These were introduced in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/703416 and haven't changed since then.

The use of UserIdentityValue was introduced then, but I made sure to match the existing behavior with User objects that had the same results as close as possible - see 825064fa759c21d22dae3535d60b80f47f04b42b where I added regression tests for the old behavior with user objects:

testProcessUser_missingName() - creating based on a user name that does not exist returned a User object with the given name and id 0

testProcessUser_missingId() - creating based on a missing id would result in a user object with that given id and no name, trying to fetch the name of that user would result in the object loading, the id then being set to 0, and the name is set to "Unknown user". Since this (waiting until after the name is requested to change the id to 0) could not be replicated with UserIdentity, I had it start with 0 from the beginning.

Add options to UserDef to avoid potentially dangerous fallbacksOpen, Needs TriagePublicActions

Description

Related Objects

Event Timeline

Add options to UserDef to avoid potentially dangerous fallbacks
Open, Needs TriagePublic
Actions