Page MenuHomePhabricator

Improve top-level fatal exception handling in PHP 7+
ClosedPublic

Authored by epriestley on Feb 11 2019, 6:05 PM.

Details

Summary

Depends on D20137. Ref T13250. Ref T12101. In versions of PHP beyond 7, various engine errors are gradually changing from internal fatals or internal errors to Throwables, a superclass of Exception.

This is generally a good change, but code written against PHP 5.x before Throwable was introduced may not catch these errors, even when the code is intended to be a top-level exception handler.

(The double-catch pattern here and elsewhere is because Throwable does not exist in older PHP, so catch (Throwable $ex) catches nothing. The Exception $ex clause catches everything in old PHP, the Throwable $ex clause catches everything in newer PHP.)

Generalize some Exception into Throwable.

Test Plan
  • Added a bogus function call to the rendering stack.
  • Before change: got a blank page.
  • After change: nice exception page.

Screen Shot 2019-02-11 at 9.59.12 AM.png (870×1 px, 231 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This is much better than having to tab over to the terminal where my nginx logs are scrolling when I'm fixing a fatal!

src/aphront/configuration/AphrontApplicationConfiguration.php
300–307

I think the Throwable vs Exception difference is worth putting in a comment here since I would have naively assumed that this code was redundant.

support/startup/PhabricatorStartup.php
327

It looks like the Throwable interface starts exists starting in PHP 7.0: https://3v4l.org/uCbXG

I am deeply saddened to learn that PHP is much more willing to let you use a non-existent class/interface as the declared type of a catch block than as a typehint: https://3v4l.org/GAPKY

webroot/index.php
15–32

Maybe another comment here similar to the above.

This revision is now accepted and ready to land.Feb 11 2019, 7:34 PM

I am deeply saddened to learn that PHP is much more willing to let you use a non-existent class/interface as the declared type of a catch block than as a typehint

I think both cases are roughly consistent -- this works fine everywhere too: https://3v4l.org/f88Xl

  • Minor correctness tweak for top-level handler.
  • Add some explanatory text.
This revision was automatically updated to reflect the committed changes.