Page MenuHomePhabricator

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

Authored by epriestley on Mon, Feb 11, 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.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Mon, Feb 11, 6:05 PM
epriestley requested review of this revision.Mon, Feb 11, 6:07 PM
amckinley accepted this revision.Mon, Feb 11, 7:34 PM

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.Mon, Feb 11, 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.