[schleppy_oc]

Phabricator was one of the first tools I brought to my latest company. We primarily use it for Differential because the arcanist command line tool is so easy to use. Even those that are somewhat git challenged benefit as it handles merging commands for you when you land a review.

The IRC channel is a great place for Q&A and epriestley et. al. are always there to offer help.

Before you bash it for being written in PHP, take a look at the source; it is unlike any other PHP code I have ever seen.

[psaintla]

Don't take this the wrong way but this is still PHP and I say this as someone that's been writing in PHP since 2001, although I've largely shifted away. It didn't take me long to find some really crazy stuff like these nested try-catch blocks in:

https://github.com/facebook/phabricator/blob/master/webroot/...

Also this comment is pretty telling:

  // If execution throws an exception and then trying to render that exception
  // throws another exception, we want to show the original exception, as it is
  // likely the root cause of the rendering exception.

I'm not saying this is a bad project, it looks excellent and the code is pretty good but you can't escape having to do some nutty things in PHP.

[epriest]

I wrote this code, but I'm not sure what's wrong with it. Can you explain what's bad about it and how you'd improve it?

---

To provide some context, this is the top-level entry point for all requests. It routes requests to code which does application-level things, like showing a code review or updating a task or whatever.

When the application-level things go wrong, we try to display a pretty, human-readable exception page with a nicely formatted stack trace (if you're in developer mode), etc. This page relies on some infrastructure subsystems (like configuration, to check for developer mode, and static resources, to include JS and CSS, and rendering, to actually produce the HTML) working correctly.

Normally, the exception is in application code and everything works fine. We produce a pretty page and send it back to the user.

However, sometimes there's an error in one of these subsystems. If we start rendering the page and encounter a second exception during rendering, we fall back to a less-pretty page which has bare text and no dependencies on subsystems. Normally, this happens during development when you've made a mistake in changing one of the subsystems, and all JS inclusion or all configuration checks are failing.

On this bare page, we want to retain the original exception (as well as show the followup exception) because it may be helpful in identifying and resolving the issue.

With the obvious caveat that I wrote the code, this architecture seems sensible and appropriate to me, and appears to function correctly in practice.

[psaintla]

I apologize for the late response I was looking at more of the code trying to get a better feel for what it is actually doing.

To answer your question, I'd start with why you are worried about exceptions in rendering your exceptions? Whatever you are using to render your exception should be so simple and stable that you shouldn't need to worry about it.

Also another question, from a fairly quick glance and I admit I didn't read through every last line of code, it appears as if all application execution is wrapped in a try/catch block, instead of exceptions being caught and handled closer to the potential points of failure is there a reason for this?

And one nitpick, because it was kind of a pain to follow some of the code but nested if/else statements five/six layers deep are generally kind of discouraged.

Thanks for taking time out of your weekend to respond and good luck to your team!

[nolok]

Code's fine, he's just taking a jab at php because that's hip

[psaintla]

Actually I'm not, I think PHP is a perfectly good language for certain things I'm just not blind to the issues with the language. I responded with my reasons that I believe that particular piece of code isn't so great.

[dylz]

You have to rethrow exceptions in other languages too. What's wrong with this?