PDO insert from check boxes

Question

I've seen so many different ways of doing this all seem very complex and all seem to be done in MySQL. I'm using PDO in my project so I've had a go myself to see what I could come up with.

1. Is this safe?
2. Is there something I've missed a better way of doing it?

 //build query to insert users_contracts
$sth = "INSERT INTO `users_contracts` (users_id,contracts_id) VALUES (:user,:contract)";
$q = $conn->prepare($sth);
//take each item in the contract array from _POST and bind to the above INSERT query
foreach($contract as $contract => $contract_id)
{
 try {
 $q->execute(array(':user'=>$last_id,':contract'=>$contract_id));
 } catch (PDOException $e) {
 $errors[] = array(
 'message' => "Could not insert \"$contract\", \"$contract_id\".",
 'error' => $e
 );
 } 
}

Its intended purpose is to allow the admin user to assign contracts to users via a checkbox list.

Answer 1

There are a couple of minor things I'd suggest you look into:

• Names matter: you're assigning the query string to a variable called $sth, and assinging the prepared statement to a variable called $q. To me, that looks wrong, and it should be the other way around. A variable called $q or $query should be a query string, and a variable called $sth or $stmt should be a prepared statement.
• Transactions: if, somewhere down the line, one of the INSERT queries failed, I wouldn't carry on. To me, that smells of bad/invalid/malicious data, and I'd want to stop inserting and even undo whatever inserts that were already executed. By starting a transaction before the loop, and committing it at the end, you can do this easily
• Because you are re-using a statement several times (which is good), you might want to add a closeCursor call. The MySQL drivers don't really care about this, but some other drivers do. Depending on what DB you're using, your code might behave differently if you leave it out.

Basically, using bindParam as Pinoniq suggested is a good idea. However, make sure $last_id stays valid. It's not being altered in your loop, so make sure the code between bindParam and the execute call doesn't do anything untoward with that variable.
All things aside, your code could be made to look something like this:

$query = 'INSERT INTO users_contracts (users_id,contracts_id)
 VALUES (:user,:contract)';
try
{//begin try-block here
 $conn->beginTransaction();//start the transaction
 $stmt = $conn->prepare($query);
 $stmt->bindParam('user', $lastId);
 $stmt->bindParam('contract', $contractId);
 foreach($contract as $contract => $contractId)
 {
 $stmt->execute();
 $stmt->closeCursor();//<-- doesn't hurt
 }
 $conn->commit();//everything went smoothly, save inserts
}
catch (PDOException $e)
{
 $conn->rollBack();//undo changes!
 //handle error
 printf(
 '%d - %s insert failed: (%d) %s',
 $lastId,
 $contractId,
 $e->getCode(),
 $e->getMessage()
 );
}

Answer 2

Instead of calling bindParam everytime in the for loop. You could make use of the late-binding that PDO does with bindParam. You actually bind a reference. On execute, that reference is evaluated and added to the query. So to clean up your code:

$sth = "INSERT INTO `users_contracts` (users_id,contracts_id) VALUES (:user,:contract)";
$q = $conn->prepare($sth);
$q->bindParam('user', $last_id);
$q->bindParam('contract', $contract_id);
//take each item in the contract array from _POST and bind to the above INSERT query
foreach($contract as $contract => $contract_id)
{
 try {
 $q->execute();
 } catch (PDOException $e) {
 $errors[] = array(
 'message' => sprintf('Could not insert %, %.', $contract, $contract_id),
 'error' => $e
 );
 }
}

I also used sprintf in your error message. Makes for easier reading, and in time. You could change the text to a call to some Language function:

sprintf(Lang::get('error.something.wrong'), $contract, $contract_id);

• Pinoniq: check the reply of Elias for a better error message ;)

Answer 3

Security

You should use htmlspecialchars around $contract and $contract_id when storing the error message. Even if you are not displaying it to the admin in a webpage right now (but for example in an error log or the database), this might be a feature added in the future, and then it might cause xss attacks.

Other than that, your code is probably save. When creating your $conn object, you should check if you are setting PDO::ATTR_EMULATE_PREPARES=>false, and maybe also PDO::MYSQL_ATTR_INIT_COMMAND=>'SET NAMES utf8', (with your charset) (see this answer for an explanation).

Naming

You could use more expressive variable names, but at this scope it's not that bad. But $sth could be $insert or $insert_statement, $q could be $query, and :user could be :user_id (as it's an id. same with :contract).

Formating

More spaces is almost always a good thing. I would surround => with spaces and put a space after ,.

Other than this, your code looks good.