Error presentation / logging

[Damian Mooyman]

I have a few comments about the way the current error handling / presentation is built, and would like to collect some ideas from you all on what expectations are of the current system, and ideas for how it can be improved.

As I understand it, here is the basic behaviour in the framework / cms module:

CMS:

• Page errors that occur through ContentController::httpError are formatted into code-customised responses through ErrorPage::response_for before being passed to RequestHandler::httpError. This gives us nice user-friendly errors using the current theme.

Framework:

• Errors that occur through RequestHandler::httpError allow extensions to intercept the extension, first by using onBeforeHTTPErrorXXX (where XXX is an error code), then by onBeforeHTTPError. A specific extension to a request handler could then  throw their own SS_HTTPResponse_Exception, although the default behaviour is to simply generate a plain text error message. 'RequestHandler' itself is an unextendable class, so extensions need to be applied to each individual subclass (Form, FormField, etc...).
• Director catches any SS_HTTPResponse_Exceptions in handleRequest, extracts the exceptions response, and returns it as its own.
• Exceptions other than SS_HTTPREsponse_Exception and user_error calls are handled by exceptionHandler / errorHandler (Debug.php). This logs the error and will perform one of two actions:
• For dev / cli environments it will print a descriptive stacktrace of the error.
• For live environments it renders any pre-generated error-XXX.html file in assets (where XXX is an error code). There is a bit of class_exists('ErrorPage') tying the framework back into the CMS here.

Both

• As with the Director class, various RequestHandler instances that call handleRequest on nested handlers catch any thrown SS_HTTPResponse_Exception instances, simply turning the contained SS_HTTPResponse object as its own.

Having different ways of presenting the custom error page (whether by live DB records as in ContentController, or by static pre-generated files) is quite understandable, but I'm a bit concerned about errors from other RequestHandler instances simply outputting their error directly to the browser (live, dev, or whatever). Additionally, even if an error is handled by ContentController::httpError, SS_HTTPResponse_Exceptions completely bypass the error logging system in Debug, meaning any email (or other logging) notifications aren't sent.

Would it be ok to suggest a rethink of the request / response stack a little in order to develop a better error notification, as well as a better reporting system?

I also understand that there's a necessity to separate CMS module (which contains ErrorPage) from the core framework, but could error handling and presentation not be done in a bit more of an "injected" fashion so that the CMS can extend the built in error handling?

For error logging I can simply add a writer with SS_Log::add_writer. Could error handling (perhaps at the Director / Debug level) not also be controlled in a similar fashion? Users could then add their own injected behaviour (e.g. creating a 404 page logging system that behaves differently to normal errors).

Whatever the solution, a default framework / cms install should not be outputting plain text error messages in the live environment, IMHO at least.

As with everything in Silverstripe, I'm probably just lacking knowledge as to why the system was built the way it was, and why certain decisions were made the way they were, so please let me know what I'm missing, or if I've failed to understand anything correctly. :)

Thank you!

Damian

[Ingo Schommer]

Thanks for documenting this, I'm actually tempted to clean this up and add it to error-handling.md :)

At the beginning there only was ErrorPage::response_for(), and a lot of user_error() invocations.

The concept of an SS_HTTPResponse_Exception was a bit newer, which is probably why

it hasn't affected the design/operation of ErrorPage::response_for().

I agree that any string passed into SS_HTTPResponse_Exception should

not be considered relevant to a website user, and in some cases might even

reveal too much information.

On the other hand, you don't want to log every "permission denied" or "page not found"

to be logged by default, right? Particularly because on a HTTP status code level, that's

already being logged by the webserver. You're right though, ContentController::httpError()

shouldn't be handled separately from other SS_HTTPResponse_Exception.

And by unifying our approach, we'd allow for consistent customizations via

onBeforeHTTPError.

Looking forward to your thoughts and patches :)

Ingo

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to silverstripe-d...@googlegroups.com.
To post to this group, send email to silverst...@googlegroups.com.
Visit this group at http://groups.google.com/group/silverstripe-dev.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[Damian Mooyman]

Thanks Ingo for your feedback,

I'll be coming up with some ideas over the next few months, maybe looking at some update for 3.2 to better customise error handling.

I think a possible solution could look something like this:

• Ditch any attempt at error handling at the director / controller level. These should only raise errors, not handle them.
• Get rid of ContentController::httpError, leave RequestHandler::httpError as is.
• Implement a pair of abstract classes to represent ErrorLogger and ErrorPresenter. Debug could be assigned any number of either, perhaps with some kind of priority? 
• I think that SS_Log would be a part of, rather than replaced by, this system.
• ErrorPage presenter could be one (but not only) presenter.
• ErrorPresenters would probably execute in succession until one generated a response, while ErrorLoggers would probably all execute on each error.
• Each handler could be configured to only handle under certain conditions, with failover down the chain, allowing multiple actions to occur on each error. Filters could be based on:
  
• Environment
  
• Error type (exception / error).
  
• HTTP error code
  
• Error level (warning, notice, error)
• Probably link it all together using the same config / yaml / Injector pattern I used for my ORM rewrite. Not sure if that really follows the desired convention though... (multiple named configurations, some of which could be based on the same class with different options / dependencies). https://github.com/tractorcow/sapphire/blob/3.2-pdo-connector/_config/database.yml. Perhaps if the error code should be lighter it could simply be stored in either a singleton object or statics, and modified in _config.php.
  
• Error handling within the error handling would probably have to be quite brutal, either by ignoring errors (lots of @ and catch(){} and treating as failed), or by having a default bare bones error handler.
  
• Not sure what the 'base' error behaviour would be?

Bear in mind, I'm still overcoming my ignorance of the current system, so my ideas above are purely hypothetical conjecture. :) I'd love to write some code, but I'd prefer to get as much developer input and advice up front before I launch into anything.

When I was developing the ORM code I really had some great advice on the class separation of each DB controller that I found really successful in the long run. Hope I can get some good advice from others for this project too.

Damian