3
\$\begingroup\$

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?

\$\endgroup\$
  • \$\begingroup\$ Are you sure you want to store images in the database? I have always opted for storing the path to the image in the database, but left the images as flat files. There are plenty of reasons both for and against. (There are many debates about it on StackOverflow stackoverflow.com/questions/815626/…) \$\endgroup\$ – bumperbox Apr 21 '14 at 20:46
  • \$\begingroup\$ can you please post full code ? \$\endgroup\$ – USer345738380 Oct 21 '16 at 5:36
5
\$\begingroup\$

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

\$\endgroup\$
  • \$\begingroup\$ You can even move those bindParam calls outside of the foreach :) \$\endgroup\$ – Pinoniq Sep 26 '13 at 14:25
  • 1
    \$\begingroup\$ @Pinoniq: That would imply using references, and IMO, it's best to avoid those in a loop (dangling references can cause unexpected behaviour) \$\endgroup\$ – Elias Van Ootegem Sep 26 '13 at 14:30
  • 1
    \$\begingroup\$ Thanks for this, learnt some new things, especially placing outside foreach and passing an array into the execute instead of binding, interesting. \$\endgroup\$ – crmepham Sep 27 '13 at 12:56
0
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I assumed I was looping through each image by using this foreach statement? \$\endgroup\$ – crmepham Sep 26 '13 at 13:51
  • \$\begingroup\$ You are, but $_FILES['event-images']['name'] is still an array. Try to do such a loop and just print the values of $tmp and $_FILES['event-images']['name']. Of course, to test this, you need to upload at least two files. As for PDO, see Example 1 in prepare documentation: you need only one preparation for multiple executes. \$\endgroup\$ – Vedran Šego Sep 26 '13 at 14:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.