how can I improve the check name file part

Question

/**
 * 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?

• Brandon - Free Palestine: You should post this to StackOverflow instead.
• jsanc623: 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).
• Emilio Gort: @Jamal question edited, it's working now
• Jamal: Alright, I'll put in my vote.

Answer 1

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

• Emilio Gort: 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
• Vedran Šego: 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.