2
\$\begingroup\$

I have coded a very simple error class that will allow me to display errors if they should happen. For example, if a file is missing or a database connection couldn't be established I would display a nice error, instead of revealing an ugly PHP error to them.

<?php
defined('LATRINA') or exit('You cannot view this file.');

class Error
{
    private $errorTitles = array();
    private $errorExplanations = array();
    private $errorDesign;

    public function load()
    {
        $this->errorTitles['myerror1'] = 'My Error 1';
        $this->errorExplanations['myerror1'] = 'This is my first error, just a small description.';

        $this->errorTitles['myerror2'] = 'Oh my, my second error!';
        $this->errorExplanations['myerror2'] = 'I really need to sort this error out, its a bad one!!';

        $this->errorTitles['myerror3'] = 'Well, well, well!';
        $this->errorExplanations['myerror3'] = 'Oh wow, this could potentially break a lot of shit..';
    }

    public function triggerError($error)
    {
        if (!isset($errorTitles[$error]) || !isset($errorExplanations[$error]))
        {
            exit('An error happened, unfortinately we couldn\'t handle it...');
        }

        echo '
        <!DOCTYPE html>
        <html lang="en-GB">
        <head>
            <title>' . Latrina::getCodeName() . ': Error</title>
            <style type="text/css">
            body {
                background-color: whitesmoke;
                font-family: sans-serif;
            }
            </style>
        </head>
        <body style="margin-top:4%;margin-left:4%;">
            <h1>'. $this->errorTitles[$error] . '</h1>
            <p>'. $this->errorExplanations[$error] . '</p>
        </body>
        </html>';

        exit();
    }
}

Usage:

Latrina::getLibrary('latrina.error.error')->triggerError('myerror2');
\$\endgroup\$
  • 3
    \$\begingroup\$ While expletives in private code are tolerable, I would avoid them in all code that potentially could be seen by many people. Such as code posted on the internet. It looks very unprofessional. \$\endgroup\$ – Graipher Dec 5 '16 at 16:25
  • \$\begingroup\$ Have a look at this library Tracy nette.github.io/tracy/tracy-exception.html \$\endgroup\$ – JasonDavis May 7 '17 at 1:06
1
\$\begingroup\$

I think there are a few large problems with this. At this point though I'm actually not going to review your code itself, but instead the general issues with how you are trying to solve this problem. Let's start with what I think is the biggest one:

This class is generally going to be useless if you have to explicitly call it for every error. In fact, a better solution would be such that you don't ever actually call it yourself.

The trouble is that there are any number of ways to trigger errors/exceptions in PHP, and you need to handle not just known errors in your application (which is what you seem to have in mind) but also unknown errors. Having an error handler that returns a nice message to the user in the event of a problem is a great idea, but it is useless if you accidentally reference an undefined variable without realizing it, and then your users see a standard PHP error. What that means is that you need to take over PHP's default error handler. Also, you need to take over its default exception handler (which is only used for uncaught exceptions). Then, while you're at it, you need to handle fatal errors. In all three of these cases (and you have to specifically deal with all three in PHP), you would then (potentially) be using your error handler class to display a nice message. To give a practical example, you can take over PHP's default error handler by doing something like this at the beginning of your application's bootstrapping process:

set_error_handler( function($errno, $errstr, $errfile, $errline){
    if (!(error_reporting() & $errno)){
        return false;
    }

    // Perform some logging here

    // output to user
    Latrina::getLibrary('latrina.error.error')->triggerError('php_error');
});

You handle uncaught exceptions by similarly setting a new exception handler via the set_exception_handler method. However, fatal errors will still leak through. Those don't have a dedicated error handler. Instead you find them by registering a shutdown function and checking for an outstanding error:

register_shutdown_function( function(){
    $error = error_get_last();
    if ( $error !== null ){
        // handle fatal error (log and display user message)
    }
});

Keep in mind that the point of this is not to attempt to recover from any unexpected errors (in the case of the exception handler and shutdown handler, you can't anyway), but instead to make sure that the error gets logged and the user sees your pretty error message. Here are some more details worth mentioning:

1. You need logging

Your code and your usage example suggests that you are specifically not logging anything. If these are errors then you need to be logging the fact that they are happening. Whether it is file logging, sending yourself an email, or shooting details off to an ELK stack for later analysis, you can't fix things if your application silently swallows your errors and leaves no trace.

2. Your application should not be in the public directory

The first first line of your PHP file is the sort of guard that is only necessary if your application is living inside of your public directory: aka the webserver can potentially pass requests directly to any file in your application. This is a very insecure setup. If you can, its better to move your application outside of the directories that are being hosted by your webserver.

3. Don't have a list of allowed errors

You've basically got a whitelist of allowed "errorcodes" that your system is allowed to generate, and which are used to lookup a user-friendly error message for the end user. Maintaining such a list of allowed error codes is going to be a pain, and you will inevitably make mistakes, which (with your current setup) will result in a less-user friendly error message sent to the user. Ditch the entire concept. That makes sense for API calls, but not for an internal application, and actually has some negative security implications. Instead the user should only ever see one single message "Whoop! We made a mistake! Someone will fix this soon." (or something along those lines). If you specifically change the error message sent to the user in response to what kind of failure is happening inside in your application, then you are basically leaking out details about the internals of your system. You never know when such an event might give a malicious user exactly the feedback they need to finish an attack. Only ever return a single error message to the user.

Putting it together

Just to summarize, here is what you want:

  1. Have an error handler set that logs errors and dumps a consistent and nice error message to the user
  2. Have an exception handler set that logs uncaught exceptions and dumps a consistent and nice error message to the user
  3. Have a shutdown_handler set that checks for an outstanding error (which happens for fatal errors) and which logs the error and dumps a consistent and nice error message to the user
  4. All of the above should use the same code for logging
  5. All of the above should use the same code for dumping an error message to the user ("Whoops! Sorry about that!").
  6. If you encounter a known error and want to stop executing, don't directly call your "error generator". Instead just throw an exception. It will get caught by your exception handler, logged, and a nice error will be sent to the user. This will be super-easy for you to manage as a developer, is much more in-line with normal coding standards (rather than a custom error generator), and gets you all the results you need.
\$\endgroup\$
  • \$\begingroup\$ surprisingly, this one is rather good \$\endgroup\$ – Your Common Sense Nov 3 '17 at 15:13
0
\$\begingroup\$

My primary feedback is that this is very hard-coded and inflexible.

I don't think you are going to want to have to change class code every time you want to make a change the end user messaging or HTML messaging format.

You have several concerns you are trying to address all within this single class:

  • error classification
  • determining end user message based on
  • defining display template (in this case and HTML template) for response tpo end user

These could all be broken out into classes that handle this responsibility. Consider something like:

  • ErrorDisplayTemplate - a class defining template to use for error display. Perhaps this takes form of an abstract base class that can be inherited for different template types (html, json, xml, etc.)

  • ErrorDisplayMapper - a class that maps system errors to end user messages

  • ErrorDisplayController - a class that uses classes above to render the display

  • UserFriendlyError - class defining fields being exposed for display (i.e. title, text, etc.)

This would allow you to decouple these pieces of functionality, giving more flexible usage such as:

// to render html
$user_error = ErrorDisplayMapper::getUserFriendlyError($some_system_error_info);
$template = ErrorDisplayTemplate::getTemplate('html');
ErrorDisplayController::render($user_error, $template);
exit();

// to render json
$user_error = ErrorDisplayMapper::getUserFriendlyError($some_system_error_info);
$template = ErrorDisplayTemplate::getTemplate('json');
ErrorDisplayController::render($user_error, $template);
exit();

Note here that you could easily define multiple templates for totally different types of responses without having to make changes to the other classes in the application.

So the primary takeaway for you is that this could certainly use some refactoring to separate these different concerns.

Some other thoughts:

  • Be meaningful and specific when naming objects (classes, functions, variables, etc.) in your code. Some poor naming examples include:
    • Error as class name. This is not an application-level error or error handling class, so please don't attach such a meaningful (in multiple contexts) term to this class. Something like UserFriendlyError is much more specific to what role this class is supposed to take in your application.
    • triggerError method does not "trigger" anything. It renders output. So a naming like renderError or displayError would be much more appropriate.
    • load method does not "load" anything. This way this class is hard-coded now, you could easily eliminate this method (and perhaps even provide more clarity in code) by having these definitions as constants on the class.
  • I don't see a case for why this should be a concrete class. Why would one need to instantiate an object of this class in order to access the behavior of this class. Consider whether this functionality should be use statically as in my usage example shown above.
  • What is $errorDesign property used for? If not used, remove it.
  • When your get into exceptional conditions in your code, it is usually bad practice to die/exit and directly message end user. For example, your triggerError method has guarding clause here where it is unclear whether this is truly an exceptional condition (the app should NEVER try to render an undefined error type) or whether you are just applying some fallback/default messaging. If truly an exceptional/unexpected case, then throw an Exception here.

To expand on my suggested usage above, you might leave it up to ErrorDisplayMapper class to determine what to do about an unknown error type - does it throw exception, or does it apply default messaging? If it applies default message, then you could use my example above as is, but if it throws an exception, perhaps you need to modify to be like this:

try {
    $user_error =
        ErrorDisplayMapper::getUserFriendlyError($some_system_error_info);
} catch (Exception $e) {
    // perhaps log exceptional case here
    // perhaps do something like this to recover
    $user_error = ErrorDisplayMapper::getUserFriendlyError(
        ErrorDisplayMapper::UNKNOWN_ERROR
    );       
}
$template = ErrorDisplayTemplate::getTemplate('html');
ErrorDisplayController::render($user_error, $template);
exit();
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.