Page MenuHomePhabricator

Improve top-level exception handling
ClosedPublic

Authored by epriestley on Jan 1 2015, 7:01 PM.
Tags
None
Referenced Files
F14093419: D11126.diff
Mon, Nov 25, 11:07 AM
Unknown Object (File)
Sat, Nov 23, 12:49 PM
Unknown Object (File)
Sat, Nov 23, 12:49 PM
Unknown Object (File)
Sat, Nov 23, 9:08 AM
Unknown Object (File)
Fri, Nov 22, 2:20 PM
Unknown Object (File)
Fri, Nov 22, 2:19 PM
Unknown Object (File)
Wed, Nov 20, 2:39 PM
Unknown Object (File)
Sat, Nov 16, 1:59 PM
Subscribers

Details

Summary

Fixes T6692. Addresses two main issues:

  • The write guard would sometimes not get disposed of on exception pathways, generating an unnecessary secondary error which was just a symptom of the original root error.
    • This was generally confusing and reduced the quality of reports we received because users would report the symptomatic error sometimes instead of the real error.
    • Instead, reflow the handling so that we always dispose of the write guard if we create one.
  • If we missed the Controller-level error page generation (normally, a nice page with full CSS, etc), we'd jump straight to Startup-level error page generation (very basic plain text).
    • A large class of errors occur too early or too late to be handled by Controller-level pages, but many of these errors are not fundamental, and the plain text page is excessively severe.
    • Provide a mid-level simple HTML error page for errors which can't get full CSS, but also aren't so fundamental that we have no recourse but plain text.
Test Plan

Mid-level errors now produce an intentional-looking error page:

Screen_Shot_2015-01-01_at_10.44.57_AM.png (769×1 px, 81 KB)

Verified that setup errors still render properly.

@chad, feel free to tweak the exception page -- I just did a rough pass on it. Like the setup error stuff, it doesn't have Celerity, so we can't use {$colors} and no other CSS will be loaded.

Diff Detail

Repository
rP Phabricator
Branch
topex
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 3461
Build 3468: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley retitled this revision from to Improve top-level exception handling.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: btrahan, chad.
epriestley added a subscriber: chad.
btrahan edited edge metadata.
btrahan added inline comments.
src/aphront/configuration/AphrontApplicationConfiguration.php
104–109

I know this is just a move, but doesn't this code block always run even if an administrator sets base-uri?

This revision is now accepted and ready to land.Jan 2 2015, 6:44 PM
src/aphront/configuration/AphrontApplicationConfiguration.php
104–109

Yeah. The "request base URI" is "the base URI which is implied by the request headers", and does not override "phabricator.base-uri". A better name might be "setFallbackBaseURI()" or something, maybe.

Calls like getURI() and getProductionURI() basically go:

if (phabricator.base-uri is set) {
  use that;
}
if (the "request base URI" is set) {
  use that;
}
throw an exception;

The comment is a little misleading because it's talking about logic elsewhere in the stack. I'll make it more clear.

epriestley edited edge metadata.
  • Make URI behavior more clear, hopefully.
This revision was automatically updated to reflect the committed changes.

Thanks, that makes tons of sense. :)