Database connection in MVC model

Question

I followed the tutorial Build a PHP MVC Application and have implemented a database connection. I made a seperate database connection class and I call it in the controller.

It works, but now I have to inject it whenever I call a method from the user model. Is this considered good (dependency injection) or bad (repeating same variable over and over again seems useless)?

I was thinking about moving the db_con method to the user model itself, but then I'd have to include it in every other future model as well, which is repeating code. Perhaps I should inject it in the first controller method:return new $model(); would become return new $model($this->db_con());? I don't know anymore. So many possibilities, but where would it fit best?

    <?php
      class Controller
      {
        private $db_con = false;

        public function model($model)
        {
          require_once '../app/models/' .$model .'.php';
          return new $model();
        }

        public function view($view, $data)
        {
          require_once '../app/views/' .$view .'.php';
        }

        public function db_con()
        {
          if($this->db_con == false)
          {
            $database = new Database();
            $this->db_con = $database->db_con;
          }

          return $this->db_con;
        }

Now, in my register controller I connect to the user model and inject the database connection like this:

<?php
  class Register extends Controller
  {
    protected $user;

    public function __construct()
    {
      $this->user = $this->model('User');
      $this->db_con = $this->db_con();
    }

    public function form()
    {
      $this->view('register/form', ['notice' => ""]);
    }

    public function submit()
    {
      //Lots of if/elses, then I connect:
      elseif($this->user->create_user($this->db_con, $_POST['username'], $_POST['password'], $_POST['email'], $_SERVER['REMOTE_ADDR']))
      {
        $this->view('register/submit', ['notice' => "You have been registered."]);
      }

And to be complete, the database connection class itself:

<?php
  class Database
  {
    public $db_con = false;
    public function __construct()
    {
      try
      {
        $this->db_con = new PDO('mysql:host='. DB_HOST .';dbname=' . DB_NAME .';charset=utf8', DB_USER, DB_PASS);
      }
      catch (PDOException $e)
      {
        error_log($e->getMessage(), 0);
      }
    }
  }
?>

Answer 1

So here are some thoughts:

• Your Database class seems to hold zero value. It provides no abstraction away from underlying PDO if that is the desired goal (which I don't see why it should be since PDO is itself an abstraction). It literally does nothing except catch and log exceptions to instantiating a PDO object. Since it doesn't abstract away things like executing queries, prepared statements, transactions, etc. any class using this class would still need to understand that it is working with an underlying PDO object, so why hide that away inside a Database object class that does absolutely nothing? It would be better, in my opinion. to have underlying classes understand clearly that they are working with PDO object.

• You are not doing dependency injection with what you currently have. If you are trying to move toward dependency injection (which is oftentimes a good strategy), then you need to actually inject the dependency to the consuming class/code, not instantiate the dependency within the class.

So for your Controller class, that might look something like this:

class Controller
  {
    // note I use null here, which is typically better than false
    // to indicate that something is missing any value whatsoever
    protected $pdo = null;

    // note that I have specified that a valid PDO object must be passed
    // to constructor.  Without this, the object would be useless.
    public function __construct(PDO $pdo) {
        $this->pdo = $pdo;
    }

    /*
    Eliminate this method as again, you should probably pass this
    dependency to the constructor.
    Definitely don't have `include` or `require` inside class methods.
    This particular method also seems to have nothing to do with the base
    controller at all. Do you need a separate model factory?
    public function model($model)
    {
      require_once '../app/models/' .$model .'.php';
      return new $model();
    }
    */

    /*
    Eliminate this function as the class should not care about the view.
    Perhaps you have another class which take a Model object as
    dependency and can render the view
    public function view($view, $data)
    {
      require_once '../app/views/' .$view .'.php';
    }
    */

    /*
    Eliminate this method altogether as we are passing PDO object
    to the constructor.  And we don't the controller class to need
    to return its database connection to any caller.  Any other class
    that needs a DB connectoin should have that DB connectoin object
    injected into it.
    public function db_con()
    {
      if($this->db_con == false)
      {
        $database = new Database();
        $this->db_con = $database->db_con;
      }

      return $this->db_con;
    }
    */

Since your code is incomplete for this class, it is hard to say if there is something else this class should be doing. To me at this point it is not really clear what this controller really does other than hold a DB connection. Perhaps you might consider implementing abstract methods here if the intent is to have inheriting classes provide implementation, though I see no signs of this from your code.

• With regards to your Register class, you would need to change the constructor to first call the parent class constructor (to load the PDO object to $this->pdo) then load user model. Also you have significant problem with working directly with user input ($_POST and $_SERVER variables) without any sort of validation. My guess is that the data cleaning validation, should happen outside this class so the class need not worry about potentially working with non-validated user input.

• Finally, I would like to point out that typically in MVC pattern, the controller can only manipulate the model, and it is the job of the view to take the model and render it. So I am not sure why your controller has any relation to the view at all. The controller should typically respond to user input and manipulate the model. Certain MVC implementations have the controller then pass the model to the view for rendering, but it seems here you are actually rendering the view with the controller, which seems a little odd (especially how you are doing it via file inclusion which seems very fragile).