PHP PDO wrapper class

Question

I'm one of those coder/designers that has been at it since day one, but is still getting the hang of several important coding nuances (e.g. OOP for PHP, and concepts such as this).

In this case, I'm migrating my toolkit from MySQL to PDO. I had considered MySQLi, but a critical mass of stackoverflow leans towards PDO, so ... I defer to experience. I've spent a few weeks in search of the best PDO wrapper class, studying dozens of PDO db wrapper classes on github, sourceforge, google code, etc.

Most of what I found was either kind of absurd, or added excessive complexity, or was over my head. So I opted to cannibalize, recombine and redesign things to work with my coding flow. As I'm new to PHP OOP, and new to PDO, it's likely I'm overlooking major things here.

I'd be most grateful if anyone could help improve this, or shed light on anything I'm not seeing! Thanks :)

Rough docs and examples of use can be found here: http://www.designosis.com/PDO_class/

class db extends PDO {

    private $error = '';
    private $crypt_salt = 'somesillystringforsalt'; // min 22 chars alphanumeric
    public  $querycount = 0;

    public function __construct($dsn, $user='', $passwd='') {
        $options = array(
            PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8',
            PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,
            PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION,
            PDO::ATTR_PERSISTENT => true
            );
        try {
            parent::__construct($dsn, $user, $passwd, $options);
        } catch (PDOException $e) {
            $this->error = $e->getMessage();
        }
    }

    public function run($query, $bind=false, $handler=false) {
        $this->querycount++;
        try {
            if ($bind !== false) {
                $bind = (array) $bind;
                $dbh = $this->prepare( trim($query) );
                $dbh->execute( $bind );
            } else {
                $dbh = $this->query( trim($query) ); // because query is 3x faster than prepare+execute
            }
            if (preg_match('/^(select|describe|pragma)/i', $query)) {
                // if $query begins with select|describe|pragma, either return handler or fetch
                return ($handler) ? $dbh : $dbh->fetchAll();
            } else if (preg_match('/^(delete|insert|update)/i', $query)) {
                // if $query begins with delete|insert|update, return count
                return $dbh->rowCount();
            } else {
                return true;
            }
        } catch (PDOException $e) {
            $this->error = $e->getMessage();
            return false;
        }
    }

    private function prepBind($pairs, $glue) {
        $parts = array();
        foreach ($pairs as $k=>$v) { $parts[] = "`$k` = ?"; }
        return implode($glue, $parts);
    }

    public function update($table, $data, $where, $limit=false) {
        if (is_array($data) && is_array($where)) {

            $dataStr  = $this->prepBind( $data, ', ' );
            $whereStr = $this->prepBind( $where, ' AND ' );
            $bind = array_merge( array_values($data), array_values($where) );

            $sql = "UPDATE `$table` SET $dataStr WHERE $whereStr";
            if ($limit && is_int($limit)) { $sql .= ' LIMIT '. $limit; }
            return $this->run($sql, $bind);
        }
        return false;
    }

    public function insert($table, $data) {
        if (is_array($data)) {

            $dataStr = $this->prepBind( $data, ', ' );
            $bind = array_values( $data );

            $sql = "INSERT `$table` SET  $dataStr";
            return $this->run($sql, $bind);
        }
        return false;
    }

    public function delete($table, $where, $limit=false) {
        if (is_array($where)) {

            $whereStr = $this->prepBind( $where, ' AND ' );
            $bind = array_values( $where );

            $sql = "DELETE FROM `$table` WHERE $whereStr";
            if ($limit && is_int($limit)) { $sql .= ' LIMIT '. $limit; }
            return $this->run($sql, $bind);
        }
        return false;
    }

    // crypt (one-way encryption)
    public function crypt($str, $moresalt='') {
        $salt = $moresalt . $this->crypt_salt;
        if (CRYPT_BLOWFISH == 1) {
            $presalt = (version_compare(PHP_VERSION,'5.3.7','<')) ? '2a' : '2y'; // PHP 5.3.7 fixed stuff
            $blowfish_salt = '$'. $presalt .'$07$'. substr($salt, 0, CRYPT_SALT_LENGTH) .'$';
            return crypt($str, $blowfish_salt);
        }
        return sha1($str . $salt);        
    }

}

Answer 1

I agree with Corbin, this probably shouldn't extend the PDO class. What he means by this is that its too limiting. You are using PDO now, but what if, as he asks, you don't want to use MySQL in the future? then you are stuck rewriting this entire database class because it is entirely hardcoded for MySQL. All databases share some common traits, they can add, remove, update, etc... So if you made this class a little more loosely it could be extended to use any database subtype. And that is the whole point of OOP: Creating reusable objects that can be extended past their initial purpose without refactoring. Now, on to review...

This is purely preference, but for legibility I would consider adding spaces around your operators when doing comparisons. Its the same concept as adding spaces around the equals sign when defining variables.

if ($bind !== false) {

ALWAYS, ALLways, always use braces on your statements, even for one-liners. Yes PHP allows this, but even they say this is wrong. In other languages, such as Python, where the braces are not necessary or don't exist, this may be fine, but in PHP it can cause issues with your code. Its just two characters, and not only does it ensure your code doesn't have issues, but it helps with legibility as well. Or, if you fancy one-liners, you may think of using ternary (though some people frown on its use). BTW: you don't need parenthesis around the ternary statement.

if (!is_array($bind)) { $bind = array($bind); }
//ternary
$bind = is_array( $bind ) ? $bind : array( $bind );

Of course, I don't even think ternary is necessary. I believe you can just typecast it here. You can either do it right after checking if it is not FALSE, or you can do it when you initially set the parameter in the method. Then you should also give it a default array value so that you can check if it is empty instead of FALSE. Type hinting it in the parameter list is the preferred method because PHP wont allow you to use anything else other than the specified parameter type, this will make all of your is_array()...return FALSE checks unnecessary.

$bind = ( array ) $bind;
//Or
public function run( $sql, Array $bind = array(), $handler = FALSE ) {

Variable names should almost always be specific and descriptive. There are some exceptions, such as for throw away variables such as iterator counters $i and exceptions $e. There are probably more, but these are two major ones. So a variable, $q, is not descriptive enough.

Consider documenting your REGEX. Not everyone can read it, so trying to figure out what it does can be difficult. For instance, I can't read this alien script, so I have no idea what this does. So I can't tell you if there is some better way that doesn't use REGEX. Typically the tasks I find people using REGEX for can be accomplished more quickly and efficiently with non-REGEX functions, so this is something to consider. If you comment this REGEX, I'll come back and take another look.

UPDATE

I didn't test this, but it reads right. So admitedly, this is probably not any easier, nor shorter, but it may be faster. Probably not due to all the function calls, I didn't profile it. But this is something you have to keep in mind when using REGEX. REGEX is usually slower than a single string function, or sometimes multiple string functions. This would be a preference call here. Which do you find easier? Which do you find faster? Do you need to worry about speed? Which do you find more legible? Which, taking all of these questions into mind, best fits your requirements? Probably here it will be the REGEX function, but its entirely up to you, just figured I'd show you another way :)

$pointers   = array( 'select', 'describe', 'pragma' );
$editors    = array( 'delete', 'insert', 'update' );

$mods       = array_merge( $pointers, $editors );
$lengths    = array_map( 'strlen', $mods );
$max        = max( $lengths );

$first          = substr( $query, 0, $max );

if( in_array( $first, $pointers ) ) {
    return $handler ? $dbh : $dbg->fetchAll();
} else if( in_array( $first, $editors ) ) {
    return $dbh->rowCount();
} else {
    return TRUE;
}

END OF UPDATE

I notice that you are not appending to your $error property but are overwriting it each time. Are you handling the errors as they come up, or are you just ignoring all but the last one? Not that you should be try/catching inside these methods, this is typically done in implementation of the class to avoid repetition.

The && and AND operators are not the same. You should use && over AND due to its precedence. But even if you were to use the later rather than the former, it should be in all-caps AND instead of and.

if (is_array($data) && is_array($where)){

I would consider using PHP's escape string for the following line. It will enhance legibility in this instance, as the back ticks wont be competing with the single quotes. Also, why $sql1 and $sql2? These aren't SQL statements, they are parameters. I would call them $whereParams and $dataParams.

$sql1[] = "`$k` = ?";

And where did these arrays come from? You are using this variable as an array without ever having declared it. What if this variable had been defined as a string earlier, or, god forbid, it was a global with a string value? Then you would be appending data onto the end of a string instead of the end of an array, and then implode() would not work properly, and then the world would come to a fiery end! Always define your arrays before using them to ensure that they aren't already being used for something else. PHP does allow array syntax to be used on strings.

$sql1 = array();
$sql2 = array();
$bind = array();

BTW: You seem to be doing this foreach loop quite a bit, even twice is enough to warrant a function/method. This follows the "Don't Repeat Yourself" (DRY) Principle, which is a key part of OOP, that and the "Single Responsibility" Principle. I use compact() and extract() here, though you may want to consider other methods. Not everyone approves of this method because there is no way to tell that these variables were used, therefore your IDE will not be able to track them and may even throw up some warnings. Its probably best to have it return an array with those two arrays imploded appropriately anyways to avoid having to repeat that task.

private function _getParams( $array ) {
    $params  = array();
    $bind    = array();

    foreach( $array as $k => $v ) {
        $params[]  = "`$k` = ?";
        $bind[]    = $v;
    }

    return compact( 'params', 'bind' );
}

extract( $this->_getParams( $where ) );//$params, $bind

Would consider removing all of those implode()s from inline and assigning them to variables to enhance legibility. Lines of code should not become so long, or convoluted, that they become difficult to read. Even the below is starting to look sketchy because of all of the concatenations going on. I would consider using double quotes here or dropping this down to multiple lines.

$dataString     = implode( ', ', $sql1 );
$whereString    = implode( ' AND ', $sql2 );
$sql            = 'UPDATE `'. $table .'` SET '. $dataString .' WHERE '. $whereString;

Unless this is a version number you are checking for, or there is some other reason that the CRYPT_BLOWFISH constant is set to one "1", then it should really use the TRUE/FALSE switch that PHP already provides, then you can just ask about the constant directly. However, this constant came out of nowhere. What if its not been defined? It's not part of either class, so it can't be guaranteed to exist. This method should really be done elsewhere, where this constant, and the others it uses, make more sense in the local scope.

if( CRYPT_BLOWFISH ) {