weblogics / Codeigniter-Ion-Auth-ACL

Add's ACL to Ion Auth
GNU General Public License v2.0
29 stars 19 forks source link

Multiple issues regarding Transactions #4

Closed chland closed 5 years ago

chland commented 6 years ago

IDK if you're still actively working on the library but I guess reporting issues is always a good thing :)

I played a little bit with your code and noticed that the demo-app wasn't working correctly when I modified the permission. After looking at the code I noticed a small bug in the remove_permission_from_user()-function in the model.

You start a transaction using start_trans() then you delete the permission from the database and exit the function. There is a COMMIT/ROLLBACK missing before exiting the function.

And while I was looking at the code i noticed that you were using trans_complete() after inserting/updating data and then you used trans_status() afterwards to determine if the transaction failed.

I'm not 100% sure as i haven't worked with transactions in CodeIgniter a whole lot but AFAIK, trans_complete() can set the trans_status to TRUE (if trans_strict is FALSE) even if the transaction fails. If you check Ben's code in his model, he is always handling the transactions "manually". So instead of doing something like you did:


        $this->db->trans_start();

    // DO SOMETHING

        $this->db->trans_complete();

        if ($this->db->trans_status() === false) {

            return false;

        } else {

            return true;

        }   

he is doing it like this:

        $this->db->trans_start();

    // DO SOMETHING

        if ($this->db->trans_status() === false) {

            $this->db->trans_rollback();

            return false;

        } else {

            $this->db->trans_commit();

            return true;

        }

This should prevent problems if the code is executed while another transaction was started earlier in the code. Or you should check the return-value of trans_complete() instead of using the value of trans_status() - that should work too as trans_complete returns FALSE if something goes wrong.

weblogics commented 5 years ago

Hey, sorry for the delay in my response on this, for some reason I didn't get any notifications regarding this issue.

Changes have now been applied.