DB Class extending PDO w/ other useful functions

Question

I have this DB class that extends PDO, that also has useful functions for certain tasks; I was wondering how well it's coded, and if so, what I could maybe improve on. I tried commenting in PHPDoc to explain each functions usage, but if any are confusing, don't hesitate to let me know so I can try and explain it a bit better.

I am really just looking for advice on where I can improve, and if not already, how to make the class more OOP.

<?php
    /**
     * Connect into a specified database
     *
     * Connect into a database using/extending PDO. Must set 
     * HOST, USER, & PASS credentials in order for it to properly
     * work.
     * 
     * @param string $DB_NAME database name to connect into.
     */
    class DB extends PDO{
        public object $connect;
        public bool $connected;
        public string $latest_error_message;

        private const HOST = "hostname";
        private const USER = "username";
        private const PASS = "password";

        public function __construct(string $DB_NAME){
            try{
                $this->connect = new \PDO("mysql:host=".self::HOST.";dbname=".$DB_NAME, self::USER, self::PASS);
                $this->connect->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
                $this->connected = true;
            }catch(PDOException $e){
                $this->connected = false;
                $this->latest_error_message = $e->getMessage();
            }
        }

        /**
         * Inserts an array into the specified table as a row using column=>value pairs
         *
         * Using key=>value pairs as column=>value pairs, insert each value of the
         * array into its coresponding column based on the key name. 
         * 
         * (e.g.) If you have a table called "data", with 2 columns inside named "id" & "name",
         * and want to add a row with the values "567" & "Crimin4L" respectively; you can execute 
         * the following:
         * 
         * DB::insert_row( "data", ["id"=>"567", "name"=>"Crimin4L"] );
         * 
         * @param string $table table name 
         * @param array $column_value_pairs associative arrays with key=>value pairs as column=>value pairs
         * @return bool 
         * @throws PDOException 
         */
        public function insert_row(string $table, array $column_value_pairs){
            try{
                $qhalf1 = "INSERT INTO `${table}` (";
                $qhalf2 = ") VALUES (";
                foreach($column_value_pairs as $column => $v): $qhalf1 .= "${column}, "; $qhalf2 .= ":${column}, "; endforeach;
                $query = substr($qhalf1, 0, strlen($qhalf1) - 2).substr($qhalf2, 0, strlen($qhalf2) - 2).")";
                $prep = $this->connect->prepare($query);
                foreach($column_value_pairs as $column => $value){
                    if(is_string($value)) $prep->bindValue(":${column}", $value, PDO::PARAM_STR);
                    elseif(is_int($value)) $prep->bindValue(":${column}", $value, PDO::PARAM_INT);
                    elseif(is_bool($value)) $prep->bindValue(":${column}", $value, PDO::PARAM_BOOL);
                }
                $prep->execute();
                if($prep->rowCount() > 0) return true; 
                else return false;
            }catch(PDOException $e){
                $this->latest_error_message = $e->getMessage();
                return false; 
            }
        }

        /**
         * Executes a quick query for backend purposes (Unsecure)
         * 
         * @param string $query
         * @return object|bool
         * @throws PDOException 
         */
        public function exec_query(string $query){
            try{
                $qstart = $this->connect->prepare($query);
                $qstart->execute();
                return $qstart;
            }catch(PDOException $e){
                $this->latest_error_message = $e->getMessage();
                return false;
            }
        }

        /**
         * Check if the specified table exists
         * 
         * @param string $table
         * @return bool
         * @throws PDOException 
         */
        public function is_table(string $table){
            try{
                $qstart = $this->connect->prepare("DESCRIBE `${table}`");
                if($qstart->execute()) return true;
                else return false;
            }catch(PDOException $e){
                $this->latest_error_message = $e->getMessage();
                return false;
            }
        }

        /**
         * Drop/Delete specified table(s)
         * 
         * @param string|array $table
         * @return bool|array
         * @throws PDOException 
         */
        public function drop_table(string|array $table){
            try{
                if(is_string($table)){
                    $qstart = $this->connect->prepare("DROP TABLE `${table}`");
                    if($qstart->execute()) return true;
                    else return false;
                }elseif(is_array($table)){
                    $r = [];
                    foreach($table as $t){
                        $qstart = $this->connect->prepare("DROP TABLE `${t}`");
                        if($qstart->execute()) $r[] = true;
                        else $r[] = false;
                    }
                    return $r;
                }else return false;
            }catch(PDOException $e){
                $this->latest_error_message = $e->getMessage();
                return false;
            }
        }

        /**
         * Truncate a specified table(s)
         * 
         * @param string|array $table
         * @return bool|array
         * @throws PDOException 
         */
        public function truncate_table(array|string $table){
            try{
                if(is_string($table)){
                    $qstart = $this->connect->prepare("TRUNCATE TABLE `${table}`");
                    if($qstart->execute()) return true;
                    else return false;
                }elseif(is_array($table)){
                    $r = [];
                    foreach($table as $t){
                        $qstart = $this->connect->prepare("TRUNCATE TABLE `${t}`");
                        if($qstart->execute()) $r[] = true;
                        else $r[] = false;
                    }
                    return $r;
                }else return false;
            }catch(PDOException $e){
                $this->latest_error_message = $e->getMessage();
                return false;
            }
        }

        /**
         * Shuffle a specified table
         * 
         * @param string $table
         * @return bool
         * @throws PDOException 
         */
        public function shuffle_table(string $table){
            try{
                $query = $this->exec_query("SELECT * FROM `${table}`");
                if($table_data = $query->fetchAll(PDO::FETCH_ASSOC)){
                    $table_data_shuffled = $this->shuffle_assoc($table_data);
                    if($this->truncate_table($table)){
                        foreach($table_data_shuffled as $data) $this->insert_row($table, $data);
                        return true;
                    }else return false;
                }else return false;
            }catch(PDOException $e){
                $this->latest_error_message = $e->getMessage();
                return false;
            }
        }

        /**
         * Delete row(s) based on search column and/or search parameters.
         * 
         * @param string $table table to delete from
         * @param string|bool $searchColumn column name to search from (or false if $searchParameter is an associative array)
         * @param string|int|bool|array $searchParameter value(s) to search for; if multiple values, use an indexed array; 
         * if multiple columns & values, use associative array as column=>value pairs and mark $searchColumn false
         * @return bool|array 
         * @throws PDOException 
         */
        public function delete_row(string $table, string|bool $searchColumn, string|int|bool|array $searchParameter){
            try{
                if(is_string($searchColumn) && is_array($searchParameter)){
                    $r = [];
                    foreach($searchParameter as $sP){
                        $delQuery = $this->connect->prepare("DELETE FROM `${table}` WHERE ${searchColumn}=:${searchColumn} LIMIT 1");
                        if(is_string($sP)) $delQuery->bindValue(":${searchColumn}", $sP, PDO::PARAM_STR);
                        elseif(is_int($sP)) $delQuery->bindValue(":${searchColumn}", $sP, PDO::PARAM_INT);
                        elseif(is_bool($sP)) $delQuery->bindValue(":${searchColumn}", $sP, PDO::PARAM_BOOL);
                        if($delQuery->execute()) $r[] = true;
                        else $r[] = true;
                    }
                    return $r;
                }elseif($searchColumn == false && is_array($searchParameter)){
                    $r = [];
                    foreach($searchParameter as $sC => $sP){
                        $delQuery = $this->connect->prepare("DELETE FROM `${table}` WHERE ${sC}=:${sC} LIMIT 1");
                        if(is_string($sP)) $delQuery->bindValue(":${sC}", $sP, PDO::PARAM_STR);
                        elseif(is_int($sP)) $delQuery->bindValue(":${sC}", $sP, PDO::PARAM_INT);
                        elseif(is_bool($sP)) $delQuery->bindValue(":${sC}", $sP, PDO::PARAM_BOOL);
                        if($delQuery->execute()) $r[] = true;
                        else $r[] = true;
                    }
                    return $r;
                }elseif(is_string($searchColumn) && !is_array($searchParameter)){
                    $delQuery = $this->connect->prepare("DELETE FROM `${table}` WHERE ${searchColumn}=:${searchColumn} LIMIT 1");
                    if(is_string($searchParameter)) $delQuery->bindValue(":${searchColumn}", $searchParameter, PDO::PARAM_STR);
                    elseif(is_int($searchParameter)) $delQuery->bindValue(":${searchColumn}", $searchParameter, PDO::PARAM_INT);
                    elseif(is_bool($searchParameter)) $delQuery->bindValue(":${searchColumn}", $searchParameter, PDO::PARAM_BOOL);
                    if($delQuery->execute()) return true;
                    else return false;
                }else return false;
            }catch(PDOException $e){
                $this->latest_error_message = $e->getMessage();
                return false;
            }
        }

        /**
         * Select specified row 
         * 
         * Find a row(s) based on specified column & parameters 
         * 
         * @param string $table
         * @param string $searchColumn
         * @param string|int|bool $searchParameter
         * @param int $limit
         * @return object
         * @throws PDOException 
         */
        public function select_row(string $table, string $searchColumn, string|int|bool $searchParameter, int $limit = 1){
            try{
                $query = $this->connect->prepare("SELECT * FROM `${table}` WHERE ${searchColumn}=:${searchColumn} LIMIT ${limit}");
                if(is_string($searchParameter)) $query->bindValue(":${searchColumn}", $searchParameter, PDO::PARAM_STR);
                elseif(is_int($searchParameter)) $query->bindValue(":${searchColumn}", $searchParameter, PDO::PARAM_INT);
                elseif(is_bool($searchParameter)) $query->bindValue(":${searchColumn}", $searchParameter, PDO::PARAM_BOOL);
                if($query->execute()) return $query;
                else return false;
            }catch(PDOException $e){
                $this->latest_error_message = $e->getMessage();
                return false;
            }
        }

        /**
         * Fetch an associative array from a table
         * 
         * Fetch an associative array from a table of a row(s) 
         * based on the search column and parameter provided.
         * 
         * @param string $table
         * @param string $searchColumn
         * @param string|int|bool $searchParameter
         * @param int $limit
         * @return array|bool
         * @throws PDOException 
         */
        public function fetch_assoc_row(string $table, string $searchColumn, string|int|bool $searchParameter, int $limit = 1){
            try{
                $query = $this->connect->prepare("SELECT * FROM `${table}` WHERE ${searchColumn}=:${searchColumn} LIMIT ${limit}");
                if(is_string($searchParameter)) $query->bindValue(":${searchColumn}", $searchParameter, PDO::PARAM_STR);
                elseif(is_int($searchParameter)) $query->bindValue(":${searchColumn}", $searchParameter, PDO::PARAM_INT);
                elseif(is_bool($searchParameter)) $query->bindValue(":${searchColumn}", $searchParameter, PDO::PARAM_BOOL);
                if($query->execute()) return $query->fetch(PDO::FETCH_ASSOC);
                else return false;
            }catch(PDOException $e){
                $this->latest_error_message = $e->getMessage();
                return false;
            }
        }

        /**
         * Shuffle an associative array
         * 
         * @param array $array
         * @return array
         * @throws PDOException 
         */
        public static function shuffle_assoc(array $array){
            $keys = array_keys($array);
            shuffle($keys);
            foreach($keys as $k) $r[$k] = $array[$k];
            return $r;
        }
    }

KIKO Software · Accepted Answer · 2022-06-16 17:16:26Z

You didn't explain, in your question, what your objectives were with this class. Why encapsulate a PDO class like this? My guess is that you did this to put all your database functions in one place? That's what it looks like. Is that a good enough reason?

I won't sugarcoat this: There are many problems with your code. Here are 3 problem I noticed, looking at the beginning of your class:

Why use class DB extends PDO when you have no intention of really extending the PDO class? Inside the DB constructor you create a new PDO object, instead of using the constructor of the one you have. You encapsulate the PDO class, instead of extending it. See: Object Inheritance.
You fix the HOST, USER and PASS as private constants inside your new DB class. This means that for every different host, and user, you will have to change this, and you might even end up with many, very similar, PHP files. The whole point of a class is reusability. In my opinion HOST, USER and PASS should be arguments for the constructor of this class.

Now let's make a list of your methods, without looking at their content, to get an overview of the functionality of your DB class:

class DB extends PDO {
    public object $connect;
    public bool $connected;
    public string $latest_error_message;
    public function __construct(string $DB_NAME)
    public function insert_row(string $table, array $column_value_pairs)
    public function exec_query(string $query)
    public function is_table(string $table)
    public function drop_table(string|array $table)
    public function truncate_table(array|string $table)
    public function shuffle_table(string $table)
    public function delete_row(string $table, string|bool $searchColumn, string|int|bool|array $searchParameter)
    public function select_row(string $table, string $searchColumn, string|int|bool $searchParameter, int $limit = 1)
    public function fetch_assoc_row(string $table, string $searchColumn, string|int|bool $searchParameter, int $limit = 1)
    public static function shuffle_assoc(array $array)
}

This immediately generates a lot of questions. For instance:

The $connected boolean only duplicates what is already in $connect.
How do I use exec_query() with bound parameters? I don't think I can do that. This would encourage me to put user input into the query string, just to get around that problem.
I can only insert one row at a time? That's very inefficient when I have a lot of rows to insert.
What on earth could shuffle_table() do? Remember that, in relational databases, rows are not ordered, so shuffling them does not make sense.
Why do select_row() and fetch_assoc_row() have a $limit parameter? Shouldn't they be called select_rows() and fetch_assoc_rows() if then can return multiple rows? And why, then, does delete_row() not have that parameter?

Diving into the code itself is difficult. This is just not how I would write my code. I will therefore restrict myself to pointing out some things that really won't do:

I see this pattern repeated 6 times:

   if(is_string($value)) $prep->bindValue(":${column}", $value, PDO::PARAM_STR);
   elseif(is_int($value)) $prep->bindValue(":${column}", $value, PDO::PARAM_INT);
   elseif(is_bool($value)) $prep->bindValue(":${column}", $value, PDO::PARAM_BOOL);

If you repeat something, that many times, you should create a separate method for it. This would prevent a lot of repetitious code. This could be a private method. See: DRY.

However, let's look closer at this code. It accepts some "value", which will be bound to a prepared statement. What would happen if this value is a floating point? Or what if the value is NULL? It won't do anything. Nothing will be bound. There's also no provision for other types, like arrays or objects. You could generate an exception for those, or, for instance, turn an array into a JSON string.

Next: Why would I ever need a function to drop or truncate multiple tables? And, if I would ever need to do that, wouldn't it be very easy to loop over an array myself? If drop_table() and truncate_table() would only accept a single table name, like their names suggest, it would make their implementation a lot simpler, and you won't sacrifice much. In the extreme event, that you really need this functionality, please use something like:

public function drop_tables(array $tables)
{
     foreach ($tables as $table) {
         if (!$this->drop_table($table)) {
             return false;
         }
     }
     return true;
}

Note that the method name now actually reflects what the method does. It is a good rule to never pack to much functionality into one method. Keep it simple. A method should do one thing, and do it well. In programming we say that a method should have a single responsibility.

Finally. Your intentions are good, I can tell, but the implementation could be better. I don't think you've actually used this code yourself very much, otherwise you would have noticed all the shortcomings I pointed out. But at least you've made a start. I hope this review helps you to improve your code.

Constructor always throws, despite the mode. Somewhat unintuitive — Your Common Sense
– Your Common Sense, Commented Jun 16, 2022 at 14:16
@YourCommonSense I didn't know. It makes sense though, otherwise you can never get an exception. I removed this from my answer to not mislead any readers. — KIKO Software
– KIKO Software, Commented Jun 16, 2022 at 17:17

mickmackusa · Accepted Answer · 2022-06-11 05:21:16Z

Much of my advice can be found in https://www.php-fig.org/psr/psr-12/.

The opening curly brace should be on the next line after the declaration.
Method names should be camelCase.
In most cases, I find DocBlocks to be unnecessarily verbose and with diminishing returns for what is described. There can be exceptions to this, but commonly I find well-named variables to be enough.
There is no benefit to curly quote wrapping the strings while accessing a variable.
I find $qhalf1 (and similar variables) to be a little too clunky/unpolished. In fact, I probably wouldn't bother to do type-specific param declarations (if I would, I would use an else with PARAM_STR so that float values weren't lost). Also in my applications, it is most common to have an auto-incremented column for virtually all tables. If you expected the same to be true in your project, it will be more useful to return the generated id value instead of only a boolean value. There are fringe cases to consider, sure, but perhaps you could build off of something like this:
```
$stmt = $this->connect->prepare(
    sprintf(
        "INSERT INTO `$table` (`%s`) VALUES (%s)",
        implode('`, `', array_keys($columnValuePairs)),
        implode(',', array_fill(0, count($columnValuePairs), '?'))
    )
);
$stmt->execute(array_values($columnValuePairs));
return (int) $this->connect->lastInsertId();
```
I don't think I have any methods in any project where a parameter might be array|string. The incoming data should be standardized as iterable or not. If you are going to honor potentially iterable data, then only accept an iterable data type and if the calling script has a scalar value, then just add (array) in front of it to cast it as a single-element array. Drawing a hard line rule like this affords your project to have leaner method bodies -- they can immediately and unconditionally iterate the incoming data.
I can't think of one reason why it would be useful to have shuffle_table() in a project. I'd remove this from the class entirely.
delete_row() was so W.E.T. (not D.R.Y.) that I couldn't even bring myself to review it. This probably needs a rethink/refactor. Why is the incoming data allowed to be so varied.
The method name select_row() seems misleading to me. It doesn't return a row. Without reading the method body, I'd assume that it should return a flat object representing a table row, or else null in the event that a row was not found. This is not the case. It endeavors to return the prepared statement object or false. Honestly, I don't like falling back to false; I prefer falling back to null -- PHP has nullable type syntax (e.g. ?object), but doesn't have falsible type syntax.
shuffle_assoc() doesn't seem to have any business in the DB or PDO class; it should just be in some array helper class somewhere else.

Your Common Sense · Accepted Answer · 2022-06-16 19:50:41Z

Given that all cosmetic suggestions were already given, I'd concentrate on rather critical flaws.

Architecture-wise, such a class shouldn't exist. Technically, it is not even a class at all, but rather an assorted collection of procedures, where $this->connection is used as a politically correct substitution for global.

A class should never be just a collection of functions. It should represent some logical entity, a piece of a jigsaw puzzle that needs to be fitted, along with other pieces, into the complete picture. The methods and properties alike should serve for the single united purpose. For example, PDO class is responsible for the database connection and query execution. It's the highest level, that should know nothing of the particular tables. Basically, it's a data storage driver that, in theory, could be replaced by another driver. In this regard, ideally, the entire database class could be written to replicate the full PDO's functionality, using PDO internally as a driver. But for now, it will be superfluous. But nevertheless, the functionality of the main database class must be limited to the main database handling tasks. Whereas particular queries must be delegated to another tier. Which would use the DB class as a dependency.

As it was already noted, this class severely violates the Single responsibility principle. It mixes connection handling, credential storage, table maintenance, query execution, result fetching, and a thick piece of imagination. Come on, what's the reason doing all this in a single class?

There are many consequences from this structure, and the most important one is that your class becomes inevitably and critically vulnerable to SQL injection.

Another concern, which is also related to OOP, but not in relation to the class structure but rather to the way it is going to be used. Having a function return false on failure is too ancient an approach. The modern way of handling errors is to make your functions throw Exceptions in case of error. Therefore, all this try/catch/latest_error_message business should be removed altogether.

In order of improvement, this class should be split into three, or at least two

only (a secure version) of execute_query could be left extending PDO. To get the query result we will use the excellent PDOStatement class that can return SQL results in dozens different formats.
all the functions working with certain tables must be moved into a distinct Table Gateway class
an additional Query Builder class could be added. But NOT in that crippled form of select_row(). Either make it real or don't make it at all.

And also we will throw away all the functions that are made for the imaginary use cases.

As a result, the main class will become

class DB extends PDO{
    public function __construct($dsn, $username = NULL, $password = NULL, $options = [])
    {
        $default_options = [
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            PDO::ATTR_EMULATE_PREPARES => false,
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
        ];
        $options = array_replace($default_options, $options);
        parent::__construct($dsn, $username, $password, $options);
    }
    public function run($sql, $args = NULL)
    {
        if (!$args)
        {
             return $this->query($sql);
        }
        $stmt = $this->prepare($sql);
        $stmt->execute($args);
        return $stmt;
    }
}

And so your unreadable

$row = $db->fetch_assoc_row("xxx", "yyy", $id);

will become

$row = $db->run("SELECT xxx WHERE yyy=?", [$id])->fetch();

(and I really hope that your entire enterprise weren't started only to get rid of a few SQL keywords)

While all functions working with certain tables have to be moved into a distinct class, which, having all table and column names hardcoded, will make the application code 100% secure from SQL injections.

Take this TableGateway class for example. It's closer to OOP because

it features encapsulation: it encapsulates internally all the stuff related to the SQL interaction with a single table. Its properties - $table, $fields, $primary - belong to a single particular entity.
it features polymorphism: the same set of methods could be used for different database tables
it features inheritance: we've got the basic class where the code for the all the basic database operation is written once, and you don't have to repeat it again

You will have to add new methods only when you need a specific query. For example, there is a UserGateway class, and we need to retrieve the record by email. No problem:

class UserGateway extends Basic
{
    protected $table = 'users';
    protected $fields = array('email', 'password', 'name',);

    public function getByEmail($email) {
        $sql = "SELECT id, password FROM users WHERE email=?";
        return $this->db->run($sql, [$email])->fetch();
    }
}

You may say that your select_row() could be used without the need of creating a dedicated method. That's true, but only with a real Query Builder. There are always small nuances, different conditions that your function doesn't support. A function for just a single condition has too limited use. That's why I said that it should be either a full featured Query builder that supports multiple conditions, multiple comparison operators, etc etc - or just a vanilla SQL query, which, among other benefits, brings readability to your code.

Stack Exchange Network

DB Class extending PDO w/ other useful functions

3 Answers 3

You must log in to answer this question.

Linked

Hot Network Questions

DB Class extending PDO w/ other useful functions

3 Answers 3

You must log in to answer this question.

Linked

Related

Hot Network Questions