5
\$\begingroup\$

I wrote my first simple image upload script that allows users to add items to a database along with pictures of said item. My script takes the images uploaded via a form, processes and resizes them and finally saves them to a hard disk, with their file paths added to a database.

While my code is currently working, I have the following questions:

  1. Should I be processing the images after the items have been successfully inserted into the database?
  2. Should I be hard-coding/ specifying the image width and height explicitly or is there a better/ more efficient way of going about it.
  3. Is my code an acceptable way of handling uploaded images?
  4. Are there any better/ more efficient/ neater methods of refactoring my code?

//Code to add the item to database.....
    //If the insertion is successful
    if($insertdata)
    {

        $upload=$_FILES['ImageUpload']['tmp_name'];
        $uploadnum=count($upload);

        for($i=0;$i<$uploadnum;$i++)
        {

            /*--------------------------------------------------------------------
            |                       Image upload Conditions                      |
            --------------------------------------------------------------------*/
            //Valid image extensions
            $validext=array('.jpg','.png','.gif');
             // Maximum filesize in BYTES (currently 2.0MB).
            $maxsize = 2097152;

            if(isset($upload[$i]) && !empty($upload[$i]) && is_uploaded_file($upload[$i]))
            {
                if($_FILES['ImageUpload']['size'][$i] < $maxsize)
                {
                    /*------------------------------------------------------------------------------
                    |                     Prepares Image for uploading                             |
                    ------------------------------------------------------------------------------*/

                    //Replaces spaces in the item name with underscores
                    $itemname=str_replace(' ', '_', $itemname);
                    //Sets the upload path for the images to be uploaded
                    $displayuploadpath='..\\..\\images\\'.$shopid."\\item\\".$itemname."\\display\\";
                    $thumbuploadpath='..\\..\\images\\'.$shopid."\\item\\".$itemname."\\thumb\\";
                    //Checks if the upload path directory exists and creates it if it's not
                    if(!is_dir($thumbuploadpath))
                    {
                        mkdir($thumbuploadpath,0644,true);
                    }

                    if(!is_dir($displayuploadpath))
                    {
                        mkdir($displayuploadpath,0644,true);
                    }

                    //New random filename
                    $renamedfile=uniqid("",true);
                    //Gets the image's width,height and filetype
                    list($width,$height,$type)=getimagesize($upload[$i]);
                    //checks the mime type, gets the extension and creates an image identifier
                    switch($type)
                    {
                        case 1://GIF
                            $uploadedfile=$_FILES["ImageUpload"]["tmp_name"][$i];
                            $src=imagecreatefromgif($uploadedfile);
                            $ext=".gif";
                            break;
                        case 2://JPEG
                            $uploadedfile=$_FILES["ImageUpload"]["tmp_name"][$i];
                            $src=imagecreatefromjpeg($uploadedfile);
                            $ext=".jpg";
                            break;
                        case 3://PNG
                            $uploadedfile=$_FILES["ImageUpload"]["tmp_name"][$i];
                            $src=imagecreatefrompng($uploadedfile);
                            $ext=".png";
                            break;
                        default:
                            die("An error has occurred.Please follow the <a href='#'>link to try again</a>");
                    }
                    //Creates thumbnail
                    $thumbnewwidth=100;
                    $thumbnewheight=100;
                    $thumbimg=imagecreatetruecolor($thumbnewwidth, $thumbnewheight);
                    imagecopyresampled($thumbimg,$src,0,0,0,0,$thumbnewwidth,$thumbnewheight,$width,$height);
                    //Creates displayimage
                    $displaynewwidth=250;
                    $displaynewheight=250;
                    $displayimg=imagecreatetruecolor($displaynewwidth, $displaynewheight);
                    imagecopyresampled($displayimg,$src,0,0,0,0,$displaynewwidth,$displaynewheight,$width,$height);
                    //If the image currently being processed is the display pic, add "DP" to the fileuploadname
                    if($i==0)
                    {
                        //Sets thumbnail filepaths to the pictures and saves it
                        $thumbfilename=$thumbuploadpath.$renamedfile."DP";
                        $thumbfilepath=$thumbuploadpath.$renamedfile."DP".$ext;

                        //Sets displaypic filepaths to the pictures and saves it
                        $displayfilename=$displayuploadpath.$renamedfile."DP";
                        $displayfilepath=$displayuploadpath.$renamedfile."DP".$ext; 
                    }
                    else
                    {
                        //Sets thumbnail filepaths to the pictures and saves it
                        $thumbfilename=$thumbuploadpath.$renamedfile;
                        $thumbfilepath=$thumbuploadpath.$renamedfile.$ext;

                        //Sets displaypic filepaths to the pictures and saves it
                        $displayfilename=$displayuploadpath.$renamedfile;
                        $displayfilepath=$displayuploadpath.$renamedfile.$ext;
                    }
                    /*-------------------------------------------------------------------------------
                    |                            Starts uploading image                             |
                    -------------------------------------------------------------------------------*/
                    if(in_array($ext,$validext))
                    {
                        if(is_writable($thumbuploadpath) && is_writable($displayuploadpath))
                        {
                            if($ext==".gif")
                            {
                                imagegif($thumbimg,$thumbfilepath);
                                imagegif($displayimg,$displayfilepath);
                            }
                            elseif($ext==".jpg")
                            {
                                imagejpeg($thumbimg,$thumbfilepath);
                                imagejpeg($displayimg,$displayfilepath);
                            }
                            elseif($ext==".png")
                            {
                                imagepng($thumbimg,$thumbfilepath);
                                imagepng($displayimg,$displayfilepath);
                            }
                            else
                            {
                                die("There was an error in saving the images.Please follow the <a href='#'>link to try again</a>");
                            }
                            //End of image saving
                            if($i==0)//This is the display pic
                            {
                                $insertDP=$cxn->prepare("UPDATE `Items` SET `ItemDP`=:dp WHERE `ItemID`=:itemid");
                                $insertDP->bindValue(":dp",$displayfilepath);
                                $insertDP->bindValue(":itemid",$iteminsertid);
                                $insertDP->execute();
                                //Inserts the displaypic info into database
                                $insertimage=$cxn->prepare("INSERT INTO `ItemPics` (`ShopID`,`ItemID`,`FileName`,`Extension`) VALUES (:shopid,:itemid,:filename,:ext)");
                                $insertimage->bindValue(":shopid",$shopid);
                                $insertimage->bindValue(":itemid",$iteminsertid);
                                $insertimage->bindValue(":filename",$displayfilename);
                                $insertimage->bindValue(":ext",$ext);
                                $insertimage->execute();
                                //Inserts the thumbnail's info into database
                                $insertthumb=$cxn->prepare("INSERT INTO `ItemThumbs` (`ShopID`,`ItemID`,`FileName`,`Extension`) VALUES (:shopid,:itemid,:filename,:ext)");
                                $insertthumb->bindValue(":shopid",$shopid);
                                $insertthumb->bindValue(":itemid",$iteminsertid);
                                $insertthumb->bindValue(":filename",$thumbfilename);
                                $insertthumb->bindValue(":ext",$ext);
                                $insertthumb->execute();
                            }
                            else//Image currently being processed is not the display pic
                            {
                                //Inserts the displaypic info into database
                                $insertimage=$cxn->prepare("INSERT INTO `ItemPics` (`ShopID`,`ItemID`,`FileName`,`Extension`) VALUES (:shopid,:itemid,:filename,:ext)");
                                $insertimage->bindValue(":shopid",$shopid);
                                $insertimage->bindValue(":itemid",$iteminsertid);
                                $insertimage->bindValue(":filename",$displayfilename);
                                $insertimage->bindValue(":ext",$ext);
                                $insertimage->execute();
                                //Inserts the thumbnail's info into database
                                $insertthumb=$cxn->prepare("INSERT INTO `ItemThumbs` (`ShopID`,`ItemID`,`FileName`,`Extension`) VALUES (:shopid,:itemid,:filename,:ext)");
                                $insertthumb->bindValue(":shopid",$shopid);
                                $insertthumb->bindValue(":itemid",$iteminsertid);
                                $insertthumb->bindValue(":filename",$thumbfilename);
                                $insertthumb->bindValue(":ext",$ext);
                                $insertthumb->execute();
                            }
                            //End of data insertion into ItemPics and ItemThumbs tables

                            //Grabs the ShopName, to use in updates, if any.
                            $getshopname=$cxn->prepare("SELECT `ShopName` FROM `Shops` WHERE `ShopID`=:shopid");
                            $getshopname->bindValue(":shopid",$shopid);
                            $getshopname->execute();
                            while($fetchshopname=$getshopname->fetch(PDO::FETCH_ASSOC))
                            {
                                $shopname=$fetchshopname['ShopName'];
                            }
                            //Add item to the updates table
                            $additemupdate=$cxn->prepare("INSERT INTO `Updates` (`ShopID`,`UpdateMsg`) VALUES (:shopid,:updatemsg)");
                            $additemupdate->bindValue(":shopid",$shopid);
                            $additemupdate->bindValue(":updatemsg",$shopname." has added ".$itemname." to their inventory");
                            $additemupdate->execute();
                            //Redirect to success page
                            $cxn->commit();
                            header("Location:success.php");
                        }
                        else
                        {
                            die("Invalid file path specified".$displayfilepath."Please try again <a href='#'>here</a> ");
                        }
                        //End of filepath validity check
                    }
                    else
                    {
                        die("Invalid file extension.Please try again <a href='#'>here</a> ");
                    }
                    //End of extension check
                }
                //Image exceeds maximum file limit
                else
                {
                    die("Your image exceeded the maximum size of 2MB.Please reduce the size and try again <a href='#'>here</a>.");
                }
                //End of Image size check

            }
            //If image being uploaded fails the upload checks
            else
            {
                die("An error has occurred.Please follow the <a href='#'>link to try again</a>");
            }
            //End of image upload checks
        }
        //End of image uploading

    }
    //If insertion is not successful
    else
    {
        die("An error has occurred.Please follow the <a href='#'>link to try again</a>");
    }
\$\endgroup\$
  • \$\begingroup\$ In relation to 2. I tend to keep the original image, then resize it as necessary from the page that requests it (and keep that cached copy). See this SO topic for a bit of an idea of how it works, stackoverflow.com/questions/10515985/php-dynamic-images/… \$\endgroup\$ – bumperbox May 4 '14 at 11:03
  • \$\begingroup\$ In relation to 4. You can easily split some of that code into functions, and have a much smaller (easily readable/maintainable) main for loop. \$\endgroup\$ – bumperbox May 4 '14 at 11:05
2
\$\begingroup\$

Let's start with a review:

  • Use foreach: When iterating an array (or an Iterator), PHP provides you with the foreach loop, which allows you to easily iterate over the array.

    foreach ($_FILES as $file) { //$file now holds the current item, i.e. $_FILES[$i];
    
  • Use absolute paths. Relative paths are brittle, and bound to cause hard-to-track errors when changing the directory structures. Absolute paths will allow you to very quickly know that you need to change something in case of structure shift.

  • Better filename handling: I usually save the file as a hash of the contents of the image (sha1($image_file_contents) . ".$ext"), this has the nice added property of not allowing duplicate images (It won't catch all cases, but it works nicely). Also, dividing the images into subdirectories (by the first letter or two of the filename) will help you out tremendously when you start getting thousands of images.

  • Don't die(): Calling die() in your code will cause the script to terminate instantly. If you were in an included file or something else still needs to run, it won't. Store the error message, and output it at the end of the script.

Now, for your questions

  1. Depends, are the pictures extremely heavy or otherwise take a burden on the server? If you're uploading 100 2MB pictures at a time, that may be the case. If you're only upload 2-3, it wouldn't.
  2. Don't generate the thumbnail in advance. Generate it on-demand and based on the requester's needs (i.e. an avatar in a forum is 64x64, but in the profile page it's 128x128). Save the results normally to a file and create a database entry for it, so that next time, you won't have to generate the thumbnail again.
  3. It is. It depends on the rest of the application. Personally, I'd go with a more OO solution, but that's because I understand how OO works and how to incorporate it into my needs.
  4. Split the code into smaller pieces of work. Either objects or functions, that do small tasks each (get input, do something, return output). This will help code readability tremendously.
\$\endgroup\$

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.