PDO MySQL class

Question

I'm new to using PDO and am trying to create a secure class to handle MySQL statements. Here is what I have so far:

class SQL {
private static $dbh = null;

private function __construct($host, $user, $pass, $data, $table) {
    $this->host = (!empty($host)) ? $host: 'localhost';
    $this->user = $user;
    $this->pass = $pass;
    $this->data = $data;
    $this->table = $table;
    static::connect();
}
private function __destruct() {
    static::close();
}
public static function connect() {
    $dsn = 'mysql:dbname=' . $this->table . ';host=' . $this->host;
    static::$dbh = new PDO($dsn, $this->user, $this->pass);
    static::$dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}
public static function close() {
    static::$dbh = null;
}
public static function query($query) {
    $sth = static::$dbh->prepare($query);
    $sth->execute();
}
}

Is what I've done correct so far and conform to best practices?

Answer 1

Disclaimer: I've not tackled SQL or PDO as of yet, so this answer will focus mainly on the code itself.

You should not set $this->data in the __construct() method unless you are planning on using it. I would set it in whatever method you use to add that data. I am assuming this class is incomplete because I did not find it being used anywhere (maybe I missed it?). Doing this you won't have to hunt for that variable later when trying to debug it.

If the only time you are going to use your connect() and close() methods are going to be in the class, you should make them private. Which probably should be the case anyways. Generally you want full control of any instances of a database, which means making all calls to it within your control, and the best way to ensure this is to make it private. Allowing a user to possibly control its state is asking for trouble. If you need to open another database, which is the only reason I can think of for making it public, then you should simply create a new object for it. This will also make having these methods static quite unnecessary.

Why are you declaring $dbh static? Just make it a private variable and call it with $this. Maybe there is a reason for doing it this way, but I wouldn't know it. I tend to steer clear of the static keyword.

Final thoughts. Your variables should be easily identifiable. Yes, I understand what a $dbh is, but it should be clear for those who don't. I would simply call it something like $handle or if I might end up using multiple handles $db_handle. But $dsn and $sth (statement handle?) kind of have me stumped. Make them clear so that people reading your code at a later date will know what they mean. If you don't wish to change the variable names themselves, then at least use PHP Doc notes to describe what they are.