What happens if you change your database credentials?
Right now your class is tied to a specific set of database credentials, and you'll need to change it if:
- you want to use a different engine,
- or a different host,
- or a different user,
- or a different password,
- or any combination of the above.
This is why PDO requires you to pass the dsn and the credentials through its constructor, and you should do the same. Then of course your class wouldn't be much different than PDO itself, if you don't have any additional functionality, just forget about it and use PDO.
If you want to keep your class, just store your credentials in a configuration file. A simplistic solution would be an INI file, let's call it database.ini:
engine = "mysql"
host = "localhost"
name = "test"
user = "root"
password = ""
Your database class could then be:
class DB {
private $pdo;
private $engine;
private $host;
private $db_name;
private $db_user;
private $db_passwd;
public function _construct($path) {
if(!is_file($path)) {
throw new InvalidArgumentException("Hey, that's not a file!");
}
$credentials = @parse_ini_file($path);
if(empty($credentials)) {
throw new InvalidArgumentException("Hey, that's a file, but it's not a valid INI file!");
}
$this->engine = $credentials["engine"];
$this->host = $credentials["host"];
$this->db_name = $credentials["name"];
$this->db_user = $credentials["user"];
$this->db_passwd = $credentials["password"];
try {
$this->pdo = new PDO($this->engine.':host='.$this->host.';dbname='.$this->db_name,$this->db_user,$this->db_passwd);
$this->pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
return $this->pdo;
} catch(PDOException $e) {
throw new Exception("Boo, wrong credentials!");
}
}
}
Looks like a lot of work? Well, it is, and it's not finished, but changing any of your credentials is now as simple as changing its value in database.ini. All you need to do to instantiate your class is:
$db = new DB("database.ini");
Let's go quickly through the main parts of the class:
Don't protect what should be private
Don't declare your class properties and methods as protected if you don't need to. Protected means:
Members declared protected can be accessed only within the class itself and by inherited and parent classes
If you are not going to extend the class, there's absolutely no reason to go with protected instead of private:
Members declared as private may only be accessed by the class that defines the member.
This might seem extremely pedantic, the world won't stop spinning because your properties are protected, but since you're a fresher, better avoid this common bad habit.
Connect? Why, let's construct instead!
The database class is useless if we don't run connect(), calling the method is a prerequisite for the class to do anything else. When that happens, it's a good sign that our method should either be called in the class constructor, or be the constructor itself. If it's absolutely necessary for the method to be called, then it makes sense to be the one function that gets called by default when we create an instance of our class, doesn't it?
Check your arguments
I'm doing two checks on our $path argument, first if it's a valid file and then if parse_ini_file returned an array (that would signify that the INI file was succesfully parsed). Throw appropriate exceptions if there are problems.
The unfinished part
$this->engine = $credentials["engine"];
$this->host = $credentials["host"];
$this->db_name = $credentials["name"];
$this->db_user = $credentials["user"];
$this->db_passwd = $credentials["password"];
What I'm not doing here is check whether the indices exists in the $credentials array. That's your homework ;)
The exception
Your original code was:
} catch(PDOException $e) {
die($e->getMessage());
}
That doesn't make sense, you catch the exception and then die with the same message? Not catching the exception will have exactly the same effect, the script will stop, so why catch it? You should catch exceptions only when you can do something productive with them, if not, just let the script fail. Usually "something productive" means gracefully recover from the error, but that's not always possible. In my code I'm doing something that might seem minor but it's very important:
} catch(PDOException $e) {
throw new Exception("Boo, wrong credentials!");
}
Yes, the only thing I'm doing is changing the message of the exception, the script will still fail. However PDO's constructor exception reveals your database credentials (except your password), so catching that and rethrowing it with a more generic message minimizes the risk of your database username (for example) falling in the wrong hands. Can't gracefully recover from the failure, but at least we plugged a potential security hole.
How about some setters and getters?
If you are still reading this, I'm sorry to inform you that your code doesn't work. This:
public function __construct() {
$pdoObj = new DB();
$this->link = $pdoObj->connect();
return $this->link;
}
does set your database object in the $link property but it's your database object, not your PDO object. Your database object does not have a prepare() method, so this:
$query = $this->link->prepare("INSERT INTO users(username, email, password, ip_address, activation_code)
VALUES(:username, :email, :passwd, :ip_addr, :code)");
will simply fail. What you need to do is pass your PDO object to your User class:
class DB {
...
public function getPDO() {
return $this->pdo;
}
}
So this simple function will always return the database's internal PDO object. Your User constructor would now be:
public function __construct() {
$database = new DB("database.ini");
$this->link = $database->getPDO();
}
Notice how I removed return $this->link;? Well, don't do that, a constructor returns the object it instantiates by default, there's absolutely no sense in returning anything else, it wouldn't be a constructor if it didn't give us what it constructed, would it?
Wait! There's a dependency!
This line:
$database = new DB("database.ini");
means that we don't have the option of renaming our INI file. Ever! That's not good, that's an unwanted dependency and we really don't like those. After all we went through a lot to get our database class to not be dependent on its credentials, we can't introduce another dependency now. What we are going to do is use a method called Constructor Dependency Injection, that's the simplest way to achieve Dependency Injection. It's extremely simple, all we need to do is:
public function __construct(DB $database) {
$this->link = $database->getPDO();
}
That's right, pass the database as a parameter in the constructor, that's all there is to it. Now, to instantiate our user class all we need to do is:
$database = new DB("database.ini");
$user = new User($database);
...and our classes are dependency free! To get an idea why this is good, assume you have two databases, and you need to use them both. All you need to do is:
$database1 = new DB("database1.ini");
$user1 = new User($database1);
$database2 = new DB("database2.ini");
$user2 = new User($database2);
Does your database only have users?
If your database has any other table, and you want to model them as classes, it might be worth looking at inheritance. All your table classes have a common dependency, the database, and you don't really have to write the same constructor every time:
class Table {
protected $database;
public function __construct(DB $database) {
$this->database = $database->getPDO();
}
}
class User extends Table {
// only user specific functions here
}
class Article extends Table {
// only article specific functions here
}
class Comment extends Table {
// only comment specific functions here
}
The User, Article, and Comment classes have inherited the same constructor from Table, no need to write the method more than once. As you probably already noticed, I've declared $database protected, it makes sense now as we need it to be available to the child classes. By the way that's your old $link property, since it holds a database object, makes sense to call it $database.
The register function
Sorry for being blunt, but this is a horrible mess:
Indendation is messed up.
Learn to format your code properly. Better yet, equip yourself with a decent tool to do it for you.
Your function should either return something or echo something, not both.
Preferably return something. Echoing HTML from your functions is a very poor practice, read up on separation of concerns. Simply put your function is where you put logic, and you shouldn't mix presentation with logic.
You catch an exception and die with the same message again.
Don't do that.
Since we've now have a handy getter in our database class, the way to get the reference to the PDO object in your function is:
class User {
public function register($username, $email, $passwd, $re_passwd) {
$link = $this->database->getPDO();
...
$query = $link->prepare( ... );
...
}
}
And what about static?
Well, don't worry about it. Most of the time is used as a convenience, and it's not really the better way to do things. Instantiate your objects properly, and then pass them around via one of the dependency injection techniques, you're passing around the same instance, you are not instantiating it again and again.
Further reading
Addendum
Corbin raises two important issues in a comment:
(1) The exception shouldn't be masked. What if consuming code wants the details? A different part of code should be responsible for hiding potentially sensitive exceptions. (2) If Table (and other classes) need a PDO instance, they should accept a PDO instance, not a DB instance. There's no reason to accept the object just to call a single getter. (I also believe that this Database class shouldn't even exist though. It's pointless.)
My thoughts:
Your database class is pointless if it doesn't have any additional functionality you didn't tell us about.
As it is, it's just a wrapper for PDO that doesn't do anything more than PDO itself does, and in my answer I've assumed (hoped?) that you do have a reason for it and would further develop it to enhance PDO (perhaps adding a query cache, or a log?). But if you don't have any further plans for it, then it would be preferable to just scrap it and just use PDO and just feed your PDO object in Table::__construct().
You shouldn't be catching exceptions if you can't do anything productive, preferably gracefully recovering from the error.
Now, I do suggest that you should catch the PDOException just in case your database credentials are leaked, but that's more of a scare tactic than a solid suggestion. I've seen one too many new developers do similar mistakes, and perhaps my suggestion there was a bit drastic.
What you need to keep in mind is that this particular PDOException should not reach the end user. If there's other code in between your database instantiation and the end user (a top level script, perhaps), it would make much more sense to deal with the exception there, and if you can't recover from it do something like throwing a 404 or 503 error (whichever applies).
Take my suggestion to hide the details of the connection PDOException with a grain of salt, I'm just tired of dealing with the same crap again and again. There was a time when such an exception created a world of trouble for me, but in a typical production environment it shouldn't be easy for anyone to do anything particularly harmful just because they caught a glimpse of your database username.
