PHP MVC Best Practice - Pass Session variable to model class from controller or access directly in model

Question

Our development team is debating a general best practice: Is it better to access a session variable directly from a function in a model class or pass the session variable from the controller as an argument to a function in the model class. Look at the two examples below:

Accessing session variable directly from the model class to use in a query:

class MyModel {
    public function getUserPrefs($userID) {
        $this->query("SELECT * FROM my_table WHERE id=$_SESSION['userID']");
    }
}

Or pass the session variable from the controller to a function in the model class as a function argument:

class MyController {
    public function displayUsers() {
        $this->model->getUserPrefs($_SESSION['userID']);
    }
}

class MyModel {
    public function getUserPrefs($userID) {
        $this->query("SELECT * FROM my_table WHERE id=$userID");
    }
}

The reasoning for passing it to the model from the controller is so all data that is referenced is coming from one point of entry, that being the controller.

What is recognized as a better practice?

Answer 1

The second version (passing $_SESSION['userId'] as an argument to the method) results in a more decoupled class, and therefore more flexible. Go with it.

Answer 2

You NEVER want to have session variables in your model. You should always pass these variables as parameters to the function in the model. This also makes your code more expandable and flexible. Consider a model that gets a user by their id. You may write a function like:

function find_by_id() {
  // SELECT * FROM Users WHERE user_id = $_SESSION['user_id'];
}

However, what if you now what to build an admin functionality with a user lookup feature? Your model is hardcoded to use the session's user_id, but you want to be able to pass your own id. You would be better off:

function find_by_id($id) {
  // SELECT * FROM Users WHERE user_id = $id
}

and in your controller

$user = Model::find_by_id(1);
//or
$user = Model::find_by_id($_SESSION['user_id']);
//etc

In this case, however, I would really consider making your code even MORE flexible:

function find($ids) {
  // this is pseudo code, but you get the idea
  if(is_array($ids))
    $ids = implode(',', $ids); // if an array of ids was passed, implode them with commas
  SELECT * FROM Users WHERE user_id IN ($ids);
}

This allows you to get multiple users in ONE query! Which is way more efficient. Then, in your view:

foreach($users as $user){
  // iterate over each user and do stuff
}

You should also consider using a singelton class for a User to limit database load. Create a non-changing instance class called CurrentUser (for example) like:

class CurrentUser {

  private static $user;

  // we never instantiate it -its singleton
  private function __construct() {}

  public function user() {
    return self::$user;
  }

}

This is a really basic example of a singleton class and is missing a lot of stuff. If you want to know more about singleton classes, post another question.