3
\$\begingroup\$

I've been looking for a way to make the database connection easy available directry from any script, and from inside other classes. And since I tend to have different connections based on what action to execute, I had to find a way to easily scale the amount of connection needed.

Here's the solution I come up with.

I created this class:

class database{
    protected static $details = array();  //  holds all connection(s) details
    protected static $con = array();  //  holds the actual PDO connection(s)

/**
 *  addConnection()
 *  Adds a new connection
 *  @param string $alias a unique identifier for the connection
 *  @param string $db the database name
 *  @param string $un username
 *  @param string $pw password for the user
 *  @param string $desc a short description for this connection
 */
    public static function addConnection($alias,$db,$un,$pw,$desc){
        self::$details[$alias]=array('alias'=>$alias,'db'=>$db,'un'=>$un,'pw'=>$pw, 'desc'=>$desc);
    }

/**
 *  getConnection()
 *  @return array of a specific connection if a valid alias is provided. Othervise return a multidimensional array of all connections.
 */
    public static function getConnections($alias=''){
        if(!empty($alias)) return self::$details[$alias];
        else return self::$details;
    }

/**
 *  setConnection()
 *  This function moves the connection to $con[] for easy access both inside and outside of this class
 *  @param string $alias a unique identifier for the connection
 *  @param string $connection the string of a new pdo connection
 */
    public static function setConnection($alias,$connection){
        self::$con[$alias] = $connection;
    }

/**
 *  useConnection()
 *  @param string $alias a unique identifier for the connection
 *  @return a specific PDO connection based on the given alias
 */
    public static function useConnection($alias){
        return self::$con[$alias];
    }
}

And this script to set/initiate multiple connections:

if(database::getConnections()){
    try{
        $dbh_options = array(
            PDO::ATTR_EMULATE_PREPARES=>false,
            PDO::ATTR_DEFAULT_FETCH_MODE=>PDO::FETCH_ASSOC,
            PDO::MYSQL_ATTR_INIT_COMMAND=>'SET NAMES utf8',
            PDO::ATTR_ERRMODE=>PDO::ERRMODE_SILENT,
        );
        foreach(database::getConnections() as $dbh){
        #   initiate connection  
            database::setConnection($dbh['alias'],new PDO('mysql:host=localhost;dbname='.$dbh['db'].';charset=utf8',$dbh['un'],$dbh['pw'],$dbh_options));
        //
        #   put the connection to a an array for easy access when coding
            $con[$dbh['alias']]=database::useConnection($dbh['alias']);
        //
        }
    } catch(PDOException $e) {
        file_put_contents('PDOException.txt',$e->getMessage(), FILE_APPEND);
        die('OBS! Something went wrong connecting to your database.<br>Please see PDOException.log for more details.');
    }
}

Here's how I add new connections:

database::addConnection('alias','database','username','password','a optional short description');
database::addConnection('alias2','database','username','password',null);

Amd this is how I can use the connection from whitin another class:

$select = $con['alias']->prepare('SELECT...');
$insert = $con['alias2']->prepare('INSERT INTO...');

And from other classes, I just extends the database class.

class sql extends database {
    public static function query($id){
        $query = self::$con['alias']->prepare('SELECT column FROM table WHERE id = ?');
        $query->execute(array($id));
        return $query->fetchColumn();
    }
}

But I would really like your oppinion on this. If there's any security issues with this setup?

\$\endgroup\$
1
\$\begingroup\$

You have some issues in code design and codestyle:

  • All class names must be Capitalized
  • Never use self::$field if $field is protected, use static::$field instead. If you use self child classes won't have access to redefine field!
  • Never use die() you have exceptions for this!
  • Save process time use if ($con = database::getConnections())
  • Class containing only static fields is useless and fully procedural-style, create Singleton or Registry instead.
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Please, don't promote the pattern of death (Singleton)... Don't suggest it, don't even mention it, unless you're advising against it \$\endgroup\$ – Elias Van Ootegem Oct 1 '14 at 11:46
  • \$\begingroup\$ @EliasVanOotegem Ok, you can use Singleton for single instance and not using it globaly :) \$\endgroup\$ – smt Oct 3 '14 at 12:11
  • \$\begingroup\$ A static is, by definition global. You can access its value anywhere, any time. The only difference is you have to add the class name: Foo::$SomeStatic;. Due to PHP's memory management (ref-counting), statics are never GC'ed either: they stay in memory to the very end, just like globals do. Honestly: not using a singleton globally doesn't mean it isn't a global value (albeit in OO drag) \$\endgroup\$ – Elias Van Ootegem Oct 3 '14 at 15:47
  • \$\begingroup\$ @EliasVanOotegem You can create "protected singleton" in some far namespace like \Database\Management\Private(in Java it usually looks like private subclass), then create Database class and delegate all functional to that singleton, this allows to save memory, add another abstraction layer to your code, also you can check refs count on destruct to prevent handling redundant instance and add lazy-loading by creating initialize method for singleton. \$\endgroup\$ – smt Oct 6 '14 at 16:58
  • \$\begingroup\$ You can't apply Java-logic to PHP. Putting it in a far-away namespace won't make any difference: the moment the singleton is instantiated, you cannot release the instance unless you explicitly re-assign the static variable that holds the instance. Which in turn implies exposing the variable more, and effectively weakening the singleton itself \$\endgroup\$ – Elias Van Ootegem Oct 7 '14 at 7:27
1
\$\begingroup\$

Code gone wrong

The problem you are trying to solve is defined as:

I've been looking for a way to make the database connection easy available directry from any script, and from inside other classes.

Allready you are in trouble, even before starting to solve the problem. If you are using the word 'and' in the definition of your problem. You actually have 2 or more problems. So let's redefine the problems we have.

Managing dependencies

Or how to use the database from inside other classes

This is a problem easily solved. If you need it, ask for it. Let's say we have a MysqlWrapper. Without a connection, this class would be useless. So we pass it in in the constructor:

class MysqlWrapper
{

    /**
     * The connection used in this MysqlWrapper
     * 
     * @var PDO
     */
    private $connection

    /**
     * Set the connection
     * 
     * @param PDO $connection 
     */
    public function __construct(PDO $connection)
    {
        $this->connection = $connetion;
    }

    /**
     * Fine a row in a resource by it's primary key
     * @param  String $resourceName 
     * @param  String||int $id      
     * @return array
     * @return false    if no resource found
     */
    public function find($resourceName, $id)
    {
        $stmt = $this->connection->prepare('SELECT * FROM ' . $resourceName . ' WHERE id = :id');
        $stmt->bindParam('id',$id);
        $stmt->execute();

        return $stmt->fetch(PDO::FETCH_ASSOC);
    }

    /* rest of the class here */
}

Managing multiple connections

Apart from using our connections, we also need some kind of PDOConnectionManager:

class PDOConnectionManager
{
    private $connectionData = array();

    private $connections = array();

    public function create($name, $dsn, $user='', $password='')
    {
        $this->connectionData[$name] = array($dsn, $username, $password);
    }

    public function get($name)
    {
        //if the connection doesn't exist, try and load it
        if ( !isset($this->connections[$name]) ) {
            $this->loadConnection($name);
        }

        //return the connection
        return $this->connections[$name];
    }

    private function loadConnection($name)
    {
        if ( !isset($this->connectionData[$name]) ) {
            throw new InvalidArgumentException(
                sprintf(
                    'Connection data for "%"" not found'
                    $name
                );
            );
        }

        list($dsn, $username, $password) = $this->connectionData[$name];

        $this->connections[$name] = new PDO($dsn, $username, $password);
    }
}

Making stuff available

The Facade pattern (as used in laravel) is a nice pattern that maps static calls to a class onto an instance. The pattern looks something like this:

class Facade
{
    protected static $instance;

    public static function __callstatic($name, $arguments=array())
    {
        if ( !static::$instance ) {
            static::$instance = static::getInstance();
        }

        return call_user_func_array(array(static::$instance, $name), $arguments)
    }

    /**
     * this method should be implemented wy a less abstract Facade
     */
    protected static function getInstance()
    {
        throw new RuntimeException("Facade does not implement getIstance method.");
    }
}

and to finish, our databse facade:

class Database extends Facade
{
    protected static function getInstance()
    {
        return new PDOConnectionManager();
    }
}

We would use this Facade as:

Database::create('alias', 'mysql:...','username','password');
Database::get('alias')->find('user',1);
| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

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];
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ How about I change protected static $con = array(); to private, and then access it with the useConnection() method inside child classes? would that improve the script a little bit? I know it's not that much of an improvement like you're pointing out, but the values would not be overwritten anymore. \$\endgroup\$ – ThomasK Oct 1 '14 at 20:15
  • \$\begingroup\$ @ThomasK: The child can override a private property, and make it public (it can't reduce visibility, but it can expand it). But using methods to access the parent's private properties could work, provided those methods don't use late static binding (ie: use self::, but not static::), and the getter and setter methods are final \$\endgroup\$ – Elias Van Ootegem Oct 2 '14 at 6:59
  • \$\begingroup\$ I just turned my script into non-static. But now, when I'm trying to access the content of $con from a child class (using a method from the parrent like this: $this->con()), the method returns empty. That's probably another question I guess.. But I know there's content since when calling the con()-method from anywhere else there's content inside... \$\endgroup\$ – ThomasK Oct 2 '14 at 7:05
  • \$\begingroup\$ @ThomasK: I can't tell you why $this->con returns empty, for that, I'd need to see the code. So yes, that would be another question. Either way: it's good to know you've tried ditching the static's. And I hope you don't mind my asking, but all of the reviewers here have put in the effort. It would be nice of you to vote and/or accept the answer(s) you found useful (I'm guessing that's pretty much all of them :-P) \$\endgroup\$ – Elias Van Ootegem Oct 2 '14 at 7:17
  • \$\begingroup\$ Yep. You're right. Did that now :) thanks for telling me. Anyway. I just posted this question here: stackoverflow.com/questions/26156037/… \$\endgroup\$ – ThomasK Oct 2 '14 at 7:23

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.