6
\$\begingroup\$

I've written PHP code to upload multiple images to a server.

This function processes a maximum of 10 images at one time and if any image's width or height is larger than 2000px it will resize it. But the code is a little bit slow.

I would like to hear your opinion of this code.

Is there a way to make it faster and more and secure?

P.S.: I use an .htaccess file in upload folder to make it more secure.

This is my PHP code:

$img_num=count($_FILES['upload_images']['name']);
if($img_num>0 && $img_num<11){

for($i=0;$i<$img_num;$i++){
if($_FILES['upload_images']['name'][$i]!=''){
$ext=explode(".",$_FILES['upload_images']['name'][$i]);
$ext=strtolower($ext[1]);

if ((($_FILES['upload_images']['type'][$i] == "image/gif")
|| ($_FILES['upload_images']['type'][$i] == "image/jpeg")
|| ($_FILES['upload_images']['type'][$i] == "image/jpg")
|| ($_FILES['upload_images']['type'][$i] == "image/pjpeg")
|| ($_FILES['upload_images']['type'][$i] == "image/x-png")
|| ($_FILES['upload_images']['type'][$i] == "image/png"))
&& ($_FILES['upload_images']['size'][$i] < 12000000)
&& in_array($ext,array("gif","jpeg","jpg","png"))){

$is_jpg=1;$ex='jpg';

switch ($ext){
case 'jpg':$image=imagecreatefromjpeg($_FILES['upload_images']['tmp_name'][$i]);break;
case 'jpeg':$image=imagecreatefromjpeg($_FILES['upload_images']['tmp_name'][$i]);break;
case 'png':$image=imagecreatefrompng($_FILES['upload_images']['tmp_name'][$i]);if($_FILES['upload_images']['size'][$i] < 2000000){$ex='png';$is_jpg=2;}break;
case 'gif':$image=imagecreatefromgif($_FILES['upload_images']['tmp_name'][$i]);$ex='gif';$is_jpg=3;break;
default:$image=imagecreatefromjpeg($_FILES['upload_images']['tmp_name'][$i]);break;
}

$path='upload/img_'.mb_substr(md5($_FILES['upload_images']['name'][$i].rand(0,50)),0,rand(5,10)).'.'.$ex;

$width=imagesx($image);$height=imagesy($image);
$resize=false;
if($width>2000 || $height>2000){
$width_orig=$width;$height_orig=$height;
$width=2000;$height=2000;
$ratio_orig=$width_orig/$height_orig;
($width/$height > $ratio_orig)?$width=$height*$ratio_orig:$height=$width/$ratio_orig;
$resize=true;
}


if($is_jpg==1){
if($resize){
$bg=imagecreatetruecolor($width, $height);
imagecopyresampled($bg, $image, 0, 0, 0, 0,$width, $height,$width_orig, $height_orig);   
imagejpeg($bg,$path,75);
imagedestroy($bg);
}
else{imagejpeg($image,$path,80);}
imagedestroy($image);
$img_url[]=$ex;
}


elseif($is_jpg==2){
$bg = imagecreatetruecolor($width, $height);
imagesavealpha($bg, TRUE);
imagefill($bg, 0, 0, imagecolorallocatealpha($bg, 0, 0, 0,127));
$resize?imagecopyresampled($bg, $image, 0, 0, 0, 0,$width, $height,$width_orig, $height_orig):imagecopy($bg, $image, 0, 0, 0, 0,$width, $height);
imagedestroy($image);
imagepng($bg,$path,8);
imagedestroy($bg);
}

elseif($is_jpg==3){
move_uploaded_file($_FILES['upload_images']['tmp_name'][$i], $path);
}

}
}
}

}

and this the .htaccess file:

IndexIgnore *

deny from all
<Files ~ "^(.*)+\.(gif|jpg|png|jpeg|svg|mp4)$">
order deny,allow
allow from all
</Files>

Options -Indexes -ExecCGI
RemoveHandler .php .phtml .php3
RemoveType .php .phtml .php3
php_flag engine off 

ForceType application/octet-stream
<FilesMatch "(?i).jpe?g$">
    ForceType image/jpeg
</FilesMatch>
<FilesMatch "(?i).gif$">
    ForceType image/gif
</FilesMatch>
<FilesMatch "(?i).png$">
    ForceType image/png
</FilesMatch>
<FilesMatch "(?i)\.mp4$">
    ForceType video/mp4
</FilesMatch>
\$\endgroup\$
0
2
\$\begingroup\$

Formatting

I'd recommend looking at the accepted PSRs - e.g. PSR-1, PSR-12. Updating the code to comply with them will help make the code readable and consistent with idiomatic PHP code.

Maybe this is just a copy-paste issue, but even if you don't strictly follow PSR-12 please indent nested lines within blocks.

Array syntax

As of the time of writing, PHP 7 has Active support for versions 7.4 and 7.3, and since PHP 5.4 arrays can be declared with short array syntax (PHP 5.4) - i.e. ["gif","jpeg","jpg","png"].

Redundant lines

If you aren't familiar with the principle Don't repeat yourself I recommend reading about it:

The Don’t Repeat Yourself (DRY) principle states that duplication in logic should be eliminated via abstraction;

Let's look at a few place that are redundant:

if ((($_FILES['upload_images']['type'][$i] == "image/gif") 
|| ($_FILES['upload_images']['type'][$i] == "image/jpeg")
|| ($_FILES['upload_images']['type'][$i] == "image/jpg")
|| ($_FILES['upload_images']['type'][$i] == "image/pjpeg")
|| ($_FILES['upload_images']['type'][$i] == "image/x-png")
|| ($_FILES['upload_images']['type'][$i] == "image/png"))
&& ($_FILES['upload_images']['size'][$i] < 12000000)
&& in_array($ext,array("gif","jpeg","jpg","png"))){

The last conditional uses in_array() - why not use that for checking the type? That way there is no need to repeat $_FILES['upload_images']['type'][$i] for each type:

if (in_array($_FILES['upload_images']['type'][$i], 
     ["image/gif", "image/jpeg", "image/jpg", "image/pjpeg", "image/x-png", "image/png"])
    && ($_FILES['upload_images']['size'][$i] < 12000000)
    && in_array($ext, ["gif","jpeg","jpg","png"])) {

Also the switch statement has three cases that do the same thing: 'jpg', 'jpeg' and default - so those first to can be eliminated.

Tri-state variable $is_jpg

This variable has three possible values, yet has a name that might be conceived as a boolean. And somebody reading the code might not know what the values 1, 2 and 3 signify without scanning through the code to see where those values are assigned. I'd suggest eliminating it and instead of checking to see if the value is 1, 2, or 3, check to see if $ex is 'jpg', 'png', or 'gif' since those values are already being set.

\$\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.