Page MenuHomePhabricator

Consider making NameTableStore easier to use
Open, Needs TriagePublic

Description

There are two things about NameTableStore that make it (IMO) unreasonably hard to use:

  1. getID() lookups for non-existent names throw exceptions rather than returning a bottom value (e.g. null). This results in ugly try-catch blocks everywhere, and code review comments like this one. In particular, this code could have been a one-liner with array_map if null had been used instead of an exception.
  2. Passing in an ID field from the database to getName() throws an exception, because our DB stack returns strings rather than integers for these. $store->getName( $row->foo_id ) is intuitive to write, but it'll blow up in your face as this code did, because $row->foo_id is a string, and getName() throws an exception if you pass it anything other than an integer. This has already necessitated a revert to unbreak beta labs once, and it'll keep happening because nobody will remember to do $storage->getName( intval( $row->foo_id ) ).

I propose that getID() return null instead of throwing a NameTableAccessException, and that getName() cast string inputs to an integer (at the very least if they pass is_numeric()).

Related Objects

Event Timeline

I propose that getID() return null instead of throwing a NameTableAccessException

I agree that looking up an ID for an unknown name is not exceptional, since names can be user input. However, the calling code still needs to check for null. I'm not opposed to the change, I'm just saying that a missing check for null may go unnoticed, hiding errors, or failing later, when the value is used. Also, an if/then check isn't much nicer than a try/catch check.

and that getName() cast string inputs to an integer (at the very least if they pass is_numeric()).

Passing in a non-integer should throw an error. We could cast to int and then check that the result is > 0, since passing 0 is also an error.
Note that getName() should throw an exception when passed an unknown ID, since that indicates database corruption.

This has already necessitated a revert to unbreak beta labs once, and it'll keep happening because nobody will remember to do $storage->getName( intval( $row->foo_id ) )

If everybody remembered to write unit tests for all their code, they wouldn't have to know this.

Passing in a non-integer should throw an error. We could cast to int and then check that the result is > 0, since passing 0 is also an error.

The action API has some logic for sane integer conversion (converting to int, then back to string, and verifying that it's the same string other than maybe stripped whitespace). That could be turned into a generic utility and used in such cases.

If everybody remembered to write unit tests for all their code, they wouldn't have to know this.

Unit tests won't include actual DB calls so they are unlikely to help here. (I quite like the idea of faking type hints for DB results, as done e.g. here and here. But even that only helps with simple queries.)