Page MenuHomePhabricator

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


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 and haven't changed since then.

Event Timeline

These were introduced in 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.