Saving and uploading images in a database

Question

I am working on a small method that saves uploaded images and records the images in a database.

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?

Answer 1

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

Answer 2

I don't think your code will work as expected. This:

        foreach($_FILES['event-images']['tmp_name'] as $tmp_name){

tells me that $_FILES['event-images']['tmp_name'] is an array (i.e., you're handling multiple files upload). But, in that case, $_FILES['event-images']['name'] should also be an array, so the concatenation will fail here:

            $imgName = $this->imgPath . $this->random_name . $_FILES['event-images']['name'];

As for PDO, why do you prepare the same SQL query over and over again? I'd put the prepare statement before the loop, so that this work is done only once per upload.