3
\$\begingroup\$

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();
            }
        }
    }
?>
\$\endgroup\$
1
\$\begingroup\$

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.
\$\endgroup\$
  • \$\begingroup\$ 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? \$\endgroup\$ – Tom Jan 21 '16 at 17:55
  • \$\begingroup\$ @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) \$\endgroup\$ – tim Jan 21 '16 at 18:13
  • \$\begingroup\$ 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. \$\endgroup\$ – Tom Jan 21 '16 at 18:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.