delete_by() function doesn't have correct return, possibly others

[facultymatt]

trafficstars

The function delete_by() will always return 1, even if nothing was deleted. To fix this the function should return $this->db->affected_rows(); instead of $result

Detailed example below with added code for debugging:

  /**
     * Delete a row from the database table by an arbitrary WHERE clause
     */
    public function delete_by()
    {
        $where = func_get_args();
        $this->_set_where($where);

        // added comment
        log_message('error', '$where params are:' . print_r($where, true));

        $where = $this->trigger('before_delete', $where);

        if ($this->soft_delete)
        {
            $result = $this->db->update($this->_table, array( $this->soft_delete_key => TRUE ));
        }
        else
        {
            $result = $this->db->delete($this->_table);
        }

        $this->trigger('after_delete', $result);

        // added comment
        log_message('error', '$result is:' . print_r($result, true));
        log_message('error', 'affected->rows is:' . print_r($this->db->affected_rows(), true));

        return $result;

    }

When you run the above code you'll notice that $result is always 1, even when nothing was deleted or updated. In comparison $this->db->affected_rows is accurate, showing 0 if nothing was updated and the number of affected rows if something was updated or deleted.

This same return happens in a few other spots, namely the update and delete functions.

The only logic I see for possibly returning $return is if we wanted to return the ACTUAL affected rows in cases of update, sending them back to the function as an object or array. However in this case I think you need to do another get using the updated ids, not 100% sure though.

[inda5th]

From my understanding, $result is returning TRUE because the DB ran the query successfully. Generally, you would know if the records exist before you delete them anyway. Then use $this->db->affected_rows() to compare the number of rows that you expected to be affected, to the number of rows that were actually affected.

But, perhaps a MY_Model::affected_rows() method should be added for consistency?

[facultymatt]

@inda5th your statement "$result is returning TRUE because the DB ran the query successfully" is correct, however just because the query was successfully ran doesn't mean anything was changed, it just means the query didn't break or fail.

For example, if you pass an incorrect ID (that doesn't exisit) to delete_by() nothing will be deleted but $result would still be true. This is where affected_rows() comes in handy, because you can check against it to make sure thing worked as expected.

I like your idea of adding MY_Model::affected_rows() but it seems more straightforward just return affected_rows() from each function, because I really don't see the need to $result as it stands now.

[jamierumbelow]

Okay, if someone wants to write a test and change the value of $result to be affected_rows(), that'd be awesome. It's going to be a while before I get the chance!

[dawondyifraw]

Thanks for the fix !