Return to Answer

• 1

• 11k
• 31
• 49

Ok, first I'm going to point out a couple of stylistic issues I have with your code:

• USE TYPE-HINTS: your addConnection method expects a PDO instance as second argument, a type-hint makes sure your code fails if this method is not passed the correct argument.
• Fix your doc-blocks: all of the params in your doc-blocks are denoted as being of the type string. This is false. Like I mentioned above: if you need a PDO instance, the method signature and the documentation should reflect this.
• Coding standards: the PHP-FIG coding standards are widely adopted, and are as close to official standards as you can get in PHP. You should adhere to them, too.
• Statics, in PHP, are essentially globals in OOP-drag. Things like Singletons are widely accepted to be bad practice. they make for hard to test code, and don't really offer you any advantage.
• You seem to have the intent of writing a sort-of wrapper around PDO. I have been quite vocal about this here. More specifically: my distaste of such practices and the pointlessness of it all. Don't do it.

A quick note on the type-hints, in case you don't know how they work: it's really easy:

/**
 * @param string $name
 * @param \PDO $conn
 * @return $this
 */
public function setConnection($name, PDO $conn)
{
 $this->connection = $conn;
 return $this;
}
//using this method:
$instance->setConnection('foobar', new PDO());//will work fine
$instance->setConnection('foobar', 'a string');//<-- fatal error

This makes life a lot easier when debugging. That, and any decent IDE will be happy to notify you of invalid arguments, without having to run the actual code code...

Be that as it may: I do understand that, in sizable projects, having access to existing DB connections can be useful, and that using straightforward dependency injection to achieve this can be tedious and sometimes error-prone.
However, seeing as I don't really know how big your project is, I will say that resorting to a sort of Registry should be kept as a last resort. Especially since your use of static properties to store the connections is very dangerous, too:

Static properties are shared across all instances and classes. Both parent and child. By making the properties protected, the child class can override them, which would constitute a breach of contract, to say the least. This is dangerous, so if you are set on using your code, I'd urge you to declare the properties as private and implement a couple of final protected getter functions in the parent class, to prevent any child classes from messing up your system.

Also be aware of things like late-static binding: you may have come across code like this:

class Foo
{
 protected static $StaticVar = 'some value';
 public function doStuff()
 {
 return static::$StaticVar;
 }
 public function otherStuff()
 {
 return self::$StaticVar;
 }
}
class Bar extends Foo
{
 protected static $StaticVar = 'another value';
}
$b = new Bar;
echo $b->doStuff(), ' <=> ', $b->otherStuff(), PHP_EOL;

Do you know what the output will be? Look closely at the use of self:: vs static::: the output is:

another value <=> some value

Couple that to the fact that the Liskov principle states that the child may not restrict the inherited contract (ie: a parents protected property or method may not be overridden by a private one), but a child can make an inherited member more visible:

class Dad
{
 protected $foo = null;
}
//perfectly valid
class LegitimateSon extends Dad
{
 public $foo = 'now $foo is public!';
}
class IllegitimateSon extends Dad
{//FATAL ERROR
 private $foo = false;
}

And what you end up with is a potentially dangerous:

class DangerousSQL extends Database
{
 //note the CamelCased class name, and statics start with an Upper-case, too
 public static $Con = array();//<-- overrides the protected static
 //your query method...
 public static function Query($string)
 {}
}
DangerousSQL::$Con = 'This is not an array!';
DangerousSQL::Query('SELECT fail FROM bad.code WHERE inheritance = 1');

And here, you're in trouble. Now the line of code where the $Con property is assigned a string is in plain sight, but imagine a sizable project where this reassignment is hiding an another file, and isn't always executed. Or worse: somebody has extended the DangerousSQL class, and it's a child of this class that is causing problems... What now?

Let me paint you a scene that is quite plausible given the above situation: imagine yourself sitting at your desk, you're hungry, it's getting late, and you're stuck chasing an obscure bug that is proving rather hard to replicate...
You've ran unit tests, so it can't be the code itself, it has to be an expression or statement somewhere in the actual application. You've set up XDebug, and set breakpoints at every line of code that involves the class that is causing the problems. At no point do you see the $Con property being reassigned, but just before PHP reaches the statement that causes the fatal error, you notice $Con is no longer an array... but how? You'll have to sift through all of your code, the DangerousSQL class, and all of its children. Each time looking for a statement that reassigns $Con. Throw in variable variables, undocumented methods, an inconsistent use of the late-static binding keyword static mixed in with some old-school self:: usage, and calls returning what is likely to be an array, and pretty soon you'll find yourself crying bloody murder.
Now think to yourself: Do you really want to use statics?

Honestly, I can't think of any reason to actually use a sort of global exposing mechanism for my DB connections like you have here. Instead, I'd either use patterns like: injection, service locator pattern (has its pro's and cons), or an even an observer pattern (where you register instances with each other, which requires you to unregister them, too so it's a bit of a faff).
But really, what you would benefit from most of all is a simple config object, in which you store all of your database connections. This config can serve as a registry, or be used in a factory, or a facade...

Basically, configure your connections along the lines of this (yaml) example:

# config.yml
databases:
 alias:
 dsn: 'mysql:host=127.0.0.1;dbname=foobar'
 user: 'username'
 pass: 'passwd'
 attr: 
 PDO::ATTR_EMULATE_PREPARES: false
 PDO::ATTR_ERRMODE: PDO::ERRMODE_EXCEPTION

And so on, and so forth.. just check how existing DBAL's are doing things. This enables you to write your code like so:

$db = new \PDO(
 Config::get('databases')->get('alias')->dsn,
 Config::get('databases')->get('alias')->user,
 Config::get('databases')->get('alias')->pass,
 Config::get('databases')->get('alias')->attr
);

Or, you can add a Facade or factory-like class (preferably a final class!) that creates the connections and stores them. Do add an additional parameter, though, in case you need a second connection using the same parameters (so you can call setAttribute on the PDO instance without it having repercussions on the rest of the application):

public function getConnection($name, $forceNew = false)
{
 if ($forceNew === true)
 {
 return new PDO();//<-- pass config arguments like above
 }
 if (!isset($this->databases[$name]))
 {
 $this->databases[$name] = new PDO();//<-- again, config...
 }
 return $this->databases[$name];
}