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)
{
/*------------------------------------------------------------------------------
------------------------------------------------------------------------------*/

//Replaces spaces in the item name with underscores
$itemname=str_replace(' ', '_',$itemname);
$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;
}
/*-------------------------------------------------------------------------------
-------------------------------------------------------------------------------*/
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'];
}
$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

}
else
{
}
}

}
//If insertion is not successful
else
{
}

• 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/… – bumperbox May 4 '14 at 11:03
• 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. – bumperbox May 4 '14 at 11:05

• 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.