First things first: You want to know if you're doing things the right way. To that, the answer would be No, you're not. And here's why I say this:
- for a kickoff, you say you're learning PDO, and want to use it in an Object Oriented way. But
PDO stands for PHP Database Object. It's already Object Oriented. It's API is clean, consistent and the design complies with the S.O.L.I.D. principles quite nicely. It's imposiburu to wrap it, extend it or abstract it and get something better. The only thing that you can do with it is to use it as a base/core for a DBAL (database abstraction layer) or in an ORM. Check this answer of mine for more details
- I've mentioned the S.O.L.I.D. principles already. Read what they're all about. They're not some abstract in-an-ideal-scenario kink of pattern or design philosophy. They're really common-sense principles to keep in mind and adhere to when writing code. Following them makes sense, it's easy and makes your code more flexible, easier to maintain and more portable
- Like I've often said in previous reviews: A class should never, ever, control the flow of the application. A class that handles database connectivity should NEVER generate direct output. It's part of the application's inner workings. It should communicate to the code that calls it, not to the client that sent a request to the server. Therefore, your
Connection class's methods have to be free of echo or print statements. Methods return or throw, then never echo
- Just like any ordinary method, a constructor mustn't
echo, but because of the job a constructor does, the rules are even more strict: if a constructor returns something (explicitly, or most commonly implicitly), its return value is an instance of the class. If the constructor encounters an error (like in your case: failure to connect to the DB), the class can't be constructed. This normally shouldn't happen, but if it does, you're in uncharted territory. It's an exceptional event, and this an Exception must be thrown. You, however, catch an exception and simply echo the error message. There's no stack trace, no error code, nothing... all you end up with is a client that gets an obscure DB error message, telling him a couple of things: what language is running server-side, what DB you're connecting to and possibly an indication as to how you're connecting and why it didn't work. That's a security issue, by anyones standard
- Coding style is important. It may not seem that important at first, but trust me, it's worth the effort to conform to the most common PHP coding style. There is a standard, put forth by PHP-FIG. All major players (Zend, Symfony, CakePHP, Apple, ...) adhere to it. So should you.
- I've covered this (quite extensively) in the answer I linked to in my first point of this list, and it is a bit of a dead-horse but: Scalability. The code you have now does not allow you to easily implement things like: re-use of prepared statements or working with transactions.
I'll post an update in the very near future, actually reviewing your code, but for now this list (and the links) should keep you busy for the time being...
On to the first bit of code, which I'll cover line by line (or almost line by line):
class Connection {
Yes, I have something to say about the first line already. 2 things, actually. The first is coding standards: PHP-FIG states that the opening brackets of class, interface, trait and method definitions go on the next line.
In this case, I'd also add that this class is clearly meant to be extended from, and you shouldn't be able to initialize it. Simply because initializing it would be quite pointless (there's no other methods to do something with the instance). So I'd declare it as an abstract class.
protected $host = "localhost";
protected $dbname = "pdo";
protected $user = "root";
protected $pass = "";
protected $DBH;
Declaring properties up front is good. Great, even. And you've specified the sensible protected access modifier. Nice, but why are you assigning connection parameters as defaults? Shouldn't the user be able to determine how he wants to connect to which DB, as what user? And shouldn't he be able to set attributes for the PDO instance? Look at the documentation: the user should be able to control all of the arguments that the PDO constructor accepts. Which is exactly what a constructor is for, which brings us to:
function __construct() {
try {
$this->DBH = new PDO("mysql:host=$this->host;dbname=$this->dbname", $this->user, $this->pass);
}
catch (PDOException $e) {
echo $e->getMessage();
}
}
Like I said, the try-catch is evil. If PDO throws an exception, then echoing it won't help. The code that creates the instance of your class knows what arguments it passed, and so you can (and should) assume that that code can deal with the exception better than you can. Therefore: don't catch it. That's not your job. Your constructor should aslo take arguments that let the user connect to the DB he wants, not the one that is determined by the properties.
PS: I'm not going to mention it any more, but coding standards...
Now the last bit:
public function closeConnection() {
$this->DBH = null;
}
}
This looks to be a bit of a pseudo-destructor. There's nothing wrong with the code itself, but really, if you look at how PDO works, you should know that PDO returns resultsets and prepared statements as instances of the PDOStatement class. If you're going to close the connection, you might have some code out there that represents a prepared statement (as a PDOStatement) that might be re-used at a later point. Prepared statements might (depending on the adapter) require PDOStatement::closeCursor to be called, but what's more: if you destroy the DB connection, how would a piece of code that is re-using a prepared statement supposed to know about it?
Honestly, there's no easy solution to this problem: you're either going to have to abstract the entire PDO interface away from the user (like Doctrine does), or you should ditch this method.
In the end, I wouldn't recommend using the kind of code to anyone, but if I were to be given the task to rewrite your code, it'd probably end up looking something like this:
abstract class Connection
{
/**
* @var string
*/
protected $host = "localhost";
/**
* @var string
*/
protected $dbname = "pdo";
/**
* @var string
*/
protected $user = "root";
/**
* @var string
*/
protected $pass = "";
/**
* @var PDO
*/
protected $DBH = null;
/**
* @param array $dsnParams
* @param string $usr
* @param string $pass = ''
* @param array $attributes = null
* @throws \InvalidArgumentException
*/
function __construct(array $dsnParams, $usr, $pass = '', array $attributes = null)
{
$requiredParams = ['type','host']
foreach ($requiredParams as $key)
{//these keys must exist
if (!isset($dsnParams[$key]))
throw new \InvalidArgumentException(
sprintf(
'%s key must be set',
$key
)
);
}
$dsn = $dsnParams['type'].':host='.$dsnParams['host'];
$optional = ['dbname','charset'];//add more if needed
foreach ($optional as $key)
$dsn .= ';'.$key.'='.$dsnParams[$key];//construct dsn string
$this->DBH = new PDO(
$dsn,
$usr,
$pass,
$attributes
);
}
}
Am I happy with this code? Not really, is it better than what you've posted? IMHO, it is. It has doc-blocks to tell users what property will hold what. It throws exceptions and it allows me to configure the DB connection in the way I want it to. But the problem with that is still the same:
Why would I pass all of the information required to instantiate PDO to this class, instead of creating the instance myself?
