Extended PDO class for MySQL

Question

I recently started with object oriented programming in PHP and I am wondering if my database class is on the right track.

Am I doing something wrong? If so, how can I improve it?

Here is the class:

    <?php
    class PDOC extends PDO{
        private $myselhost;
        private $dbname;
        private $myseluser;
        private $mysqlpass;

        public function __construct($mysqlhost, $dbname, $mysqluser, $mysqlpass){
            try{
                parent::__construct("mysql:dbname={$dbname};host={$mysqlhost}", $mysqluser, $mysqlpass);
                $this->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            }catch(PDOException $e){
                return $e->getMessage();
                //die('Something went wrong, please try again.');
            }
        }

        public function queryInsert($tableName, $insertKey, $insertData, $executeData){
            $prepare = parent::prepare("INSERT INTO {$tableName} ({$insertKey}) VALUES ({$insertData})");

            try{
                $prepare->execute(
                    $executeData
                );
            }catch(PDOException $e){
                return $e->getMessage();
            }
        }

        public function queryUpdate($tableName, $insertData, $executeData, $insertWhereKey, $insertWhereValue){
            $prepare = parent::prepare("UPDATE {$tableName} SET {$insertData} WHERE {$insertWhereKey} = {$insertWhereValue}");

            try{
                $prepare->execute(
                    $executeData
                );
            }catch(PDOException $e){
                return $e->getMessage();
            }
        }

        public function queryDelete($tableName, $deleteWhereKey, $deleteWhereValue){
            $prepare = parent::prepare("DELETE FROM {$tableName} WHERE {$deleteWhereKey} = {$deleteWhereValue}");

            try{
                $prepare->execute();
            }catch(PDOException $e){
                return $e->getMessage();
            }
        }

        public function querySelect($tableName, $selectData, $sellectWhereKey = null, $selectWhereValue = null, $orderByKey = null, $orderByValue = null){
            if(isset($selectWhereKey, $selectWhereValue)){
                $prepare = parent::prepare("SELECT {$selectData} FROM {$tableName} WHERE $insertWhere");
            }
            elseif(isset($orderByKey, $orderByValue)){
                $prepare = parent::prepare("SELECT {$selectData} FROM {$tableName} ORDER BY {$orderByKey} {$orderByValue}");
            }
            elseif(isset($selectWhereKey, $selectWhereValue, $orderByKey, $orderByValue)){
                $prepare = parent::prepare("SELECT {$selectData} FROM {$tableName} WHERE {$selectWhereKey} = {$selectWhereValue} ORDER BY {$orderByKey} {$orderByValue}");
            }
            else{
                $prepare = parent::prepare("SELECT {$selectData} FROM {$tableName}");
            }

            try{
                $prepare->execute();
            }catch(PDOException $e){
                return $e->getMessage();
            }
        }
    }
?>

Answer 1

Your approach vs PDO

First of, you take perfectionally acceptable exceptions, and transform them into error strings, which are harder to handle.

And I'm also not sure if all your the methods really make things easier.

Let's take insert as an example:

// with your code:
$result = $db->queryInsert("myTable", "key1, key2, key3", ":value1, :value2, :value3", ["value1", "value2", "value3"]);
if ($result == "well, what do I even check here?") {
    // handle error
}

// without your code:
$prepare = $db->prepare("INSERT INTO myTable (key1, key2, key3) VALUES (:value1, :value2, :value3)");
try {
    $prepare->execute(["value1", "value2", "value3"]);
} catch(PDOException $e) {
    // handle error
}

Without your code, it's not that much more to write, and it is a lot clearer what is actually happening.

Improving your approach

Now, I'm not generally a fan of wrapping/extending PDO like this, but you can improve your methods to make it a bit more usable.

First of, documentation. It is completely unclear from the outside how to use your code. This can also easily lead to security issues, as $insertKey and $insertData should not be user-supplied.

Secondly, it's really not needed that a user passes $insertData themselves. It makes your method harder to use, it doesn't provide any additional functionality for a user, and as said above, it can easily lead to vulnerabilities.

Thirdly, as mentioned, use exceptions instead of error strings.

A better method signature might be this:

public function queryInsert($tableName, $insertKey, $insertData) throws PDOException

Now it can be used like this:

try {
    $db->queryInsert("myTable", "key1, key2, key3", ["value1", "value2", "value3"]);
} catch(PDOException $e) {
    // handle error
}

That's already a bit nicer, but really not that much simpler than the original query (and harder to use, as it's not really clear what the passed values need to be), so I'm not sure if it's worth it.

I would probably stick to the well-known and simple PDO interface. If you want to simplify it, or don't like to write plain SQL queries, something like a query builder or an orm might be worth looking into.

Security

Your approach completely disguises which values are safe and which are not, which will lead to security issues.

Basically, only $executeData may contain user input.

For some of the other values, a user may not expect it to be secure (such as $tableName), but just to be safe, I would still validate it.

For some values, this is devestating. Having such an interface as yours, I would completely expect eg $insertWhereValue to be secure.

Misc

• Bug: $insertWhere doesn't exist.
• You have a couple of spelling mistakes, such as sellect or mysel.