CodeSOD: Exceptoinal Spelling

Object-oriented languages often let you implement your own exception classes to hook into their structured exception handling. This is, usually, a good thing: you create your own custom types for your various errors, which makes various exception states more clear. PebkacException is more useful than just Exception, and you can build catch blocks specific to your application/API needs.

But there's a dark side to this feature. People want to hook functionality into their exception objects. Instead of just using them as a signal to announce a failure state, they slap features into them, like exceptions which automatically self log.

Muriel discovered this PHP SelfLoggingException object when trying to throw an exception caused the application to crash with a stack overflow.

class SelfLoggingException extends Exception { protected $cause = null; public function __construct($message, $cause) { if (!is_null($cause)) { $this->cause = $cause; } parent::__construct($message, 0, null); $logger = LoggerProvider::get('exceptoin'); $logger->warn($message); } public function get_cause() { if ($this->cause) { return $this->cause; } return $this->getPrevious(); } public function get_root_cause() { $root = $this; $root_found = false; while (!$root_found) { if ($root instanceof SelfLoggingException) { if ($root->get_cause()) { $root = $root->get_cause(); } else { $root_found = true; } } else { if ($root->getPrevious()) { $root = $root->getPrevious(); } else { $root_found = true; } } } }}

There's a lot going wrong here. For starters, the Exception base class accepts a $previous parameter in their constructor, which is the intended way to build a chain of exceptions. Instead of just, y'know, using that built in functionality, this class re-implements it with a $cause property. This has the result of making get_root_cause extra complicated, since now it has to be smart enough to walk two possible chains- back up a series of $causes, or getPrevious calls.

The real WTF, though, is logging from inside of an exception object. Logging is how you handle an exception. By putting behavior into an exception object, you're reducing the utility of your catch blocks- what exactly does it mean to catch a SelfLoggingException? You'll need to wander up the cause chain to understand what went wrong, meaning your catch block doesn't get to do the job. Essentially, everything this class does just makes exception handling worse.

There's another reason to not put behavior in your exception objects, though, which is illustrated in this code. Note this line: $logger = LoggerProvider::get('exceptoin');, which has exceptoinal spelling in it. There is no exceptoin LoggerProvider configured, so this get fails. Any attempt to construct a SelfLoggingException would cause a stack overflow as accessing the logger failed.

Since then, Muriel has "fixed" this class, which is to say she's corrected the typo. The SelfLoggingException no longer causes a crash. It is still in use.

[Advertisement] ProGet supports your applications, Docker containers, and third-party packages, allowing you to enforce quality standards across all components. Download and see how!