Simple PDO database class in PHP

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.

```
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 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 bindValues[] = $where[$keys[$i]];
}
$this->query_string .= $action;
return $this;
}

public function execute() {
if (!empty($this->query_st

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 ever

StackExchange Code Review Q#142148, answer score: 4