Connect to MYSQL database in PHP with prepared statements

Question

I have recently learned about using the functions exposed by the PDO extension. I created a db class which can handle common actions like create, update, delete and select. Is this method a correct way to connect to the database using prepared statements?

<?php class Db { private $db_host = DB_HOST; private $db_user = DB_USERNAME; private $db_pass = DB_PASSWORD; private $db_port = DB_PORT; private $db_name = DB_DATABASE; private $db_charset = DB_CHARSET; private $dsn = ""; private $pdo = null; private $stmt = null; private $result = array(); private $conn = false; private $options = [ PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, PDO::ATTR_EMULATE_PREPARES => false, ]; public function __construct() { if(!$this->conn) { try { $this->dsn = "mysql:host=" . $this->db_host . ";dbname=" . $this->db_name . ";charset=" . $this->db_charset . ";port=" . $this->db_port; $this->pdo = new PDO($this->dsn, $this->db_user, $this->db_pass, $this->options); return $this->pdo; } catch (PDOException $e) { $err = "Connection Failed: " . $e->getMessage(); } } return false; } protected function select($table, $rows = "*", $join = null, $join_side = null, $where = array(), $order = null, $limit = null) { if($this->tableExists($table)) { $sql = "SELECT $rows FROM $table"; $paramsArray_where = null; if($join != null && $join_side != null) $sql .= " $join_side $join"; if(!empty($where)) { $table_val_where = ":" . implode(',:', array_keys($where)); // Create an array of new keys that contain : in front of them $table_key_where = explode(",", $table_val_where); // get array values $values_where = array_values($where); // combine key with their respective values $paramsArray_where = array_combine($table_key_where, $values_where); $args = array(); foreach($where as $key=>$value) $args[] = "$key=:$key"; $sql .= " WHERE " . implode(' && ', $args); } if($order != null) $sql .= " ORDER BY $order"; if($limit != null) $sql .= " LIMIT $limit"; $this->stmt = $this->pdo->prepare($sql); if($this->stmt->execute($paramsArray_where)) { $this->result = $this->stmt->fetchAll(); return true; } } return false; } protected function insert($table, $params=array()) { if(!empty($params)) { if($this->tableExists($table)) { // Seperating $params key and values $table_cols = implode(',', array_keys($params)); $table_val = ":" . implode(',:', array_keys($params)); // Create an array of new keys that contain : in front of them $table_key = explode(",", $table_val); // get array values $values = array_values($params); // combine key with their respective values $paramsArray = array_combine($table_key, $values); $sql = "INSERT INTO $table ($table_cols) VALUES ($table_val)"; $this->stmt = $this->pdo->prepare($sql); if($this->stmt->execute($paramsArray)) return true; } } return false; } protected function update($table, $params=array(), $where=array()) { if(!empty($params)) { if($this->tableExists($table)) { $table_val = ":" . implode(',:', array_keys($params)); // Create an array of new keys that contain : in front of them $table_key = explode(",", $table_val); // get array values $values = array_values($params); // combine key with their respective values $paramsArray = array_combine($table_key, $values); $args = array(); foreach($params as $key=>$value) $args[] = "$key=:$key"; $sql = "UPDATE $table SET " . implode(', ', $args); if(!empty($where)) { $table_val_where = ":" . implode(',:', array_keys($where)); // Create an array of new keys that contain : in front of them $table_key_where = explode(",", $table_val_where); // get array values $values_where = array_values($where); // combine key with their respective values $paramsArray_where = array_combine($table_key_where, $values_where); $bind_params = array_merge($paramsArray, $paramsArray_where); $args = array(); foreach($where as $key=>$value) $args[] = "$key=:$key"; $sql .= " WHERE " . implode(' && ', $args); }else{ $bind_params = $paramsArray; } $this->stmt = $this->pdo->prepare($sql); if($this->stmt->execute($bind_params)) return true; } } return false; } protected function delete($table, $where = array()) { if($this->tableExists($table)) { $sql = "DELETE FROM $table"; $paramsArray_where = null; if(!empty($where)) { $table_val_where = ":" . implode(',:', array_keys($where)); // Create an array of new keys that contain : in front of them $table_key_where = explode(",", $table_val_where); // get array values $values_where = array_values($where); // combine key with their respective values $paramsArray_where = array_combine($table_key_where, $values_where); $args = array(); foreach($where as $key=>$value) $args[] = "$key=:$key"; $sql .= " WHERE " . implode(' && ', $args); } $this->stmt = $this->pdo->prepare($sql); if($this->stmt->execute($paramsArray_where)) return true; } return false; } private function tableExists($table) { $sql = "SHOW TABLES FROM " . $this->db_name . " LIKE '$table'"; $this->stmt = $this->pdo->query($sql); if($this->stmt->execute()) { if($this->stmt->rowCount() > 0) return true; } return false; } public function getResult() { $result = $this->result; $this->result = array(); return $result; } public function __destruct() { if($this->conn) { $this->conn = false; $this->pdo = null; $this->dsn = ""; $this->stmt = null; $this->result = array(); return true; }else{ return false; } } } 

I have tested the above code and it works fine.

Answer 1

Sam's answer covers a lot of good points I am not going to repeat, but I will tell you more issues.

Private properties

What is the point in setting constants as private properties? What is the purpose of having all these config values as private properties if they are only used in the constructor? These values should come from a config file that is stored outside of VCS.

Catching PDO exception

You copied some code without understanding why. You catch an exception and do nothing with it, as a result shooting yourself in the leg. Either remove the try-catch entirely or throw a new exception in the catch block. See https://phpdelusions.net/pdo#catch

Use strict types

Use any types at all! All your parameters and return values should have types specified. If you are not doing this, I guarantee you, at some point you will pass invalid value to one of your methods.

Use early returns.

Instead of wrapping 20 lines in an if block, use the opposite of that condition and return. For example:

if (!$this->tableExists($table)) { return false; } 

Avoid empty()

Usage of empty() is an antipattern. You can use it if you think the variable might be empty or undefined but when you know the variable is defined you don't need it. Change if (!empty($where)) to if ($where).

Don't check for the return value of PDOStatement::execute()

What do you expect to receive from this method? You set PDO to exception mode so that function should never return false. That if statement is useless.

Usage of rowCount()

Generally, you should never need to use this method. Especially you do not need if ($this->stmt->rowCount() > 0). 0 is false-ish and rowCount() should never return negative numbers. In this case, however, you can keep this method call, but reduce the whole return statement to a single line.

{ $sql = "SHOW TABLES FROM " . $this->db_name . " LIKE '$table'"; return $this->pdo->query($sql)->rowCount() > 0; } 

Destructor

Why, oh why? You are destroying the object and all your properties are private. Unless you go out of your way to leak one of these variables, that destructor is pointless. You are destroying all of this anyway when the object is destroyed!

Answer 2

Canonical advice from a helpful post is likely useful

As was mentioned in a comment there is a post with helpful advice for classes like this: Your first database wrapper's childhood diseases (written by @YourCommonSense). It recommends setting the exception mode to PDO::ERRMODE_EXCEPTION which appears to be the case with the code here. It also recommends:

1. Tell PHP to report ALL errors:

   error_reporting(E_ALL); 

2. On a development server just turn displaying errors on:

   ini_set('display_errors', 1); 

3. On a production server turn displaying errors off while logging errors on:

   ini_set('display_errors', 0); ini_set('log_errors', 1); 

¹

Additionally, it recommends making the PDO connection once and only once per PHP request. The code above appears to attempt to only connect once but that is flawed - see the section on Constructor Logic below for more on that topic.

SQL injection is likely possible

As I mentioned in this review of a similar wrapper class there is potential for SQL injection - likely from methods like insert() and update(). That article from phpdelusions.net has an entire post about how many update and insert methods are vulnerable to SQL injection.

"placeholders are used and data is safely bound, therefore our query is safe". Yes, the data is safe. But in fact, what does this code do is taking user input and adding it directly to the query. Yes, it's a $key variable. Which goes right into your query untreated.

Which means you've got an injection. ²

While it is up to the calling code to provide fields to update, where conditions etc. this code cannot prevent calling code from passing input straight from the client side. As the article explains later on that one can attempt to make a whitelist of fields that can be updated but for this central code that doesn’t seem feasible. However using backtick delimiters will help but then delimiters inside the name need to be escaped

As it was said before, escaping delimiters would help. Therefore, your first level of defense should be escaping delimiters (backticks) by doubling them. Assembling your query this way,

$setStr .= "`".str_replace("`", "``", $key)."` = :".$key.","; 

you will get the injection eliminated. ²

Constructors typically lack a return statement

As was mentioned in a comment constructors typically don't have a return statement, though it is valid.

It appears that the $conn property is not assigned in the constructor (only in the destructor), but that property doesn't seem to be doing much. The default value is false so this conditional in the constructor will always evaluate to true:

if(!$this->conn) { 

It might make more sense to check if $this->pdo is not null but in the constructor that seems pointless since the object is just being created.

Method visibility could be changed

It appears that the only public methods are the constructor, destructor and getResult(). Perhaps the intention is that subclasses should be defined in order to utilize the core CRUD methods but if that is not the case then consider making those methods have public visibility.

Conditionals can be combined in one line

In method tableExists() the following lines exist towards the end:

if($this->stmt->execute()) { if($this->stmt->rowCount() > 0) return true; } 

Those conditionals could be combined into a single line:

if($this->stmt->execute() && $this->stmt->rowCount() > 0) { return true; } 

Addition of where conditions can be abstracted

A commonly accepted principle is the Don't Repeat Yourself principle. There are places where blocks of code are repeated - e.g. the following block appears to be repeated within select(), update(), delete().

 $args = array(); foreach($where as $key=>$value) $args[] = "$key=:$key"; $sql .= " WHERE " . implode(' && ', $args); 

It would be simpler to abstract that logic into a method instead of having it exist multiple times.

Because it is a loop that pushes into an array, it can be simplified using array_map():

 $args = array_map(function($key) { return "$key = :{$key}"; }, array_keys($where)); $sql .= "WHERE " . implode(" && ", $args); 

And hopefully the server is running PHP 7.4 or later since currently there is LTS for version 7.4+, arrow functions can be used to simplify the mapping:

 $args = array_map(fn($key) => "$key = :{$key}", array_keys($where)); $sql .= "WHERE " . implode(" && ", $args); 

To utilize the advice from the article on phpdelusions.net a method could be created to use delimeters on the field name and escape them within the name:

public static function sanitizeNameCondition($name) { return "`" . str_replace("`", "``", $name) . "` = :" . $name; } 

Then that can be used with array_map():

$args = array_map([$this, 'sanitizeNameCondition'], array_keys($where)); $sql .= " WHERE " . implode(" && ", $args); 

And that method can be used for the fields to set also:

$args = array_map([$this, 'sanitizeNameCondition'], array_keys($params)); $sql = "UPDATE $table SET " . implode(', ', $args);