Is this wrapper for PDO 'good code' ? Are there any potential problems?

Answer 1

Yes and No.

It is good code for a simple quick and dirty application.

The problem comes when you use this in a more complex structured application. Where the error handling will vary depending on which sql you are executing.

Also any severe errors will show up as "problem at line 999" type errors where 999 is in your super duper routine and you will have difficulty tracing it back to a particular sql request.

Having said that I do this sort of thing myself all the time on small projects.

Answer 2

It's not bad and as it's been said it might help for small applications although it's mostly a very thin abstraction on another abstraction. It's not bringing a lot of others functionalities.

Something you might want to consider, amongst other things:

• As this is PHP5 code, use exceptions instead of trigger_error and set_exception_handler if necessary until exceptions are more widespread, but it's definitely cleaner and more future-proof.
• You are using a singleton, it's not a bad thing necessarily but in this case, for example, one shortcoming will be that you'll only be able to handle one connection to one database.
• I don't know if you make use of stored procedures, but a stored procedure might return a result set through the query() method too.
• You have two semi-colons (;;) at the end of your new PDO line.

That being said, I don't think your query method is too big and there's not much that could be recalled from elsewhere in there at the moment. Though as soon as you see two or three lines that could be called from another function, split it. That's a good way to DRY.

Answer 3

Here's what I've used (just replace the references to Zzz_Config with $GLOBALS['db_conf'] or something):

/**
 * Extended PDO with databse connection (instance) storage by name.
 */
class Zzz_Db extends PDO
{
    /**
     * Named connection instances.
     *
     * @var array
     */
    static private $_instances;

    /**
     * Retrieves (or instantiates) a connection by name.
     *
     * @param  string $name  Connection name (config item key).
     * @return Zzz_Db        Named connection.
     */
    static public function getInstance($name = null)
    {
        $name = $name === null ? 'db' : "db.$name";
        if (!isset(self::$_instances[$name])) {
            if (!$config = Zzz_Config::get($name)) {
                throw new RuntimeException("No such database config item: $name");
            }
            if (!isset($config['dsn'])) {
                if (!isset($config['database'])) {
                    throw new RuntimeException('Invalid db config');
                }
                $config['dsn'] = sprintf('%s:host=%s;dbname=%s',
                    isset($config['adapter']) ? $config['adapter'] : 'mysql',
                    isset($config['host']) ? $config['host'] : 'localhost',
                    $config['database']);
            }

            $db = self::$_instances[$name] = new self(
                $config['dsn'],
                isset($config['username']) ? $config['username'] : null,
                isset($config['password']) ? $config['password'] : null);
            $db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            //$db->setAttribute(PDO::ATTR_STATEMENT_CLASS, 'Zzz_Db_Statement');

            if ($db->getAttribute(PDO::ATTR_DRIVER_NAME) == 'mysql') {
                $db->setAttribute(PDO::ATTR_EMULATE_PREPARES, true);
                $db->exec('SET CHARACTER SET utf8');
            }
        }

        return self::$_instances[$name];
    }
}

Usage whould be:

$db = Zzz_Db::getInstance(); // or Zzz_Db::getInstance('some_named_db')
$stmt = $db->prepare('SELECT ...

The goal is to keep the db configuration in an *.ini file (editable by a non-coder).

Answer 4

To answer your question, if it is a good code or not, ask yourself:
What is the added value of my code compared to using PDO directly?

If you find a good answer, go for using your code. If not, I would stick with PDO.

Also try considering implementing Zend Framework's DB class which works on its own and supports PDO.

Answer 5

I went the other way and made a class that extends PDO with a bunch of wrapper functions around prepare()/execute(), and it's much nicer than the built in functions (though that's a bit subjective...).

One other thing: you should set PDO::ATTR_EMULATE_PREPARES to false unless you're using a really old version of mysql (<=4.0). It defaults to true, which is a huge headache and causes things to break in obscure ways... which I'm guessing is the reason you've got a huge wrapper around bindParam() in the first place.