patternphpMinor
Saving and uploading images in a database
Viewed 0 times
uploadingdatabaseandimagessaving
Problem
I am working on a small method that saves uploaded images and records the images in a database.
How can this be made more efficient?
public function setEventImages($event_id){
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 = $this->dbh->prepare("INSERT INTO adrenaline_junkies_uk_event_images (event_image_name, event_id) VALUES (?,?)");
$stmt->bindParam(1, $imgName, PARAM_STR);
$stmt->bindValue(2, $event_id, PARAM_INT);
$stmt->execute();
}
}How can this be made more efficient?
Solution
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
For a start, that saves you the trouble of that
Now, I do have some other niggles with your code, like this statement:
Now, I don't know how your
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:
Now this is looking better, I do believe. But like I said in the comments,
, which (in a loop) can lead to unexpected behaviour from time to time, so I'd rather pass my values to
But for completeness' sake, here's the same thing using
Lastly: You're using both
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 riskyCode Snippets
$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();
}$imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];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
}public bool PDOStatement::bindParam ( mixed $parameter , mixed &$variable [, int $data_type = PDO::PARAM_STR [, int $length [, mixed $driver_options ]]] )
^^ & => referenceforeach($_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));
}Context
StackExchange Code Review Q#31853, answer score: 5
Revisions (0)
No revisions yet.