In D21501#272924, @epriestley wrote:

Does this just break a test or something?

Yes, that single test (and no other test) kills the entire engine because there's nothing to catch the unhandled throwable, which is a particularly sad failure mode. It seems like the engine could benefit from a try {} catch (Exception) {} catch (Throwable) {} so it can keep trying other tests and mark that one as failed.

If so, I think modifying the test to catch Error (or, specifically, either Throwable or Exception) is likely a better fix -- callers in Phabricator catching generic exceptions generally should not distinguish between Throwable and Exception because of PHP version differences. That is, code which does catch (Exception ...) without catch (Throwable ...) usually has the wrong behavior under certain conditions in some version of PHP. It's also rare that you want to do something in response to any Exception but not any Throwable. See similar T12855, previously.

I had originally added ValueError to the test but then saw that phutil_utf8_convert has the following comment:

* [...]. We can detect failures caused by invalid
* encoding names, [...]

so given it explicitly tries to handle that I thought it made sense to preserve the same interface, especially if it's something other libraries might be relying on (though probably unlikely, I'd hope they're not actually passing invalid encodings).

This is probably a true enough rule that it should be added as a lint warning ("Block catches Exception without catching Throwable"); I suspect there are very few (and perhaps zero) legitimate cases in the codebase where we really want to catch only Exception subclasses.

If this is trickier than "breaks a test" and we really want to re-throw, throwing a PhutilProxyException is usually preferable to proxying the lower-level exception as a string with "%s", since it preserves the lower-level exception stack.

The explicit use of ValueError is also slightly tricky because lint under older versions of PHP will raise an error ("Unknown class 'ValueError'..."). This can be corrected in configuration and will probably have to be corrected eventually (see D18809 for a similar change) but we can kick that can down the road if we don't need to use ValueError explicitly.

Upshot:

• If this just breaks a test, I think catch (Throwable ...) in the test is probably a better fix.
• If we retain a new throw that wraps another exception, PhutilProxyException is usually cleaner than using "%s" if there's no reason we need Exception specifically.
• If ValueError appears in the final change as a literal symbol, add it to arcanist/support/lib/extract-symbols.php following D18809 (the file was called phutil_symbols.php at the time, but they're the same script) to avoid lint warnings when running on a version of PHP older than PHP 8.
• I'll add a note about linting for catch (Exception ...) without catch (Throwable ...) in the same statement list, these are probably almost all incorrect in the Phabricator codebase.

I'll add a Throwable handler to the test and drop the phutil_utf8_convert change.

Sounds good. I think it's very unlikely anything is relying on the type of exception thrown, and we would (or, at least, should) normally throw a narrower exception (UTF8ConversionEncodingFailedVeryNarrowlyException) if really trying to make this part of the API.