Code Review Asked by retqio on November 25, 2021
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>
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.
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"]
.
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.
$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.
Answered by Sᴀᴍ Onᴇᴌᴀ on November 25, 2021
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP