Page MenuHomePhabricator

Fix PhutilUTF8TestCase::testUTF8Convert for PHP 8
ClosedPublic

Authored by jrtc27 on Jan 11 2021, 2:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 21, 8:27 PM
Unknown Object (File)
Sun, Mar 10, 7:05 AM
Unknown Object (File)
Feb 15 2024, 3:58 AM
Unknown Object (File)
Feb 4 2024, 12:33 AM
Unknown Object (File)
Jan 13 2024, 5:27 PM
Unknown Object (File)
Jan 9 2024, 8:01 AM
Unknown Object (File)
Jan 9 2024, 8:00 AM
Unknown Object (File)
Jan 9 2024, 7:59 AM
Subscribers

Details

Summary

In PHP 8 passing an invalid encoding to mb_convert_encoding raises a
ValueError (which extends Error not Exception), so fix the test to also
catch Throwable (but leave the explicit Exception case for PHP 5, which
lacks Throwable).

Test Plan

Ran arc unit --everything

Diff Detail

Repository
rARC Arcanist
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 24994
Build 34484: arc lint + arc unit

Event Timeline

Does this just break a test or something?

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.

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.
This revision now requires changes to proceed.Jan 11 2021, 3:18 AM

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.

Catch Throwable in the test rather than converting to Exception inside phutil_utf8_convert

This revision is now accepted and ready to land.Jan 11 2021, 4:48 AM