2
\$\begingroup\$

This is a simplified version of ExceptionHandler.php:

<?php
class ExceptionHandler
{
 public function __construct()
 {
 set_exception_handler(array($this, 'handle'));
 }
 public function handle($exception)
 {
 // output some information about the exception
 }
}

I'm wondering where the best place is to call set_exception_handler:

  • Register itself in the constructor, as above? Usage would simply be:

    $exceptionHandler = new ExceptionHandler;
    
  • Move the set_exception_handler call into a separate method, and call after instantiation:

    $exceptionHandler = new ExceptionHandler;
    $exceptionHandler->register(); 
    
  • Outside of the class entirely:

    $exceptionHandler = new ExceptionHandler;
    set_exception_handler(array($exceptionHandler, 'handle'));
    
asked Oct 29, 2014 at 10:51
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Some general things to consider:

  • As stated in the docs, the handler's signature is: void handler ( Exception $ex ), note the type-hint. Below is a lengthy explanation as to why this type-hint is important.
  • Your handler is not static (nor should it be). set_exception_handler expects the argument to be callable: set_exception_handler ( callable $exception_handler ). What if I destroy your class? The handler is not unregistered. I'd like to have a method that calls the restore_exception_handler function. Removing the handler this class registers.
  • Add properties that capture the exception handlers' state prior to registering your handler, so you can restore it correctly, and allow users to pass arguments to the contructor that allow them to control the behavior of your handler
  • Whenever you're writing echo in a class method, take a step back. Think of a case where you really don't want to echo everything to the end-user. If your handler catches an unhandled PDOException instance, do you really want the end-user to see everything about how, when and where you're trying to connect to the DB? Probably not.

There are a lot of things you should think about when writing something like an exception handler. It's a component that plays a vital part of any sizable application. Therefore, it should allow a lot of customization, and it should be extendable. This, in turn, implies that your base handler is as generic as is humanly possible. Generic code is hard to get right, so it's a very good exercise. I'll leave you with these critiques for now. If you want to, apply them to your code, and perhaps submit the result for another review here later.

Also take a peek at how other projects implement exception handling (Symfony2, Zf2, Codeignitor, ... they're all open source. Copy-pasting their code is not allowed, but you can look at them for inspiration)

Am I calling set_exception_handler in a suitable place?

IMHO, not really. If I use an exception handler class, I'd expect to be able to set it up some more. I want to customize a thing or two like: where does the exception handler send its output to, how should the exception handler format the exceptions. I may want to set the handler up differently depending on the environment (production behavior vs development).
When I see a handler as minimalist as yours (minimalism is a good thing in code), I'd probably think about extending it, to best suite my needs. I might add a logger property, which I can use to write output to (instead of echoing it). I could also choose to log the exceptions to the DB, or have the handler call a webservice. Your class makes this rather difficult to do:

You've probably come across references to the SOLID principles. An important part of this, and one that is too often overlooked or not understood, is the Liskov principle. This states that, in order for a child to comply with the substitution principle, it is not allowed for that child to strengthen the contract it inherits from the parent. What does that mean, well let's look at your handle method's signature:

public function handle($exception)

If I extend your class, but I don't want this handle method to handle objects that aren't instances of the Exception class, I might want to add a type-hint:

class MyHandler extends ExceptionHandler
{
 public function handle(Exception $exception)//only accepts Exception instances
 {}
}

This, according to the Liskov principle is not allowed. Because MyHandler extends your class, any code using my child class has to be able to assume that the contract (number of arguments, type requirements and visibility of members) is the same. Meaning: public properties remain public, protected properties have to be at least protected, too (public is allowed, private is not).
By my adding this simple type-hint, I'm breaking this rule. To clarify, take this example:

class Model
{
 public $data = null;
 public function setData($value)
 {
 $this->data = $value;
 return $this;
 }
}
class ArrayModel extends Model
{
 protected $data = [];//<-- reduce visibility
 public function getData($offset = null)
 {
 if ($offset === null)
 return $this->data;
 if (!isset($this->data[$offset]))
 return null;
 retrun $this->data[$offset];
 }
 public function setData(array $value)//strengthen argument restrictions
 {
 $this->data = $value;
 return $this;
 }
}

Now, at first glance, you may not see the problem: I'm using a generic Model class as a data container, and extend it to create a data container that only deals with arrays. But let's say there's a piece of code that looks like this:

function initData(Model $model)
{
 if ($model->data === null)
 $model->setData('initialized');
 return $model;
}

This function uses a type-hint to ensure that the $model argument will be an instance of the Model class. Because this is guaranteed to be the case (if not, PHP will generate a fatal error), it should be safe to assume that the data property is public, and the setData method accepts a string as an argument. However, if I were to pass it an instance of ArrayModel, these assumptions are false: data is protected, and setData only accepts an array argument.

Bottom line

Your ExceptionHandler class is too permissive to be extended in a safe, meaningful way.

Now this wouldn't be much of a problem if I were able to do everything I wanted with your class as it stands, but like I said in the beginning: I can't.

Because you simply have to add customization support to your class, and rethink the contract this class imposes, you simply can't call set_exception_handler in the constructor. I want to be able to extend this class, set up everything I need to set up, and then register a handler when, where, and how I want to register it.

I also want to be able to register several handlers, and perhaps unregister them if I need to. Your class seems to have been written under the assumption that no other handlers are registered, and no other handlers will be registered after your handle method. That's just not right.

answered Oct 29, 2014 at 12:19
\$\endgroup\$
2
  • \$\begingroup\$ Thank you so much for taking the time to write another detailed and clear answer. You may have been writing this while I edited my question but this answers it. \$\endgroup\$ Commented Oct 29, 2014 at 12:26
  • \$\begingroup\$ @ZougenMoriver: I've seen the edit, so I added a short list of key issues, and a preachy piece of friendly advice/encouragement: Exception handlers are difficult components to write, but they're very educational. Stick with it, and enjoy the experience \$\endgroup\$ Commented Oct 29, 2014 at 12:33

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.