Yes, there certainly is. Prepared statements can be reused, over and over again. The way your code works is: it's creating the same prepared statement over and over again, and binds the new parameters right after doing so, why not move the prepare
call outside of the loop?
$stmt = $this->dbh->prepare("INSERT INTO adrenaline_junkies_uk_event_images (event_image_name, event_id) VALUES (?,?)");
foreach($_FILES['event-images']['tmp_name'] as $tmp_name)
{
$imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];
move_uploaded_file($tmp_name, $imgName);
$stmt->bindParam(1, $imgName, PARAM_STR);
$stmt->bindValue(2, $event_id, PARAM_INT);
$stmt->execute();
//done for now, to re-use a statement, close its cursor first?
//though this is done implicitly, best do it ASAP
//especially when using SELECT + PDOStatement::fetch in a loop
$stmt->closeCursor();
}
For a start, that saves you the trouble of that prepare
call in the loop, increasing readability and, if all goes well, it'll also improve performance a bit.
Now, I do have some other niggles with your code, like this statement:
$imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];
Now, I don't know how your $_FILES
array is structured, but check some of the contributed notes on the man page there are quite a few snippets that deal with $_FILES
arrays recursively.
Another thing, and this is really a detail, is that you don't seem to be using transactions. When you're in the process of inserting images, one by one, and you reach a point where your script fails (either through timeout or another reason), you might end up with partially inserted data. That's not great. Transactions are an easy way to avoid this:
try
{
$this->dbh->beginTransaction();//start transaction here
$stmt = $this->dbh->prepare("INSERT INTO adrenaline_junkies_uk_event_images (event_image_name, event_id) VALUES (?,?)");
foreach($_FILES['event-images']['tmp_name'] as $tmp_name)
{
$imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];
move_uploaded_file($tmp_name, $imgName);
$stmt->bindParam(1, $imgName, PARAM_STR);
$stmt->bindValue(2, $event_id, PARAM_INT);
$stmt->execute();
}
$this->dbh->commit();//ok, all was well, commit inserted data
}
catch (PDOException $e)
{
$this->dbh->rollBack();//undo inserts, because not all of them were successful.
throw $e;//rethrow, let caller deal with the exception
}
Now this is looking better, I do believe. But like I said in the comments, bindParam
uses references:
public bool PDOStatement::bindParam ( mixed $parameter , mixed &$variable [, int $data_type = PDO::PARAM_STR [, int $length [, mixed $driver_options ]]] )
^^ & => reference
, which (in a loop) can lead to unexpected behaviour from time to time, so I'd rather pass my values to PDOStatement::execute
, if I were you:
foreach($_FILES['event-images']['tmp_name'] as $tmp_name)
{
$imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];
move_uploaded_file($tmp_name, $imgName);
$stmt->execute(array($imgName, $event_id));
}
But for completeness' sake, here's the same thing using bindParam
outside of the loop:
$stmt = $this->dbh->prepare("INSERT INTO adrenaline_junkies_uk_event_images (event_image_name, event_id) VALUES (?,?)");
$imgName = null;
$stmt->bindParam(1, $imgName, PARAM_STR);
//$event_id might fail, because this var is argument
$stmt->bindParam(2, $event_id, PARAM_INT);
//if it fails, try:
$eventId = &$event_id;
$stmt->bindParam(2, $eventId, PARAM_INT);
foreach($_FILES['event-images']['tmp_name'] as $tmp_name)
{
$imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];
move_uploaded_file($tmp_name, $imgName);
$stmt->execute();
}
Lastly: You're using both bindParam
and bindValue
as though they both do the same thing. Now at first glance, they do, but be weary of references. read this, to see why references in loops are risky