1

I'm currently working in PHP. I'm working on a error system for my CMS I'm building (for the fun of it). For fatal errors in my system (not in the php compiler) I created a FatalException class that extends the built in Exception class. Since these types of errors kinda halt the system anyway, I threw in a exit into the __construct.

    class FatalException extends Exception{  
        public function __construct($message) {
            exit("<h1 style='color:red;' >FATAL ERROR: $message </h1>");
        }
    }

So in my code I'll check something like the connection to the database and if it can't then I'll just throw a FatalException("Can't connect to database: $database_error_message"). It won't be in a try/catch block.

When I run the code and can't connect to the database, for example, all I see on the screen is a sentence in big red letters. So it works just fine, but is this bad practice/coding?

EDIT:

Actually in truth it didn't start out this way. I was originally logging the error and then exiting in the catch area, but then I thought, if all fatal errors are going to exit anyway then just put in the in constructor. Then I noticed it wasn't actually getting to the catch area it was exiting. So putting the statement in a try/catch block was kind of a moot point. Which lead to the question.

3 Answers3

3

If you're going to exit() unconditionally in a constructor, there's not really much point making it a constructor, let alone making the class an Exception. You could more simply (and honestly) have a static function called Fatal::Die($message).

The point of exceptions is that they describe what the error is (by having different classes for different exceptions) and can be caught - even if only to log them to a file and bail out of the program.

What if a particular page of your site could actually cope fine without a database connection (just missing the "latest news" or something)? Then it could catch( Database_Exception $e ) and continue, while the rest of your site just falls straight into the last-ditch "oh no something went wrong" message.

A message in big red letters is also not a great error-handling mechanism for anything that someone other than you will use - either they end up seeing technical details of the error when you're not looking, or you don't know what went wrong because you hid that error.

IMSoP
  • 89,526
  • 13
  • 117
  • 169
  • The showing the message in big red letters was suppose to be similar to Wordpress' "Can't connect to the database". That is shows if it can't connect to a database. Since all its settings are stored in a database. The error message from the database is only suppose to be shown if the show errors is on in the config.ini file, but I haven't gotten that far yet. – Christopher Wilson Apr 11 '13 at 21:54
  • @ChristopherWilson My point is that the display of the error should be up to the error *handler*, not the error *producer*: your database code shouldn't even know it's rendering a web page (it might be an AJAX callback). With exceptions (or some kind of central error handling function), the details can be stored in appropriate fields of the object, and output in an appropriate way by the code that's rendering. – IMSoP Apr 12 '13 at 09:19
  • Incidentally, Wordpress is not generally considered an example of good coding practices, particularly in terms of overall architecture. – IMSoP Apr 12 '13 at 09:37
  • I understand and that is what my system is doing now, but at the time I was simply testing the database object. A simple exit did its job nicely (by telling me it didn't work). Referring to Wordpress was just for an example. I'm not copying in in the slightest. I dislike Wordpress and Joomla for CMSs. Drupal seems overkill for my needs and wants, which is why I'm building my own :) – Christopher Wilson Apr 12 '13 at 19:49
2

Even you wrap exit() into a member function of an exception, you are not using the exception here for error handling, but just the exit() - the exception is never thrown, PHP comes to halt before that happens.

So it works just fine, but is this bad practice/coding?

Yes, it is bad practice. It also works just fine if you would have created yourself a function:

function error_exit($message) {
    exit("<h1 style='color:red;' >FATAL ERROR: $message </h1>");
}

Instead think about whether you want to use Excpetions or you want to exit().

M8R-1jmw5r
  • 4,896
  • 2
  • 18
  • 26
  • You're the only one to actually answer the question. I was just going to replace the exception with a exit anyway, but I was just wondering. – Christopher Wilson Apr 11 '13 at 22:10
  • Replace the exception with a function that contains the `exit()` first. Because later on you want to replace the `exit()` as well. The part about bad practice is 99% `exit()` in the end. Your code right now is just only chaotic. And also bad practice because it contains `exit()`. Wrapping it into a function is the way out for the moment. – M8R-1jmw5r Apr 11 '13 at 22:41
  • The First thing my code checks is for a connection to a database. Everything but a config.ini file is stored in the database. So if it can't connect to the database then there isn't much need for it to load anything else. Outputting a simple can't connect to the database is all I really need it to do. – Christopher Wilson Apr 11 '13 at 23:58
  • 1
    Well, better keep things flexible. Today you say "Database is King". In two years you say "Connection to OtherService is King". So if fundamental failure is part of your application (it is of every application, so do not feat that), take care with Exception Handling. E.g. Wrap all you application in one try {} catch {} block. You can pick those "fundamental exceptions" at that level. The users get's a message. Your application gets exception handling. At a central place. Testable. Not 100 places with some exit which makes breaking the floor under your feets. – M8R-1jmw5r Apr 12 '13 at 00:13
  • Actually I have already built my system so the database is actually a plugin. So the default is mysql, but if you wanted it could be a xml database, or a pure text based database, JSON if you had a desire. It could be actually anything I, or anyone else using it, wants it to be. So the flexibility is there. I have actually decided to use a custom error handler. Simply because php may have the ability but it isn't inherent to php core functions. I have to manually test everything, and manually throw exceptions. Even for the simplest of things. It prefers to show warnings and what not. – Christopher Wilson Apr 12 '13 at 16:59
1

This is a bad idea imho. An exception needs to be caught in order to function properly. You may however create a method like ->error( $index ); that attaches to every object you make. From there you can route the error to a specific class with a try / catch block to properly handle the errors.

class TestClass 
{
    public function error( $index )
    {
        try 
        {
        // Convert index to exception and throw it.
        }
        catch ( Exception $e )
        {
        // Handle the error
        }
    } 
}
$a = new TestClass();
$a->error( 1000 );

Do note that this will not work for exceptions that php throws, you'll need to catch these seperately or work with execution hubs.

Digitalis
  • 570
  • 2
  • 12