# how can I improve the check name file part

/**
* Upload has a limit of 10 mb
* @param string $dir$_SERVER['DOCUMENT_ROOT']
* @param string $path Path do you want to upload the file * @param string$filetype jpg|jpeg|gif|png|doc|docx|txt|rtf|pdf|xls|xlsx|ppt|pptx
* @param array $_FILES An associative array of items uploaded to the current script via the HTTP POST method. * @return string ?$fileName:False
*/
function uploadFiles($dir,$path, $filetype) {$dir_base = "{$dir}{$path}";
$dateadded = date('ynj_Gis-');$rEFileTypes = "/^\.($filetype){1}$/i";
$MAXIMUM_FILESIZE = 10 * 1024 * 1024; // UPLOAD IMAGES$isFile = is_uploaded_file($_FILES['file']['tmp_name']); if ($isFile) {
$safe_filename =$dateadded . preg_replace(array('/\s+/', '/[^-\.\w]+/'), array('_', ''), trim($_FILES['file']['name'])); if ($_FILES['file']['size'] <= $MAXIMUM_FILESIZE && preg_match($rEFileTypes, strrchr($safe_filename, '.'))) {$isMove = move_uploaded_file($_FILES['file']['tmp_name'],$dir_base . $safe_filename); } } if ($isMove) {
return $safe_filename; } else { return false; } }  How can I improve the name check part or another things? • You should post this to StackOverflow instead. Sep 19 '13 at 17:50 • You're re-assigning/overriding the auto-global variable$_FILES in the function arguments. Since $_FILES is a global, there's no need to pass it as an argument to the function (its accessible from within the function scope). Sep 19 '13 at 18:00 • @Jamal question edited, it's working now Sep 19 '13 at 23:15 • Alright, I'll put in my vote. – Jamal Sep 19 '13 at 23:44 ## 1 Answer I have two minor comments. First, I prefer having my variables defined in all branches, to avoid the PHP Notice: Undefined variable:... error in logs. So, an initialization $isMove = false;


would be nice. Btw, maybe $isMoved seems a bit more accurate/correct name. This part might also be written like this: if (!is_uploaded_file($_FILES['file']['tmp_name'])) return false;
$safe_filename =$dateadded . preg_replace(array('/\s+/', '/[^-\.\w]+/'), array('_', ''), trim($_FILES['file']['name'])); if ($_FILES['file']['size'] > $MAXIMUM_FILESIZE) return false; if (!preg_match($rEFileTypes, strrchr($safe_filename, '.'))) return false; if (!move_uploaded_file($_FILES['file']['tmp_name'], $dir_base .$safe_filename)) return false;
return $safe_filename;  Depending on your taste, this might be easier to read. My second comment is related to this: $rEFileTypes = "/^\.($filetype){1}$/i";


What is the purpose of {1} (which means "repeat once")? I'd write that regex like this:

$rEFileTypes = "/\.($filetype)\$/i";  and I would replace preg_match($rEFileTypes, strrchr($safe_filename, '.'))  with preg_match($rEFileTypes, $safe_filename)  It seems to me a bit less cluttered this way, and I'd expect such regex to be faster than doing the old one + strrchr, but I have no proof that it really is. However, I don't think this is a kind of code in which speed is very important (upload of a file and moving it around will eat up much more time). Btw, $filetypes might be a bit better name.

A bit more important comment is regarding the possibility that, given a long enough filename, your safe one will grow too long when you prepend it with a date, and you will lose the file extension (or a part of it). You might want to address that issue by checking the length and trimming the filename part if needed.

Now, for the general approach, there is a question about what you're doing with this. Usually, it is better to keep the original filenames in a database, and to store the uploaded files under a completely generic name (using, for example, an auto_increment primary key from the filenames table). Also, if you don't need filenames at all, you can then just make them generic.

Notice that your $dateadded makes it very likely that your filenames are unique, but it doesn't really guarantee it. Depending on your use and the expected number of users, this might be a potential problem (although I wouldn't expect it). • What do you think about add to $dateadded a $rand=rand(100,999) to be unique, what is your advice to me in this? $dateadded.'_'\$rand Sep 20 '13 at 17:23
• How is rand going to make anything unique? That way you are indeed slimming the chances, but - as I wrote before - they are already very slim. I'm not sure how to make it unique without using generic names (in that case, tempnam can help you) and/or database (then primary auto_increment field can give you the name). Try asking at StackOverflow (or, maybe, Programmers.SE; I'm not sure) about that problem. But, I repeat, unless you have a really huge number of uploads, this will probably never cause any problems. Sep 20 '13 at 17:46