PHP MySQL connection class

Question

I'm trying to build a class that connects to the database. It does the job but it is not very elegant. I also have no understanding of singleton class or dependency injection.

Is this safe? Can you explain how I can make this more elegant in simple language?

Also, I use the $logs array instead of commenting. I got some remarks on it, while it makes debugging easy for me (I call print_r() on the logs of certain classes during development). What do you guys think?

<?php
  class database{
    private $host = DB_HOST;
    private $user = DB_USER;
    private $pass = DB_PASS;
    private $name = DB_NAME;

    public $db_connection = false;
    public $logs = array();

    public function __construct(){
      $this->logs[] = "Checking if the database is already open.";
            if($this->db_connection != false){
                $this->logs[] = "The database connection is already open.";
            }
            else{
                $this->logs[] = "The database connection is not already open.";
                try{
                    $this->db_connection = new PDO('mysql:host='. DB_HOST .';dbname=' . DB_NAME .';charset=utf8', DB_USER, DB_PASS);
                    $this->logs[] = "A new database connection has been established.";
                }
                catch (PDOException $e){
                    $this->logs[] = "A new database connection could not be established.";
                }
            }
    }
  }
?>

I use the class like this:

if($db_connection){
 $query_search = $db_connection->prepare('SELECT * FROM ...');
 $query_search->bindValue(':sqlvar', $phpvar, PDO::PARAM_STR);
 $query_search->execute();
 $query_result = $query_search->fetch();
}
else{
 // Failed connection
}

Answer 1

From the things I see in your code it looks to me like you are quite new to Object Oriented Programming (OOP), so I will try to keep that in mind during this review.

Basic OOP

The __construct() method of a class is called only once per object of that class, when it is instantiated like so: $db = new Database(). Your class contains a field $db_connection, which default to false. Since the __construct() method is called only when the object is being instantiated, there is no reason to check if ($this->db_connection != false), since $db_connection will always be false for that object.

Static vs non-static

It seems to me like you are misunderstanding how regular class fields and static class fields work. Lets do a quick example based on your code:

$database1 = new Database();
$database2 = new Database();

I now have two instantiated Database objects. Lets assume that when I created $database1, the connection succeeded, so a PDO object will be assigned to $db_connection:

// Will print some stuff about the PDO object
print_r($database1->db_connection);

However, when creating $database2, the $db_connection field will be set to false for $database2, because it is in no way related to the $db_connection field of any other instantiated Database object.

If you were to make $db_connection a static field, then they would be shared between all class instances, because it would be a field on the actual class itself, and not on the instantiated objects.

Using fields

What else is weird is that you assign the global variables DB_HOST, DB_NAME, DB_PASS and DB_USER to some private fields of the class, and then in the constructor you do not use the private fields, but you use the global variables again. This is very unusual and should definitely be removed like so:

$this->db_connection = new PDO('mysql:host='. $this->host .';dbname=' . $this->user .';charset=utf8', $this->name, $this->pass);

Now that we have that out of the way, you should not be setting private fields of a class based on some global variable that might be there. You should change it so they have to be passed to the constructor like so:

public function __construct($host, $user, $password, $name) {
    // Use the variables provided as arguments to the constructor here
}

This makes your class less dependent on your specific setup and global variables and allows for easier reuse and testing of your class. If you for whatever reason need to setup a connection to another database, you can reuse the class with different parameters. You can now use it like this:

$database = new Database(DB_HOST, DB_USER, DB_PASS, DB_NAME);

Furthermore, now that these values are passed as arguments to the constructor, you no longer need the fields to store them in, because you only use them in the constructor!

At this stage into the code review, your class would look something like this:

<?php
class database {
    public $db_connection = false;
    public $logs = array();

    public function __construct($host, $user, $pass, $name) {
        $this->logs[] = "Attempting to connect to the database.";

        try{
            $this->db_connection = new PDO('mysql:host='. $host .';dbname=' . $name .';charset=utf8', $user, $pass);
            $this->logs[] = "A new database connection has been established.";
        }
        catch (PDOException $e) {
                $this->logs[] = "A new database connection could not be established.";
        }
    }
}

One small nitpick, you should get into the habit of not ending PHP files with the ?> closing tag. See this question for an explanation.

Logging with interfaces

One final thing I will just briefly mention: You shouldn't be logging like this. You should be logging to an interface. Consider the interface:

interface Logger {
    public function log($data)
}

Now your Database class could take an optional Logger object in the constructor like so:

public function __construct($host, $user, $pass, $name, Logger $logger = NULL) {
    $this->logger = logger;
}

Now you can replace $this->logs[] = "..."; with $this->logger->log("...");. The beauty of this is that your Database class no longer cares how the data is being logged, only that it is being logged. You could create a ConsoleLogger that logs to the console or a FileLogger, that logs the data to a file. Any of these could be passed to the constructor of a new Database object, as long as they implement the Logger interface.

Edit: The Singleton

Since you verified that you attempted to make a singleton, I will talk about singletons for a bit. By definition, a singleton is a class that can only be instantiated once. What does this mean? This means that you should not be able to do this:

$db1 = new DatabaseSingleton();
$db2 = new DatabaseSingleton();

You have to ensure that only one instance of this class is made. You might get sneaky and attempt to do something like this:

// Don't do this!
class DatabaseSingleton() {
    private static $instance_count = 0;

    public function __construct() {
        if (self::$instance_count > 0) {
            throw new Exception("This singleton has already been instantiated!");
        }

        self::$instance_count += 1;
    }
}

This would technically do the trick, as a second instantiation of the class would throw an exception. Notice how I have used a static variable to achieve this.

The "singleton" above is actually a very bad implementation and should be avoided. The generally accepted way of creating a singleton in PHP looks something like this:

public class DatabaseSingleton {
    private static $instance;

    private __construct() {

    }

    public function getInstance() {
        if (empty(self::$instance)) {
            self::$instance = new DatabaseSingleton();
        }

        return self::$instance;
    }
}

A few clever tricks have been used here. First of all, I have made the constructor private! This means that the constructor can only be called from within the class itself, which sounds weird, but is the key to singletons. It means you can no longer do this outside of the DatabaseSingleton class:

// An error occurs because the constructor is private
$db = new DatabaseSingleton();

I have also created a public and static getInstance() method. This method first checks if the static field self::$instance already contains something. If it does not, then it instantiates a new DatabaseSingleton object and stores it in the self::$instance field. Finally it returns the field. The second time this method is called, the if statement will be skipped because self::$intsance is no longer empty.

The instance can now be retrieved as follows from anywhere in the code:

$db = DatabaseSingleton::getInstance();

Tada! There you have your singleton. But notice how I said anywhere. This basically means that you now have a global variable, but enclosed in a class. Singletons can sometimes be useful, but you should think real hard about whether or not you really need/want one before using this design pattern.

I hope this little extra section was informative for you!

Answer 2

The answer provided by @ThijsRiezebeek gives really good information about moving your class towards using a singleton pattern. Since you mentioned dependency injection (though you are not currently using it), I figured I would provide some commentary more geared towards that approach, which many people would prefer over using something like singleton (especially in complex applications).

With dependency injection, you have to really embrace a different approach with regards to how you deal with dependencies. Rather than having calling code or classes need to instantiate or create their dependencies (for example a database connection), you look to instead make the dependency available to the code that needs it as part of the executing that section of code. Take for example a class that needs a database object to interact with the database to do something like interact with a user record.

An approach that does not use dependency injection may need to instantiate the database object itself in a constructor. Let's give an example that utilizes a singleton to provide the DB object.

class User {
    private $pdo = null;
    // other user properties here

    public function __construct($user_id) {
        // get db
        try {
            // set PDO object reference on this object
            $this->pdo =  PDOSingleton::getInstance();
        } catch (Exception $e) {
            error_log('Unable to get PDO instance. Error was: ' .
                $e->getMessage();
            // perhaps rethrow the exception to caller
            throw $e;
        }
        // query DB to get user record
    }

    // other class methods
}

// example usage
$user = new User(1);

When using dependency injection, that might instead look like this:

class User {
    private $pdo = null;
    // other user properties here

    // pass PDO object to the constructur. Enforce parameter typing
    public function __construct($user_id, PDO $pdo) {
        $this->pdo = $pdo;
        // query DB to get user record
    }

    // other class methods
}

// example usage
// instantiate PDO object. This probably happens near beginning
// of code execution and might be the single instance you pass around
// the application
try {
    $pdo = new PDO(...);
} catch (PDOException $e) {
    // perhaps log error and stop program execution
    // if this dependency is required
}

// somewhere later in code
$user = new User(1, $pdo);

This may seem like only a subtle difference, but developers who use this approach like it because it:

• decouples the consuming class from the details of how to instantiate the dependency. Why should the user class have to know which singleton to use in order to get it's PDO dependency? All the class should care about is knowing how to work with the dependency (i.e what properties and methods it has), not how to create it. This more closely follows the single responsibility principle which is typically desired in OOP, as the user class only has to deal with instantiating a user representation, not also with instantiating its dependencies.
• eliminates duplicate code across classes that need the dependency as you don't need all the handling in each class around instantiating/creating the dependency. In the example code, I eliminate the need to potentially handle failed DB instantiation within the constructor, as I know I already have a valid PDO object passed as parameter (if I did not get one passed, I would get invalid argument exception).
• unit testing the code becomes easier as you can simply use a mock dependency to emulate the dependency and you are no longer writing unit tests that have to cover the code paths involved with instantiating dependencies.