Request: Catching errors

[peterdenk]

Many lines of code would be saved if we could catch simple errors with the base model and have an error message echoed and/or logged. You know all those infrequent errors that you want to check for but don't want to make a pretty user friendly interface for handling since it's broke and any how only the man in charge can fix it. Like this:

->on_error('Foobar missing.') ->get_all(...);

->on_error('Multiple Foobars matching criteria.') ->get_one(...);

Could even have a default error message so that just this would do the trick:

->on_error() ->get_all(...);

Brilliant or simply stupid?

[jamierumbelow]

I've actually been thinking about this for a while. Rather than return errors like described above, I'd much rather throw exceptions:

try
{
    $book = $this->book->get(1);
}
catch (MY_Model\RecordNotFound $e)
{
    show_404();
}
catch (MY_Model\SQLError $e)
{
    log_message('error', $e->getMessage());
    $book = FALSE;
}

The reason I haven't implemented it yet is because I'm worried it will be a culture shock for a lot of CI devs (CI isn't exactly an exception-conducive environment).

The benefits of using exceptions are many: errors can be selectively ignored, easily logged, caught and re-thrown with new messages, reported to other parts of the application, not to mention all the extra context you get (stack traces).

I'd certainly favour an exception-based system, but what does the community think?

[facultymatt]

@jamierumbelow I'd be interested in seeing how the exceptions approach would play out. Especially after reading your book on API design, I'm beginning to see the power in throwing exceptions.

It might be helpful to see this approach in more detail, perhaps in a feature branch, so that the community could better understand how this method would effect things.

Personally, my only concern with this approach is this seems like it would add a ton of lines to the code - adding try/ catch/ catch lines to many of the model functions (ie: get, get_all, get_by, get_many_by, insert, update, delete). And this could, as you pointed out, be a bit of a shock. It could also make the code harder to follow.

[peterdenk]

Throwing an exception is indeed much more powerful and a beautiful way of handling, well, exceptions. However my concern is also that it would add a lot of extra code. But I have only played with exceptions so there might be a nice way of putting standard catchers in the base model so that one can either let the standard catcher take care of exceptions or write code to catch them. But I guess that the default catcher for all uncaught exceptions should realy be in CI.

[andrewryno]

Using the MY_Controller from Jamie I think you could just put the try/catch around the call_user_func function and do it that way. It would be hard to control on an error-by-error basis though.

[jamierumbelow]

I'd rather not tie users into using MY_Controller as well as MY_Model... the two should be usable independently.

There are a few good benefits to forcing the usage of exceptions:

• Errors will be handled, full stop. It can be very easy to get complacent and forget about handling errors. This can lead to bad bugs that devs don't notice. Having it crap out as an exception is a great way of bringing this to one's attention.
• Errors can be tracked and logged much easier
• Certain errors can be ignored
• It's the right thing to do

The cons:

• We force people to use exceptions. This very well may reduce the number of people using MY_Model.
• We add complexity to the stack
• People get angry and shout and throw things

Perhaps a way of disabling exceptions if you so wish? The exception system would almost certainly be implemented as observers, much like how relationships work. We could add a boolean to the class to disable exceptions if you weren't interested.

class Post_model extends MY_Model { }
class Book_model extends MY_Model
{
    public $exceptions = FALSE;
}

$this->post_model->get(0); // throw new MY_Model\Exceptions\RecordNotFound
$this->book_model->get(0); // return FALSE;

[andrewryno]

I definitely think that allowing them to turn it off would be the best way of handling it.

[jamierumbelow]

If anyone fancies implementing this, be my guest. I'll get to it soon but am far too busy right now!

[inda5th]

+1 for for being able to turn it on and off.

[mikedfunk]

:+1: looking forward to this. I was just thinking about how to properly handle errors with the stuff I've learned from volumes 1 and 2.

[jamierumbelow]

I'm assuming that means nobody wants to implement it.... ;)

[peterdenk]

hmm, well, I might just give it a shot ... I'll get back with a description of how I plan to implement it in a few days

[peterdenk]

How errors are handled for a model will be set in

public $error;

by setting it to '' (default), 'exception', 'show' or 'log'.

Error handling can also temporarily be set to something else with these calls:

->no_error()        Inhibits all error handling.
->throw_exception() Throws an exception.
->show_error()      Shows the error using CI show_error().
->log_error()       Also logs the error using CI log_error().

To catch errors there will be six triggers. The first three can be combined, the last three are combinations.

on_none( $message ) No records.
on_one( $message )  One record.
on_many( $message ) Many (2+) records.

on_few( $message )  None or one record.
on_notone( $message )   None or many (2+) records.
on_exists( $message )   One or more records.

For each there is also a default error message.

Example use:

class Book_model extends MY_Model
{
    public $error = 'exception';
}

$this->book
    ->on_notone( )
    ->get( $id )

$this->book
    ->on_none( 'No books in the database.' )
    ->log_error()
    ->get_all()

[mikedfunk]

I really like that setup. Easy to implement as a chain and allows you to account for multiple possibilities.

[omegasteffy]

I need some way of protecting my site from database errors. However i do not see any way of checking for this in the base model. i call $one_entry=$this->vesMapper->get($ves_id); If debug is enabled in /config/database.php i get a quite raw error message Alternatively my there is just a execution stop (or php-error is this is enabled)

1. Is there any possible way other than modifying the base_model ? (i prefer correct usage over modification :-)

2. Are anyone familiar with a way to set the CI-database to throw errors

3. I guess I can manage to put in some simple error checking, but would like to ensure i cover all cases. I guess i can find the place to modify by looking up the before_xxx() calls?

[kbjohnson90]

Without modifying the code base, you could extend My_Model to clean the data based on what you expect as the input and write tests to verify. I believe this My_Model has some sort of 'before' function that you could use.

[omegasteffy]

I guess i need to clarify. It is not the data i expect errors from (that is handled just fine by validation) Is is error in the database connection. MySQL connection stall or etc.

[kbjohnson90]

You could run a try {} catch{} on your own code when you call a query (similar to what is being discussed above).

It won't fix the error, but it will allow you to 'catch' the error and handle it as you need, as opposed to falling back on the system or language erorrs.

[omegasteffy]

No, that is my point! No errors are thrown! code after modifications:

I do not see how to get my exception without modifying the code

class MY_model {
//missing everything 
public function get_by()
    {
        $where = func_get_args();

        if ($this->soft_delete && $this->_temporary_with_deleted !== TRUE)
        {
            $this->_database->where($this->soft_delete_key, (bool)$this->_temporary_only_deleted);
        }

        $this->_set_where($where);

        $this->trigger('before_get');

        $row_tmp = $this->_database->get($this->_table);

    if(!$row_tmp){      
        throw new Exception('database-error');
    }
    $row=$row_tmp->{$this->_return_type()}();
        $this->_temporary_return_type = $this->return_type;

        $row = $this->trigger('after_get', $row);

        $this->_with = array();
        return $row;
    }

}

Before modification line 144:

$row = $this->_database->get($this->_table)
                        ->{$this->_return_type()}();
//no exceptions are thrown but program stop
 //enabling ini_set('display_errors', 1);  enables me to see that 

[jamierumbelow]

@omegasteffy Unfortunately, I never got round to implementing an exceptions-based error handling system, even though this was my intention. Feel free to modify the core code as per the above suggestion and submit a pull request.

[omegasteffy]

I will consider to do so. However I am not certain the exception-based approach is needed.

My problem is that there is no error handling for dB-connection failure... or rather i do not see how. Is another (intended) approach ?

[kbjohnson90]

"How to use throw exception in mysql database connect" http://stackoverflow.com/a/9836727

try
{
    if ($db = mysqli_connect($hostname_db, $username_db, $password_db))
    {
        //do something
    }
    else
    {
        throw new Exception('Unable to connect');
    }
}
catch(Exception $e)
{
    echo $e->getMessage();
}

Alternatively,

http://stackoverflow.com/a/9836807

/* It is documented here http://ie2.php.net/manual/en/mysqli.error.php */

if (mysqli_connect_errno()) {
    throw new RuntimeException("Connect failed: %s\n", mysqli_connect_error());
}

Or,

To hide the PHP Error, use the @ operator

http://stackoverflow.com/a/6121357

$connect = @mysql_connect(HOST, USER, PASS);//won't display the warning if any.
if (!$connect) { echo 'Server error. Please try again sometime. CON'; }

[kbjohnson90]

I will look into implementing an exceptions-based error handling system, also.