Image upload and thumbnail creation script using PHP GD

Question

Ignoring my shocking use of the mysql_* extension (and any associated issues), could you take a look at my script that uploads an image, adds it to a database, creates a thumbnail of the image 578px in width, and adds that to the database too?

<?php
require_once("validate.php");

while ($hq = mysql_fetch_assoc($hash_query)) {
    $ukip_name = $hq['ukip_name'];
    $speaker_id = $hq['speaker_id'];
    $conference_id = $hq['conference_id'];
    $conf_title = $hq['conf_title'];
    $webmaster_id = $hq['webmaster_id'];
    $first_name = $hq['first_name'];
    $family_name = $hq['family_name'];
    $job_title = $hq['job_title'];
    $company_name = $hq['company_name'];
    $country = $hq['country'];
    $speaker_photo = $hq['speaker_photo']; // Default = NULL
    $company_logo = $hq['company_logo']; // Default = NULL

}
$errors   = $_FILES['file']['error'];
$tempname = $_FILES['file']['tmp_name'];
$filename = $_FILES['file']['name'];
$filetype = $_FILES['file']['type'];
$filesize = $_FILES['file']['size'];
$extensions = array("jpg", "jpeg", "JPG", "JPEG");
$expl = explode(".", $filename);
$extension = end($expl);
if ((($filetype == "image/jpg") ||
    ($filetype == "image/jpeg")) && in_array($extension, $extensions)) {
    if ($errors == 0) {
        if ($filesize <= 10485760) {
            $timestamp = date("ymd-His");
            $filename = $speaker_id . "_" . $timestamp . "_photo." . $extension;
            $db_insert = mysql_query("INSERT INTO temp_uploads
                                     SET speaker_id = $speaker_id, image_filename = '$filename', image_type = 1");
            move_uploaded_file($tempname, "uploads/" . $filename);
            // Resize JPEG
            $md = explode(".", $filename);
            if (end($md) != "eps") {
                $filedims = getimagesize("uploads/" . $filename);
                $md_width = 578;
                $md_height = ($filedims[1] / $filedims[0]) * $md_width;
                $filename_md = $md[0] . "_md." . end($md);
                $quality = 100;
                $src = "uploads/" . $filename;
                $new = imagecreatefromjpeg($src);
                $canvas = imagecreatetruecolor($md_width, $md_height);
                imagecopyresampled($canvas, $new, 0, 0, 0, 0, $md_width, $md_height, $filedims[0], $filedims[1]);
                imagejpeg($canvas, "uploads/" . $filename_md, $quality);
                $db_insert_md = mysql_query("INSERT INTO temp_uploads
                                            SET speaker_id = $speaker_id, image_filename = '$filename_md', image_type = 1");
            }
            mysql_close();
            header("Location: crop.php?hash={$hash}&file={$filename_md}");
            exit();
        } else {
            closeCon(105, "index", "&hash={$hash}");
        }
    } else {
        closeCon(104, "index", "&hash={$hash}");
    }
} else {
    closeCon(103, "index", "&hash={$hash}");
}
?>

I do checks on the filesize, so that something over 10MB is disallowed, but I have hit out-of-memory PHP errors on line 43 ($new = imagecreatefromjpeg($src);) a couple of times recently - one on an 8MB file, and one on a 3.5MB file, which is worrying. These aren't dropping me out of the script in the expected way (closeCon(105...)) but with a PHP fatal error.

I have temporarily set my initiation file to memory_limit=-1 whilst I try and resolve the error, but I was hoping someone here could look at ways of streamlining my code so as to free up as much memory as possible, as well as suggesting general improvements (again, ignoring any mysql_* related ones - I know, and I would use mysqli_* if I could).

Answer 1

It's considered better to omit the closing ?> of the PHP file.

If you're wondering why, there are a few rare situations in which you can accidentally have whitespace trailing the closing tag and issues will result. Since PHP doesn't require the closing tag anyway, it's safer to just always omit it.

---

I'm a bit skeptical of validate.php. What exactly is it doing? Is it processing user input, or bootstrapping, or what? I suspect it's doing way too many things (processing user input and bootstrapping). It's a bit uncomfortable to me that $hash_query likely has some kind of user input going into it, yet it cascades through another file.

It would be a lot cleaner and cleaner to instead have a getSpeakerByHash() function.

---

Why is the mysql_fetch_assoc in a loop? It seems it's only actually operating on one row.

---

Since you don't use most of the variables that come from the hash_query stuff, I wouldn't bother even defining them. They're just clutter.

---

Also, instead of pulling the values out into independent variables, I would instead give $hq a more meaningful name (perhaps $speaker or something). That makes it a lot clearer that the variables are related, and it makes it a bit easier to see where they came from.

---

What happens if the mysql_fetch_assoc call doesn't return anything? You should handle that situation. (Although it might not actually be possible for there to not be a result since I have no idea where hash_query is coming from.)

---

Once again, with the $_FILES stuff, I would defined a $file = $_FILES['file'] and then access into that array. Not only does it cut down on the clutter of defining 5 variables only to use each one once or twice, it makes it clearer that everything is related in one group.

---

$errors = $_FILES['file']['error']; is prone to emit a warning if a request is submitted without a file field. This means that you should be checking if the field exists:

if (isset($_FILES['file'])) {
    $file = $_FILES['file'];
} else {
    // Error
}

---

$extension = pathinfo($filename, PATHINFO_EXTENSION);

Should be a little cleaner than exploding and accessing the last array element.

---

Since the filetype comes from the client side, you can't trust it at all. I wouldn't even bother checking it. I would just check the extension, and then if the extension is allowed, check if it can actually be opened as a jpg.

---

You should pull 10485760 into some kind of configuration management. That way, if you want to change it to something else later, you don't have to go digging around for it. Also, it might be clearer to have it as 10*1024*1024 since that's immediately clear to be 10MB.

---

Since $timestamp is only used once, I would just do the date call inline instead of having it out in a variable.

---

Instead of a deeply nested if-tree, I would use an if-else flow to keep things flatter:

if (!blah) {
    closeCon(x, "...", "...");
} else if (!bleh) {
    closeCon(y, "...", "...");
} else {
    // Do actual work
}

---

Don't use $errors == 0. UPLOAD_ERR_OK is much clearer.

---

You should make sure move_uploaded_file doesn't fail. If it does, you'll end up with a DB record that has no matching file with it. This can be fixed by either using a transaction and rolling back if the move fails, or more simply, just do the insert after the file moving. (Technically you can then have a file without a DB record, but that will likely be less bad.)

---

Don't reuse variable names ($filename). It's confusing to others reading your code, and it's prone to error.

---

I've cut out the irrelevant code, but consider this:

if ((($filetype == "image/jpg") ||
    ($filetype == "image/jpeg")) && in_array($extension, $extensions)) {
    $filename = $speaker_id . "_" . $timestamp . "_photo." . $extension;
    $md = explode(".", $filename);
    if (end($md) != "eps") {

$md can't be eps. md will always be a value that's in $extensions.

---

With regards to the Location header, technically it's supposed to have a full url (i.e. http://.../... rather than a relative url). No browser actually requires this, and all of them act as expected, but it's worth knowing that they're not technically required to, nor is there a guarantee that future browsers will.

---

It would be cleaner for closeCon to take an array as the third parameter and build the query string internally with http_build_query. It also means that callers to closeCon don't have to handle potential URL escaping.

---

Resizing an image seems a pretty clear cut requirement that might be nice to push into a function rather than embedding in the upload functionality. resizeImageToMaxWidth is also clear enough that it's understandable at a glance rather than having to either comment it or read 10 lines of code.

---

Rather than using "uploads/" . $filename twice, I would create a $filepath variable that contains the entire relative path. This means that you can avoid the potential bug of changing one of the uploads and not the other.

It might also be nice to make uploads configurable.

---

With your insert queries, you should be checking that they succeed. The query never should feasibly fail, but you'll very much want to know if it ever does for some weird reason.

And, if you're not going to check that they succeed, there's no point in assigning them to variables.

I tried not to harp on this since you already seem aware of it, but PDO's exception throwing is a rather nice way of having errors crop up from queries when you don't actually expect them to happen, but you really should know if they do, as in this case.

---

With regards to the first parameters of closeCon, it would be nice to pull those numbers into constants. I have no idea what any of them mean, and you likely won't either 3 months from now.

---

Unfortunately my knowledge of image processing is very limited, but I can tell you that I tend to prefer imagemagick over GD. I have no proof of this, but in the few situations I've used both, IM seems to perform the same functionality a bit snappier than GD.

The only other thing I can tell you is that image processing is surprisingly memory intensive and you might just need to crank up the amount of memory script executions can use. GD at some point probably represents the image as a matrix of pixels. This means that the amount of memory required isn't the size of the image but rather the resolution multiplied by colors per pixel. That means that a simple 8 megapixel picture could be 32MB of memory whereas the JPG compressed version would likely only be 5MB.

This SO question seems to have had a similar problem to you and might be worth looking into.