Simple PDO database class in PHP

Question

I'm writing a simple database class so I could use it in my future projects. It is based on some Codeigniter database methods, but my implementation, so if you could review this, that would be cool. It is not finished yet, but I want to remove all bad code early on.

<?php


class Database {

    private static $instance = null;
    private $pdo;
    private $query;
    private $results;
    private $count = 0;
    private $error = false;

    private $query_string = "";
    private $bindValues = array();
    private $lastId;

    private function __construct() {

        try {
            // Put your database information
            $this->pdo = new PDO("mysql:host=127.0.0.1;dbname=login","root","");
        } catch (PDOException $e) {
            die($e->getMessage());
        }

    }

    public static function getInstance() {
        if (is_null(self::$instance)) {
            self::$instance = new Database();
        }
        return self::$instance;
    }

    public function query($sql, $parameters = array()) {
        $this->error = false;

        if ($this->query = $this->pdo->prepare($sql)) {
            $i = 1;
            foreach ($parameters as $param) {
                $this->query->bindValue($i, $param);
                $i++;
            }
            if ($this->query->execute()) {
                // You can PDO::FETCH_OBJ instad of assoc, or whatever you like
                $this->results = $this->query->fetchAll(PDO::FETCH_ASSOC);
                $this->count = $this->query->rowCount();
                $this->lastId = $this->query->lastInsertId();
            } else {
                $this->error = true;
            }
        }
        return $this;
    }


    public function select($fields = "*") {
        $action = "";
    $this->query_string = "";
        if (is_array($fields)) {
            $action = "SELECT ";
            for ($i = 0; $i < count($fields); $i++) {
                $action .= $fields[$i];
                if ($i != count($fields) - 1)
                    $action .= ', ';
            }
        } else {
            $action = "SELECT * ";
        }
        $this->query_string .= $action;
        return $this;
    }

    public function from($table) {
        $this->query_string .= " FROM {$table} ";
        return $this;
    }

    public function where($where = array()) {
        $keys = array_keys($where);
        $action = " WHERE ";
        for ($i = 0; $i < count($keys); $i++) {
            $action .= $keys[$i] . ' = ?';
            if ($i < count($keys) - 1)
                $action .= ' AND ';
            $this->bindValues[] = $where[$keys[$i]];
        }
        $this->query_string .= $action;
        return $this;
    }

    public function execute() {
        if (!empty($this->query_string))
            $this->query($this->query_string, $this->bindValues);
        $this->bindValues = array();
    }

    public function getQueryString() {
        return $this->query_string;
    }

    public function results() {
        return $this->results;
    }
    public function first() {
        return $this->results[0];
    }
    public function last() {
        return $this->results[$this->count-1];
    }
    public function row($id) {
        return $this->results[$id];
    }
    public function error() {
        return $this->error();
    }
    public function count() {
        return $this->count;
    }
    public function lastId() {
        return $this->lastId;
    }
}

Answer 1

My suggestion is to not do what you are trying to do. Why do you want to wrap PDO?

This is a common type of code review that I see here and in most cases the attempt to "wrap" a perfectly good abstraction library like PDO is misguided.

Please look at the discussion on these threads:

Class for reducing development time PHP PDO Wrapper

Given that, let me give you some thoughts on what you do have.

Don't mix snake case and camel case in your codebase. I know PHP is pretty bad about mixing these two, but that doesn't mean you should be.

---

• You have a class that is doing two things - acting as a singleton and acting like a DB wrapper. If you truly want to move forward with having a wrapper, you should at least consider moving the singleton behavior into it's own class, with the singleton managing the creation or retrieval of the Database object (or PDO object if you drop the wrapper idea). Name that class appropriately like DatabaseSingleton, PDOSingleton, or similar.

---

private function __construct() {

    try {
        // Put your database information
        $this->pdo = new PDO("mysql:host=127.0.0.1;dbname=login","root","");
    } catch (PDOException $e) {
        die($e->getMessage());
    }

}

• Don't just assume that PDO instantiation works. You could be assigning a bad value to $this->pdo.
• Don't put your DB credentials here in this class. They should be in app config.
• Don't use "root" for this application. Create a user in MySQL for this application, with appropriate permissions. Don't use "" as your password, especially in combination with "root". Your MySQL server could be vulnerable.
• Don't echo out application error messages to standard output.
• You try-catch block is essentially useless here as it is not really doing anything and is, in fact, hiding the error from calling code which is likely in better position to determine how to recover from the failure and/or message the end user.

---

public function query($sql, $parameters = array()) {

• This method is poorly named. Perhaps something like prepare_and_execute is more descriptive of what is happening here.
• You are performing no validation whatsoever for the data passed. You need to verify that $sql is (at a minimum) a non-zero length string and that you have an array passed for $parameters. Your code could break if those conditions aren't met, so you need to fail out of the method (possibly with INvalidArgumentException being thrown) before you ever get to the point of trying to prepare a statement. You should have parameter type-hinting and/or validation as the first thing that happens in ANY publicly accessible method.
• This method has removed user's ability to do any parameter type enforcement in their bindings.
• This method rally only considers happy path. What if prepare fails (right now you silently just return the object with no indication to caller that something went wrong). Same with bindValue and execute(). To reinforce the point about pointlessness of this wrapper. You have now written a number of lines of code here to make working with PDO less flexible, more fragile, and less transparent to the caller.

---

        $this->results = $this->query->fetchAll(PDO::FETCH_ASSOC);
        $this->count = $this->query->rowCount();
        $this->lastId = $this->query->lastInsertId();

This is just odd. In what scenario would a single query have both a result set and an insert ID?

---

I know that people may like the natural language interfaces like you are trying to achieve with select(), where(), etc. methods. There are a number of good libraries out there that can give you this functionality, and I guarantee you those methods in those libraries contain a lot more code in them to address all the data validation and edge cases you are missing (like what if I want to OR my WHERE filters). How much benefit are you really getting from this functionality?

Is this:

$result = $database->select('*')->from('table')->where(["column = 'abc'"])

Really easier to read and understand than:

$sql = "SELECT * FROM table WHERE column = 'abc'";

What happens when you need to perform join, order by, limit, etc.? With your current class, you just end up having to revert to using raw SQL anyway because your current implementation is very basic in terms of functionality.

By the way defaulting to (or even allowing) SELECT * is something you ought to think twice about. SELECT * is bad. SELECT * can quietly break your applications when there are schema changes. SELECT * can suck up a lot more bandwidth in transmission than is required for any given piece of logic to execute. SELECT * makes it difficult for someone only looking at code to understand what sort of data is being made available to them.

---

The last(), first(), row() methods are ill-conceived. These concerns should really be addressed by LIMIT clauses in SQL. Why do you need to retrieve and store every row from a table if you are only interested in row X?

Answer 2

A bit about SOLID principles after Mike Brant answer.

First of all, Single Responsibility Principle.

If you had to define your class, that's a :

1. Singleton
2. Wrapping PDO
3. Storing Credentials
4. Able to execute SQL queries
5. Able to build SQL queries

5 responsibilites ! That means, if you ever want to change one of these features, you'll have to change this class. That's already a god class you got there. You should split all of these into smaller, simpler classes. This will encourage reusability and reduce redundancy through your code.

---

Second of all, Dependency Inversion Principle.

$this->pdo = new PDO("mysql:host=127.0.0.1;dbname=login","root","");

This line binds the whole class to the implementation PDO. It means it depends on it. Client using the Database class should be able to pick the implementation they wish to collaborate with Database. This is not Database business to choose which implementation should be used, its only business should be how the interface of the chosen implementation should be used.

The Open Closed Principle is not even in question here, until the first point is fixed. That's because while it's available for extension (open) it's absolutely not defined by a concise interface (closed).

The Liskov Substitution Principle can't really be reviewed without any inheritance.

As for the Interface Segregation Principle...well, go back to first point, define proper interfaces and this will flow in easily.