PHP/MySQL - OOP database class

Question

I use this database class in a CMS, just a project to learn OOP. Would like to have some review on how to improve it, what should I change, add, remove?

<?php namespace Core; if ( !class_exists( 'DB' ) ) { class DB { private $cxn; public function __construct() { $this->connect(); } private function connect() { $this->cxn = new \mysqli( DB_HOST, DB_USER, DB_PASS, DB_NAME ); if ( $this->cxn->connect_error ) { die( "Connection failed: " . $this->cxn->connect_errno . ' ' . $this->cxn->connect_error ); } } private function create_db( $name ) { $sql = "CREATE DATABASE IF NOT EXISTS $name"; if( !$this->cxn->query( $sql ) ){ die( "Error creating database: " . $this->cxn->errno . ' ' . $this->cxn->error ); } } private function select_db( $name ) { if( !$this->cxn->select_db( $name ) ){ die( "Error selecting database: " . $this->cxn->errno . ' ' . $this->cxn->error ); } } /** * Create a new table in the db with the specified columns * @param array $tables */ public function create_tables( $tables ) { $sql = ''; if ( !is_array( $tables ) ) $tables = array(); foreach ( $tables as $name => $columns ) { $sql .= "CREATE TABLE IF NOT EXISTS $name ($columns);"; } if( !$this->cxn->multi_query( $sql ) ){ die( "Error creating table: " . $this->cxn->errno .' '. $this->cxn->error ); } } /** * Insert a row in table * @param string $table * @param array $data * @param array $format * @return boolean */ public function insert( $table, $data, $format ) { if ( empty($table) || empty($data) ) { return false; } //cast $data and $format to array $data = (array) $data; $format = (array) $format; //Build format string $format = $this->build_format($format); //prepare data list($fields, $placeholders, $values) = $this->prep_query($data); //prepend $format in to $values array_unshift($values, $format); //prepare statements if ( !( $stmt = $this->cxn->prepare("INSERT INTO {$table} ({$fields}) VALUES ({$placeholders})") ) ) { echo "Error preparating the query: (" . $this->cxn->errno . ") " . $this->cxn->error; } //Dinamically binding if ( !call_user_func_array(array($stmt, 'bind_param'), $this->ref_values($values)) ) { echo "Error binding parameters: (" . $this->cxn->errno . ") " . $this->cxn->error; } //execute the query if (!$stmt->execute()) { echo "Error executing the insert query: (" . $this->cxn->errno . ") " . $this->cxn->error; } //Check for succesful insertion if ( $stmt->affected_rows ) { return $stmt->insert_id; } return false; } /** * Update a row in a table * @param string $table * @param array $data * @param string $format * @param array $where * @param string $where_format * @return boolean */ public function update( $table, $data, $format, $where, $where_format ) { if ( empty($table) || empty($data) ) { return false; } //cast to array $data = (array) $data; $format = (array) $format; $where_format = (array) $where_format; //Build format string $format = $this->build_format($format); $where_format = $this->build_format($where_format); $format .= $where_format; //prepare data list($fields, $placeholders, $values) = $this->prep_query($data, 'update'); list($where_clause, $where_values) = $this->prep_where($where); //prepend $format onto $values array_unshift($values, $format); $values = array_merge($values, $where_values); //prepare statements if ( !( $stmt = $this->cxn->prepare("UPDATE {$table} SET {$placeholders} WHERE ({$where_clause})") ) ) { echo "Error preparating the update query: (" . $this->cxn->errno . ") " . $this->cxn->error; } //bind params if ( !call_user_func_array(array($stmt, 'bind_param'), $this->ref_values($values)) ) { echo "Error binding parameters: (" . $this->cxn->errno . ") " . $this->cxn->error; } //execute if (!$stmt->execute()) { echo "Error executing the query: (" . $this->cxn->errno . ") " . $this->cxn->error; } //Check for succesful insertion if ( $stmt->affected_rows ) { return true; } return false; } /** * Delete a row from a table * @param string $table * @param string|array $where * @param string|array $where_format * @return false */ public function delete( $table, $where = '', $where_format = '' ) { if ( !is_array( $where ) ) { $where = array( 'ID' => $where ); $where_format = 'i'; } $where_format = (array) $where_format; $where_format = $this->build_format($where_format); //prepare data list($where_clause, $where_values) = $this->prep_where($where); //prepend $format onto $values $values = $where_values; array_unshift($values, $where_format); //prepare statements if ( !( $stmt = $this->cxn->prepare("DELETE FROM {$table} WHERE {$where_clause}") ) ) { echo "Error preparating the delete query: (" . $this->cxn->errno . ") " . $this->cxn->error; } //bind params if ( !call_user_func_array(array($stmt, 'bind_param'), $this->ref_values($values)) ) { echo "Error binding parameters: (" . $this->cxn->errno . ") " . $this->cxn->error; } //execute if (!$stmt->execute()) { echo "Error executing the query: (" . $this->cxn->errno . ") " . $this->cxn->error; } //Check for succesful insertion if ( $stmt->affected_rows ) { return true; } return false; } /** * Select a row from a table * @param string $table * @param string $where * @param string $where_format * @return array */ public function select( $table, $where = '', $where_format = '' ) { if ( !is_array( $where ) ) { $where = array( 'ID' => $where ); $where_format = 'i'; } $where_format = (array) $where_format; $where_format = $this->build_format($where_format); //prepare data list($where_clause, $where_values) = $this->prep_where($where); //prepend $format onto $values $values = $where_values; array_unshift($values, $where_format); //prepare statements if ( !( $stmt = $this->cxn->prepare("SELECT * FROM {$table} WHERE {$where_clause}") ) ) { echo "Error preparating the query: (" . $this->cxn->errno . ") " . $this->cxn->error; } //bind params if ( !call_user_func_array(array($stmt, 'bind_param'), $this->ref_values($values)) ) { echo "Error binding parameters: (" . $this->cxn->errno . ") " . $this->cxn->error; } //execute if (!$stmt->execute()) { echo "Error executing the query: (" . $this->cxn->errno . ") " . $this->cxn->error; } $results = $this->get_results($stmt); if ( $results ) { return $results; } else { throw new \Exception('Invalid query, no results founds.'); } } /** * Select multiple row from a table * @param string $table * @param string $where * @param string $where_format * @return array */ public function select_array( $table, $where = '', $where_format = '' ) { if ( !is_array( $where ) ) { $where = array( 'ID' => $where ); $where_format = 'i'; } $where_format = (array) $where_format; $where_format = $this->build_format($where_format); //prepare data list($where_clause, $where_values) = $this->prep_where($where); //prepend $format onto $values $values = $where_values; array_unshift($values, $where_format); //prepare statements if ( !( $stmt = $this->cxn->prepare("SELECT * FROM {$table} WHERE {$where_clause}") ) ) { echo "Error preparating the query: (" . $this->cxn->errno . ") " . $this->cxn->error; } //bind params if ( !call_user_func_array(array($stmt, 'bind_param'), $this->ref_values($values)) ) { echo "Error binding parameters: (" . $this->cxn->errno . ") " . $this->cxn->error; } //execute if (!$stmt->execute()) { echo "Error executing the query: (" . $this->cxn->errno . ") " . $this->cxn->error; } $results = $this->get_results($stmt, 'array'); if ( $results ) { return $results; } else { throw new \Exception('Invalid query, no results founds.'); } } /** * Select all the rows from a table * @param string $table * @return array */ public function select_all( $table ) { //prepare statements if ( !( $stmt = $this->cxn->prepare("SELECT * FROM {$table}") ) ) { echo "Error preparating the query: (" . $this->cxn->errno . ") " . $this->cxn->error; } //execute if (!$stmt->execute()) { echo "Error executing the query: (" . $this->cxn->errno . ") " . $this->cxn->error; } $results = $this->get_results($stmt, 'array'); if ( $results ) { return $results; } else { throw new \Exception('Invalid query, no results founds.'); } } /** * Get results from a query * @param object $stmt * @param string $type * @return array */ private function get_results($stmt, $type = 'string') { $stmt->store_result(); $meta = $stmt->result_metadata(); while ( $field = $meta->fetch_field() ) { $params[] = &$row[$field->name]; } call_user_func_array( array( $stmt, 'bind_result' ), $params ); $results = array(); while ( $stmt->fetch() ) { foreach( $row as $key => $val ) { $c[$key] = $val; } if ($type === 'array') { $results[] = $c; } else { $results = $c; } } if ( !empty( $results) ) return $results; return false; } /** * Build the format string for the query values * @param array $format * @return string */ private function build_format( $format ) { $format = implode('', $format); $format = str_replace('%', '', $format); return $format; } /** * Prepare data for a query * @param array $data * @param string $type * @return array */ private function prep_query($data, $type = 'insert') { //instantiate $fields and $placeholders for looping $fields = ''; $placeholders = ''; $values = array(); //loop through $data and build $fields, $placeholders and $values foreach ( $data as $field => $value ) { $fields .= "{$field},"; $values[] = $value; if ( $type == 'update' ) { $placeholders .= $field . '=?,'; } else { $placeholders .= '?,'; } } //normalize $fields and $placeholder for inserting $fields = substr( $fields, 0, -1 ); $placeholders = substr( $placeholders, 0, -1 ); return array( $fields, $placeholders, $values ); } /** * Prepare where data for a query * @param array $where * @return array */ private function prep_where($where) { $where_clause = ''; $where_values = array(); $count = 0; foreach ($where as $field => $value) { if ( $count > 0 ) { $where_clause .= ' AND '; } $where_clause .= $field . '=?'; $where_values[] = $value; $count++; } return array($where_clause, $where_values); } /** * Create references for query values * @param array $array * @return array */ private function ref_values( $array ) { $refs = array(); foreach ( $array as $key => $value ) { $refs[$key] = &$array[$key]; } return $refs; } /** * Hash a password * @param string $password * @param string $nonce * @return string */ public function hash_password($password, $nonce) { $secureHash = hash_hmac('sha512', $password . $nonce, SITE_KEY); return $secureHash; } /** * Close the connection to database */ private function disconnect() { if ( !$this->cxn->close() ) { die('Can\'t close the connection'); } } } } ?> 

Answer 1

Setting yourself up for sql injection

You seem to expect the format of $tables to be Array( "key" => "a type, b type, c type, d type" ). That's... odd. One should either predefine a long string of types and column names, or one chooses to use a more abstract type and converts it before use with your function.

You never check if the passed parameters are sensible. In fact, you seem to rely on die( "..." );, which is great to stop a script, but is printing debug information to the screen...

You are using mysqli::multi_query, which allows for execution of any amount of queries.

You probably get the point now; Due to how this function is setup, it is hard to figure out if the passed variables are sensible to use. If an attacker is able to pass anything but valid sql to this function, they can do anything, including uploading files, printing root passwords right to the screen, perform timing attacks. It relies on external code to be safe, which is a bad assumption.

Prepared queries

(Looking at insert(..)) You are using prepared queries... but sql injection is still possible if you insert variables directly, instead of binding ? to a variable. What you insert directly seems to depend greatly on prep_query(..), but this function does not sanitize field names. In effect, one can do sql injection by altering field names.

Also $table is not sanitized.

Strange correction mechanisms

In insert(..) you cast two variables to an array. Why?! This is a database interface class. You want to be able to use more convenient ways to interact with the database, but allowing any string, int, boolean, null or array to be passed makes reading the code more difficult. You already return early when NULL or Array() is passed. Add an extra check to see if the parameters have the right format. If not, return false, or terminate the script completely, so that you know that you should fix a script that uses this class.

In select(..) you change the where-clause based on if it is an array. I recommend against this. Do yourself a favour and cause the script to terminate on invalid input, just so your other scripts are consistent. It is easier to maintain that way.

prep_where(..) and prep_query(..)

You are building a string here where each of the elements is concatenated with some kind of delimiter. Instead of using a somewhat confusing construction with a counter and custom "AND" insertion or truncating the resulting string, just use an Array, then implode the Array:

private function prep_where($where) { $where_clause = array(); $where_values = array(); foreach ($where as $field => $value) { $where_clause[] = $field . '=?'; $where_values[] = $value; } $where_clause = implode( $where_clause, " AND " ); return array($where_clause, $where_values); } private function prep_query($data, $type = 'insert') { if(!in_array($type, array('insert', 'update'), TRUE)) { die( "Incorrect usage of prep_query" ); } //instantiate $fields and $placeholders for looping $fields = array(); $placeholders = array(); $values = array(); //loop through $data and build $fields, $placeholders and $values foreach( $data as $field => $value ) { $fields[] = $field; $values[] = $value; if ( $type == 'update' ) { $placeholders[] = "{$field}=?"; } else { $placeholders[] = "?"; } } $fields = implode( $fields, "," ); $placeholders = implode( $placeholders, "," ); return array( $fields, $placeholders, $values ); } 

General remarks

For a while now, there is no real performance difference between using double quoted strings and single quoted strings. There are, of course, differences between both kind of strings, so you are free to use them as you please.

You are building strings in various ways.

"INSERT INTO {$table} ({$fields}) VALUES ({$placeholders})" //or... "Error preparating the query: (" . $this->cxn->errno . ") " . $this->cxn->error //or... "CREATE TABLE IF NOT EXISTS $name ($columns);" 

I would recommend always using the bracket notation. Consistency makes code easier to read; in particular for concatenated strings it makes it far more readable what the general structure of the string is.

"INSERT INTO {$table} ({$fields}) VALUES ({$placeholders})" //or... "Error preparating the query: ({$this->cxn->errno}) {$this->cxn->error}" //or... "CREATE TABLE IF NOT EXISTS {$name} ({$columns});" 

Speaking of consistency, either always use brackets or don't use brackets at all when using an if-statement with just one line of code in it:

if ( !is_array( $tables ) ) $tables = array(); //vs if( !$this->cxn->multi_query( $sql ) ){ die( "Error creating table: " . $this->cxn->errno .' '. $this->cxn->error ); } 

Take a look at the consistency of your whitespace. In particular between function names and brackets, and between various kinds of brackets.

PHP allows you to return early with an empty return statement in functions that did not return anything anyway. Use this to your advantage. In the if-statement above you set an empty array if it isn't an array, which will execute a multiquery sql-statement of the empty string... Instead just return early and skip all that nonsense. Similarly, if for some reason the construction of a prepared statement fails, why continue binding parameters. We already know a step failed; return early, or return false wherever that is more appropriate.