Want to check if I am using PDO correctly

Question

So today after few years of not programming in PHP I decided to go back, but to do so I needed to change all MySQL connections. So I want to check if I am using it good.

Earlier I used to write something like this:

$user = clean($_COOKIE["Logged"]);
$GameReg = mysql_fetch_array(mysql_query("SELECT * FROM DB.GameReg WHERE hash='$user'"));

But now it is like this:

$stmt = $dbh->prepare("SELECT * FROM DB.GameReg WHERE hash = :user");
$stmt->bindParam(':user', $user);
$user = $_COOKIE["Logged"];
$stmt->execute();
$GameReg = $stmt->fetch(PDO::FETCH_ASSOC);

So am I using it correctly? I used to do all kinds of filtering before inserting or updating SQL table, but I've read that prepared statements doesn't need that. Is this safe to use?

As I am knew to PDO I am trying to save up some code, so I kinda came up with this:

$stmt = $dbh->prepare("SELECT * FROM DB.GameStats WHERE hash = :user");
$stmt = $dbh->prepare("SELECT * FROM DB.GameReg WHERE hash = :user");

$stmt->bindParam(':user', $user);
$user = $_COOKIE["Logged"];

$stmt->execute();
$GameReg = $stmt->fetch(PDO::FETCH_ASSOC);

$stmt->execute();
$GameStats = $stmt->fetch(PDO::FETCH_ASSOC);

I checked this by trying to call some table members:

echo $GameReg["pass"];
echo "\n";
echo $GameStats["nick"];

It works, but I am not sure if this is the best way to do. Any comments?

Answer 1

Your code is fine. Really. However, it does show some lack of understanding the true benefits and power of prepared statements.
I'm also not convinced you fully understand how the PDOStatement::bindParam method works. Allow me to explain:

A prepared statement is sent to the DB server as a string, with the placeholders that will be replaced with the query values. This query string is already parsed, compiled and optimized at this point, in case it is possible, the result is already being stored in a temp-table, too.
At this point, you have a resource handle to this prepared statement, assigned to the variable $stmt. The really neat thing is that this statement can be used as much as you like, each times specifying new parameters, for example:

$stmt = $pdo->prepare('SELECT age, sex, nationality FROM people WHERE name = :name');
$names = array(
    'Frank',
    'Lisa',
    'John',
    'Jane'
);
$bind = array(
    ':name' => null
);
$personalia = array();
foreach ($names as $name)
{
    $bind[':name'] = $name;
    $stmt->execute($bind);
    $personalia[$name] = $stmt->fetch(PDO::FETCH_ASSOC);
}

As you can see, I only call prepare once, but execute 4 distinct queries. The advantage then, is simple: the query only has to be parsed, compiled and optimized once, and in some cases, the result set can be pre-fetched in such a way that you'd never need to search through the entire tbl.
Bottom line: prepared statements not only provide security against injection, it often outperforms regular queries, too.

You may have noticed how I used execute(array), and not bindParam, like your code does. Though sometimes I do use bindParam, I find it best to avoid this method when I can, in favour of execute(array), or if needs must PDOStatement::bindValue. As to why, we need to look at the method's signature:

public bool PDOStatement::bindParam ( mixed $parameter , mixed &$variable [, int $data_type = PDO::PARAM_STR [, int $length [, mixed $driver_options ]]] )

The $parameter argument is the Parameter identifier, or placeholder. In this case, the string :name.
The $variable argument is not the value that will be used in the query but if you look closely, the parameter is passed by reference: &$variable <-- that's what the & operator is for. Using it yourself is, arguably, bad practice, but no matter how you look at it, it does make your code harder to read and maintain. Imagine I did this, for example:

$stmt->bindParam(':name', $name);
while($name = array_pop($names))
{
    $stmt->execute();
}

This should work just fine, but what if $names changes, or someone re-assignes $name at some point as the while loop becomes more complex? You end up with quite error-prone code.

The advantage of this method, then, is the third parameter: $data_type. It allows you to specify the data-type you want to pass to the query. using the PDOStatement::execute(array) method, all parameters will follow the default behaviour, which means they'll be passed as strings. MySQL is capable to cast those string values to the appropriate types most of the time. As always, a cast implies some overhead, but it's highly unlikely that this will be a bottleneck and most of the time, this'll do just fine. It's an old quote, but keep in mind that premature optimization still is the root of all evil. Be that as it may, when dealing with values like NULL or booleans, it can sometimes come in handy to pass specific types.
Still, for those cases, PDOStatement::bindParam shouldn't be the default method to turn to. As you can see from the method's signature, it's powerful, but complex. Bugs and complexity go hand in hand, so let's look at another method, that does allow you to specify specific types: PDOStatement::bindValue. This method does the same thing as PDOStatement::bindParam, but does not use references (that's one less source of woes you don't have to think about), and doesn't take as many arguments:

public bool PDOStatement::bindValue ( mixed $parameter , mixed $value [, int $data_type = PDO::PARAM_STR ] )

If all you need is to specify a specific type, then this method is to be preferred: it allows you to specify all the types, but without the added risk of passing references around.

To recap: Unless you're looking to optimize your code to save every millisecond you can, or you need to pass some driver-specific options, I'd use bindValue rather than bindParam. Which, again, makes your code less error-prone and generally easier to maintain.

In short, then, and applied to your specific code, I'd recommend you write something like:

$stmt = $pdo->prepare(
    'SELECT * FROM DB.GameReg WHERE hash = :user'
);
$stmt->execute(
    array(
        ':user' => $_COOKIE['Logged']
    )
);
$gameReg = $stmt->fetch(PDO::FETCH_ASSOC);

Which gives you spacious, easy to read and maintain code.
If you want to be able to specify the type, and need nothing more, then use bindValue.

If you want to call a stored procedure, that uses an INOUT argument, then you have to resort to `PDOStatement::bindParam:

$stmt = $pdo->prepare(
    'CALL your_sp(:io)'
);
$was = $io = 'hello';
$stmt->bindParam(
    ':io',
    $io,
    PDO::PARAM_STR | PDO::PARAM_INPUT_OUTPUT,//string, inout argument
    6
);
//example:
$stmt->execute();
echo 'Argument was: ', $was, PHP_EOL, 'Now is: ', $io;

Could echo something like:

Argument was: hello
Now is: world

Answer 2

It's good that you're moving over. Many people think it's too difficult to switch over, but in the long run, it's definitely worth it.

Technically you can assign $user after you bind, because it's the actual execute call that substitutes. Although, in my opinion, it's easier to read when scanning the code to see the assignment before the binding. You could also do it like so:

$stmt->execute(array(':user' => $user));

However, it's even safer to apply a data type, so that if $user is an unacceptable type (assuming it's supposed to be a string), it returns false. Like so:

$stmt->bindParam(':user', $user, PDO::PARAM_STR);