PHP PDO Factory Classes

Question

So, my question is more of a 'best practices' question rather than a question with a particular aim. This is my current understanding of PHP factories and how to incorporate them into a project using PDOs and dependency injection. I'm pretty new at this and I still don't understand a lot about this subject. As such, I really don't know if I'm doing this quite right. Could someone point me in the right direction?

<?php
class PDOSettings_Factory {
    private function __construct() {}
    public static function build(Array $args = NULL) {
        $settings = array();
        // Set default values.
        $settings["dbType"] = 'mysql';
        $settings["dbName"] = 'test';
        $settings["host"] = 'localhost';
        $settings["user"] = 'guest';
        $settings["pass"] = '1234';
        // If keys are the same, they will be overwritten.
        if(isset($args)) {
            foreach($args as $key => $val) {
                $settings[$key] = $val;
            }
        }
        // Return setings.
        return $settings;
    }
}

$settings = PDOSettings_Factory::build();

class PDOConnection_Factory {
    private function __construct() {}
    public static function build($args) {
        // Set up PDO connection
        if(!isset($args)) {
            return NULL;
        } else {
            $statement = 
                $args['dbType'].':'.
                'host='.$args['host'].';'.
                'dbname='.$args['dbName'];
            try {
                $dbh = new PDO(
                    $statement, 
                    $args['user'], 
                    $args['pass']);
                $dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
                // Return DBH
                return $dbh;
            } catch (PDOException $e) {
                echo 'Error: ',  $e->getMessage(), "\n";
            }
        }
        return NULL;
    }
}

And how to use the factories:

class demo {
    private $dbh = NULL;
    public function set($args) {
        if (!isset($args)) {return FALSE;} 
        else {
            $this->dbh = $args;
            return TRUE;
        }
    }
    public function __construct($args = NULL) {
        $this->set($args);
    }

    public function doSomething() {
        // Set SQL command here.
        $sql = 'SELECT * FROM test';

        $results = $this->dbh->prepare($sql);
        // Perform param binds here.
        $results->execute();
        return $results;
    }
}

$db = PDOConnection_Factory::build($settings);

$test = new demo($db);
foreach ($test->doSomething() as $row) {print_r($row[0].PHP_EOL);}
?>

Answer 1

demo should be Demo and in new classes you might want to use namespaces instead of underscores in your class names. See PSR-0

As non of your $args is optional and there are only 5 I would recommend the use real method parameters instead of this array. Otherwise some validation and documentation of this array is missing.

Furthermore you could refactor your ifs to real guard conditions and skip the else branch. This makes the code more readable and you get rid of a indentation level. There are also no braces on one-line-ifs.

Edit Sample for guard conditions

class PDOConnection_Factory {
    private function __construct() {}
    public static function build($args) {
        if (!isset($args)) return NULL;
        $statement = ...
        try {
            $dbh = ...
            return $dbh;
        } catch (PDOException $e) {
            //as Cygal stated this is not very nice
            echo 'Error: ',  $e->getMessage(), "\n";
            return NULL;
        }
    }
}

Edit 2 Handle many and optimal method/constructor parameter

First the usage:

new PDOConnectionBuilder().name("test").user("guest").password("1234").create();

And now the details:

class PDOConnectionBuilder()
{
    private type="mysql"; //defaults
    private name;
    private host="localhost";
    private user;
    private password;

    public function create()
    {
        //guards again
        if (empty($name)) throw new PDOConnectionBuilderException("Name is not optional");
        if (empty($user)) throw ...
        if (empty($password)) $passwordToUse=$user; //fallback to default
        else $passwordToUse=$password;

        //do the required stuff
        return $connection;
    }

    public function name ($name)
    {
        $this->name=$name;
        return $this;
    }
    //...
}   

This is a really awesome pattern for building complex object in larger system and in tests. You could even reuse the builder and call create twice. Or change only one property and call create again.

Answer 2

Formatting strings

You should try to avoid those kind of things:

$statement = $args['dbType'].':'.'host='.$args['host'].';'.'dbname='.$args['dbName'];

It's easy to miss something, and it's also this kind of constructions that leads to difficult to read code and (in other contexts) SQL injections. I would argue that using sprintf is a better idea here:

$format = '%s:host=%s:dbname=%s';
echo sprintf($format, $args['dbType'], $args['host'], $args['dbName']);

I don't know if it is considered idiomatic PHP or not, but it seems more readable and less error-prone to me.

PDO and exceptions

mnhg already gave you a way to get rid of the first return NULL in PDOConnection_Factory::build(), but it's not enough, you should also remove the second one. I assume it comes from the possible exceptions in PDO::__construct and PDO::setAttribute. The issue is that you're catching the exception but doing nothing with them.

You should instead embrace exceptions. :) A good first step is the setAttribute call. Since there's nothing you can do to handle them in the factory: you should instead let the application code decide what to do if something fails: for example, display a warning message to users and send an email to the administrator.