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();
            }
        }
    }
?>

tim · Accepted Answer · 2016-01-21 17:07:03Z

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.

Thanks for the feedback, it is still a work in progress, so yeah. It is also my first actual class that I am working on, so I was more wondering whether I am doing it right or not. One more thing is, how do I implement the public function queryInsert($tableName, $insertKey, $insertData) throws PDOException in the class? — Tom
– Tom, Commented Jan 21, 2016 at 17:55
@Tom Well, as to doing it right: At a minimum, you really need to recheck the security of your code (when doing this, always consider two things: How will a user generally use my code (this must be secure), and how can a user mess up (which ideally should still be secure)). But like I said, I don't really see the value of your current approach. If you're still learning, I think implementing that function yourself would be a great exercise. It shouldn't be that difficult (you just need to build the :var1, :var2, ... or ?, ?, ... string dynamically, depending on the size of the values array) — tim
– tim, Commented Jan 21, 2016 at 18:13
Yeah, I wanted to do that at first, but I couldn't figure out how I would go about doing that, so I decided against it, which I know wasn't smart. — Tom
– Tom, Commented Jan 21, 2016 at 18:19

Stack Exchange Network

Extended PDO class for MySQL

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Extended PDO class for MySQL

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions