Put Transactions in the Controller or Model

[eluser]asciiCode[/eluser]
I have been reviewing my code and noticed that I am placing a lot of business logic in the model.

The reason for this is because I believe the database transactions have to be called in the model? Is this true or can I start a database transaction in the controller so it looks something like this in the controller

$this->db->trans_start();
$this->my_model->InsertSomeInfo();
$this->my_model->DeleteSomeInfo();
$this->db->trans_complete();

Thanks!

[eluser]sketchynix[/eluser]
You can call the database class from the controller or even the view, the MVC framework is there simply to keep your logic clean and separated.

[eluser]asciiCode[/eluser]
Just because you can do something doesn't mean you should. For example I am pretty sure you should not call any database calls in the view.

However is what I suggest above the best way to structure everything?

[eluser]sketchynix[/eluser]
Sorry, I should have been a bit clearer. I thought you were just asking if it was possible to call them from the controller.

Transactions should be housed in the model as it is direct manipulation of the database. If you have separated the Insert and Delete functions in your model, you would want to have each start and complete a transaction within their function and then in the controller, depending on the result of the function, have it call the next step.

Controller:

Code:

$insert_result = $this->my_model->InsertSomeInfo();
if($insert_result){
  $delete_result = $this->my_model->DeleteSomeInfo();
}

Model:

Code:

function InsertSomeInfo(){
  $this->db->trans_start();
  //insert statements
  $this->db->trans_complete();
  if ($this->db->trans_status() === FALSE)
  {
    return false;
   }
   else
   {
     return true;
   }
}

function DeleteSomeInfo(){
  $this->db->trans_start();
  //delete statements
  $this->db->trans_complete();
  if ($this->db->trans_status() === FALSE)
  {
    return false;
   }
   else
   {
     return true;
   }
}

[eluser]asciiCode[/eluser]
Thank you for your reply. That helps a little however I still have this problem.

I only want to insertSomeInfo and deleteSomeInfo if they *both* work, hence the transaction.

In your example I could insert some information and then not delete any information. This is a theoretical example I presented. It may be more realistic to just assume there are multiple inserts. So I want all of the inserts to work or none of the inserts to work.

Maybe the main problem is my model and I can give a more concrete example if needed.

Thanks for your time and help!

[eluser]sketchynix[/eluser]
The location is still going to be in the model, but if I understand you correctly, you are trying to not repeat yourself in your code. I am assuming there are times when you want to run DeleteSomeInfo() by itself without running InsertSomeInfo(), and other times when you want to run both together.

If this is the case, the way I would approach it (hopefully someone will step in if this is bad practice), is to create a third function in your model, call it InsertAndDeleteSomeInfo(), and then in the other two functions make the $this->db->trans_start(); and $this->db->trans_complete(); dependant on a variable passed in.

Code:

function InsertAndDeleteSomeInfo(){
   $this->db->trans_start();
  
   $this->InsertSomeInfo(true);
   $this->DeleteSomeInfo(true);
  
   $this->db->trans_complete();

   if ($this->db->trans_status() === FALSE)
    {
      return false;
     }
     else
     {
       return true;
     }
}
function InsertSomeInfo($trans=''){
if(empty($trans)){
    $this->db->trans_start();
  }
  
  $run_query=$this->db->query();
  
  if(empty($trans)){
      $this->db->trans_complete();

    if ($this->db->trans_status() === FALSE)
    {
      return false;
     }
     else
     {
       return true;
     }
   }
   else{
      return $run_query;
   }
}

function DeleteSomeInfo($trans=''){
  if(empty($trans)){
    $this->db->trans_start();
  }

  $run_query=$this->db->query();


  if(empty($trans)){
    $this->db->trans_complete();
    if ($this->db->trans_status() === FALSE)
    {
      return false;
     }
     else
     {
       return true;
     }
   }
   else{
      return $run_query;
   }
}

Feel free to post your code or pm me and I'll take a look...quite bored at work today

[eluser]asciiCode[/eluser]
Yes I think you understand my question correctly. But I am unsure how to properly structure the code. Your suggestion is to put all of the database transactions in the models. I agree on a theoretical point of view. However, passing $trans='' into the functions gets messy.

That is why I thought the transactions may be easier to manage in the controller. But I wanted to make sure that was the proper design decision because theoretically database stuff should be in the module not the controller.

However, if you read this post for more information you will see the last post reference "transactions scripts (controllers)" so maybe this is a common way to do things

http://ellislab.com/forums/viewthread/164839/

[eluser]bretticus[/eluser]
I am of the opinion that 99% of the time, business logic (not just database transactions) should be in your models. ie, your controllers know where to get the data and pass it to your views and that's about all they do (not always easy in practise.) The advantage is that you end up with very modular code that can be referenced from various controllers. In your case, however, I see no big issue with starting and ending your transactions in your controller methods. Especially, if it spans multiple models, etc.

However, to keep with the fat models/thin controller paradigm, you could always make an overarching model method that runs the entire transaction. Of course, this would not be suitable (it is possible) to work over multiple model calls. In that case, I'd just stick to the controller. No reason to overthink.

[eluser]asciiCode[/eluser]
Thank you for the input. What started all of this thinking on my part is that I was looking at my code. I had all of the business logic in the model and I had error message codes in the model. The controller was essentially just calling the model and passing the information through.

This didn't seem right to have my error codes defined in the model so I thought about moving the business logic out into the controller.

However, maybe this is a mistake if models should be fat and controllers should be thin?

[eluser]treeface[/eluser]
Hello all. I've got an idea that extends from the logic of sketchynix. Instead of accessing the db class from the controller or putting a lot of extra logic in each model function, we have a third option: extending the model class. Consider this file sitting in your /application/libraries/MY_Model (though I believe you will need to load it in the autoload of your config):

Code:

<?php
class MY_Model extends Model
{
    //this is a variable that will let the two extension methods know
    //whether or not to start/override the trans_start and trans_complete
    var $maintain = false;
    
    function MY_Model()
    {
        parent::Model();
    }
    
    /**
     * This is our custom trans_start()
     *
     * @param bool    $maintain
     *
     * @return void
     */
    function trans_start($maintain = FALSE)
    {
        if ($this->maintain) {
            return TRUE;
        }
        
        $this->maintain = $maintain;
        $this->db->trans_start();
    }
    
    /**
     * This is our custom trans_complete()
     *
     * @param bool    $override
     *
     * @return void
     */
    function trans_complete($override = FALSE)
    {
        if ($override) {
            $this->maintain = FALSE;
        }
        
        if ($this->maintain) {
            return TRUE;
        }
        
        $this->maintain = $maintain;
        $this->db->trans_complete();
    }
}

Now we can maintain a trans_start as long as TRUE was passed to it and subsequently TRUE isn't passed to trans_complete. Of course, you'll have to refer to it as $this->trans_start() and $this->trans_complete() instead of $this->db... So here is what our model looks like now:

Code:

class Whatever_Model extends MY_Model
{
    function insertAndDelete()
    {
        //sets $maintain to true...now must be overridden to complete trans
        $this->trans_start(true);
        $this->insertSomething();
        $this->deleteSomething();
        //overrides $maintain and completes trans
        $this->trans_complete(true);
        
        return $this->db->trans_status() !== FALSE;
    }
    
    function insertSomething()
    {
        $this->trans_start();
        //perform query
        $this->trans_complete();
        
        return $this->db->trans_status() !== FALSE;
    }
    
    function deleteSomething()
    {
        $this->trans_start();
        //perform query
        $this->trans_complete();
        
        return $this->db->trans_status() !== FALSE;
    }
}

So since we've passed TRUE into the insertAndDelete trans_start, when it encounters $this->trans_start() and $this->trans_complete() in the insert and delete functions, it will skip it and perform the query. Thoughts?