bcit-ci / CodeIgniter

Open Source PHP Framework (originally from EllisLab)
https://codeigniter.com/
MIT License
18.27k stars 7.61k forks source link

Exception handling feature request #1590

Closed fideloper closed 9 years ago

fideloper commented 12 years ago

CodeIgniter handles errors, but not uncaught Exceptions.

  1. I submit that CI can fairly easily add in functionality to handle uncaught Exceptions via PHP's set_exception_handler()
  2. CI's code for error handling uses the terminology "Exception", which should change to "error" (And new Exception handling code will take its place as "Exception handler")

This will eventually result in a pull request. Files involved are:

  1. system/core/CodeIgniter.php, ~ line 67
<?php
/*
 * ------------------------------------------------------
 *  Define a custom error handler so we can log PHP errors
 * ------------------------------------------------------
 */
    set_error_handler('_error_handler'); //Change to _error_handler
    set_exception_handler('_exception_handler'); // Add in _exception_handler

    if ( ! is_php('5.3'))
    {
        @set_magic_quotes_runtime(0); // Kill magic quotes
    }
  1. system/core/Common.php ~ line 467
<?php
//Explanation and function changed to 'error' handler, rather than 'exception' handler

/**
* Error Handler
*
* This is the custom error handler that is declaired at the top
* of Codeigniter.php.  The main reason we use this is to permit
* PHP errors to be logged in our own log files since the user may
* not have access to server logs. Since this function
* effectively intercepts PHP errors, however, we also need
* to display errors based on the current error_reporting level.
* We do that with the use of a PHP error template.
*
* @access   private
* @return   void
*/
if ( ! function_exists('_error_handler'))
{
    function _error_handler($severity, $message, $filepath, $line)
    {
         // We don't bother with "strict" notices since they tend to fill up
         // the log file with excess information that isn't normally very helpful.
         // For example, if you are running PHP 5 and you use version 4 style
         // class functions (without prefixes like "public", "private", etc.)
         // you'll get notices telling you that these have been deprecated.
        if ($severity == E_STRICT)
        {
            return;
        }

        $_error =& load_class('Exceptions', 'core');

        // Should we display the error? We'll get the current error_reporting
        // level and add its bits with the severity bits to find out.
        if (($severity & error_reporting()) == $severity)
        {
            $_error->show_php_error($severity, $message, $filepath, $line);
        }

        // Should we log the error?  No?  We're done...
        if (config_item('log_threshold') == 0)
        {
            return;
        }

        $_error->log_exception($severity, $message, $filepath, $line);
    }
}

//Add in Exception handling

/**
* Exception Handler
*
* This is the custom exception handler that is declaired at the top
* of Codeigniter.php.  The main reason we use this is to permit
* PHP exceptions to be logged in our own log files since the user may
* not have access to server logs. 
*
* @access   private
* @return   void
*/
if ( ! function_exists('_exception_handler'))
{
    function _exception_handler($exc)
    {

        $_error =& load_class('Exceptions', 'core');

        $_error->show_php_error('Error', $exc->getMessage(), $exc->getFile(), $exc->getLine());

        // Should we log the error?  No?  We're done...
        if (config_item('log_threshold') == 0)
        {
            return;
        }

        $_error->log_exception('Error', $exc->getMessage(), $exc->getFile(), $exc->getLine());
    }
}

Improvements from this code snippet that would be nice:

  1. Ability to show stack trace in Exceptions
  2. Ability to hpass control of caught errors and exceptions to a controller, so errors can be shown to users in a pleasant way (And of course allow developers to add in logging / email or other alerts when exceptions occur).

What are your thoughts on this?

warpkid commented 12 years ago

Came across this whilst looking to a solution to handling exceptions in Codeigniter in a sane way. This seems good. Would like to see this become part of the core.

fideloper commented 12 years ago

I don't think they will, however. CodeIgniter's convention is to NOT use exceptions, and so adding this functionality in would go against the grain of their choice to use errors over exceptions.

It makes sense to not add it as that could cause havoc going against their own conventions.

That being said, exceptions > errors in my opinion, as they give the programmer better options for handling application errors.

But hey, there's Laravel for that, I guess.

warpkid commented 12 years ago

Ahhh, I see. I've not really up on the overall architecture of codeigniter but am aware of their error handling (well, sorta). I guess a lot of it is a throwback to the pre PHP 5 days. I'm with you on the whole Exceptions being better than CI Errors. As something now native to PHP it would be a shame for CI not to adopt them somehow in future revisions.

Coming from C# I found the implementation of exceptions in PHP's core functions and Codeigniter to be disappointing / non existent.

Thanks for the reply btw!

dchill42 commented 12 years ago

Actually, the CI_Exception::show_php_error method that is the heart of the registered exception handler in CodeIgniter does show a stack trace as long as the app is configured to show exceptions. As such, I'm not clear on what your suggestion would achieve. Maybe you just need to set your error reporting level and display_errors in index.php to make the exception pages show up.

As for routing caught exceptions to a controller for custom display, that is a good idea. #1818 (if merged) will add overrides for regular errors ( as set with show_error() ) and 404s. I could easily apply that same mechanism to show_php_error (thought about it - haven't done it yet).

Between those two things, I think you'd have what you're looking for with this request.

warpkid commented 12 years ago

Ah I see. I've taken a quick look at CI_Exception::show_php_error etc and it makes a little more sense. I guess all I would like to see is a simple method to intercept uncaught exceptions before they make it to the predefined codeigniter error handler.

Being able to define a route for uncaught exceptions sound like a good idea. Would be nice to be able to decide at that point what to do with the error and fire out the relevant view / messages etc.

dchill42 commented 11 years ago

Two things: First, I slightly misstated the location of the stack trace in my previous message - it's actually handled in the default error_php template, not directly in show_php_error. The effect, of course, is the same - the trace shows in the error page when the template is used.

Second, I added exception_override routing to #1818, such that users would be able to specify a URI-style path to an exception handler controller method. The method would receive severity, message, filepath, and line information (after the filtering that happens in show_php_error), followed by any arguments designated in the override route. As it stands now, your handler would have to add the stack trace to the output just like the template does, but I'm considering adding a method to the Exception class to encapsulate that functionality.