Page MenuHomePhabricator

Improve top-level exception handling
ClosedPublic

Authored by epriestley on Jan 1 2015, 7:01 PM.
Tags
None
Referenced Files
F14027547: D11126.diff
Fri, Nov 8, 7:52 AM
F14005500: D11126.id26774.diff
Sun, Oct 27, 2:09 PM
F14002347: D11126.id26773.diff
Fri, Oct 25, 6:07 PM
F14002345: D11126.id26718.diff
Fri, Oct 25, 6:07 PM
F13986392: D11126.id.diff
Mon, Oct 21, 2:35 AM
F13977823: D11126.diff
Fri, Oct 18, 8:15 PM
Unknown Object (File)
Sep 20 2024, 12:59 AM
Unknown Object (File)
Sep 10 2024, 3:31 AM
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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. :)