I'm in the process of refactoring a fairly large web app. One of the major issues is inconsistent error handling and I'm trying to come up with a sensible strategy.
I've created a custom error handler, via set_error_handler that essentially turns PHP errors in ErrorExceptions and a custom base exception class, that directly inherits from Exception.
On production I'm using a generic exception catch-all, via set_exception_handler, and I'm about to add exception logging* to the mix. My dilemma is where to do the actual logging, in the base exception class or in the catch-all.
I've thought of a couple of reasons to log it in the catch-all:
- There are quite a few exceptions in the code that need to be converted to some appropriate child of the base exception class. Until that happens, not all exceptions will get logged.
- It somehow feels more natural to do it in the catch-all, a base exception class shouldn't do more than being just that. (It might be a single responsibility principle thing, but it could just be a misguided feeling)
and one reason to log in the base exception class:
- Currently the catch-all is only used on production. It would be easy to introduce it on our other environments (development, testing) but that would call for a few adjustments, as errors are handled differently per environment, as on production they are translated to 404/503 error pages.
Is there any acceptable practice for where to logging exceptions?
* Logging will involve writing to a text file at first, and it may evolve to sending mails for certain types of exceptions.
Some clarifications, prompted by @unholysampler's answer:
I'm facing a 2*10^6 sloc codebase, with a lot of third party stuff I have no control over, and some of the code I do have control over pre-dates exceptions in PHP. And there's also some crappy recent code, we are recovering from a long period of intense pressure where we virtually had to stop thinking and just hacked.
We are actively refactoring to address all the inconsistencies and introduce a sensible error handling approach, but that's going to take some time. I'm more interested in what to do until I reach the point where errors are handled appropriately. I'll probably ask another question on a sensible exception strategy at some point.
The main motivation behind logging is to get an email on my phone whenever something bad happens on production. I don't care if data dumps get huge, if they do I'll have a cron job deleting old ones every now and then.
Best Answer
In short, the only time you should log the existence of an exception is when you are handling it.
When you throw an exception, it is because your code has reached a state where it can not proceed correctly. By throwing an exception, you are representing a specific message to your program about the error that occurred. You should not catch an exception until you are at a point where it can properly be handled.
The code you write as part of your main application should be aware of the types of exceptions that can be thrown and when they might be thrown. If you can't do anything productive with an exception, don't catch it. Do not log an exception until it is being handled. Only the handling code knows what the exception means in the context of the program flow and how to respond to it. Writing a log message here may make sense here. If you use a logging framework, you can set a log level for the message and potentially filter them. This works well for exceptions that can occur, but are not critical and can be recovered from cleanly.
Your exception catch-all, is your last ditch effort to keep your code from crashing an ugly death. If you have gotten this far, you log all the state and error information you can. Then you do your best to nicely tell the user that the program is crashing before everything stops. Your goal should be to never have this code executed.
Embedding logging into the base class does not follow the above guidelines. The base class doesn't know anything about the state of the code. (Having the stack trace doesn't count because you are not going to write code that makes decisions based on parsing it.) The base class can't do anything to indicate the severity or how the exception might be handled. You don't want huge data dumps and stack traces every time there is a simple exception that you can handle and recover from cleanly.