1
\$\begingroup\$
/**
 * 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?

\$\endgroup\$
4
  • \$\begingroup\$ You should post this to StackOverflow instead. \$\endgroup\$
    – Brandon
    Sep 19 '13 at 17:50
  • 1
    \$\begingroup\$ 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). \$\endgroup\$
    – jsanc623
    Sep 19 '13 at 18:00
  • \$\begingroup\$ @Jamal question edited, it's working now \$\endgroup\$ Sep 19 '13 at 23:15
  • 1
    \$\begingroup\$ Alright, I'll put in my vote. \$\endgroup\$
    – Jamal
    Sep 19 '13 at 23:44
1
\$\begingroup\$

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

\$\endgroup\$
2
  • \$\begingroup\$ 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 \$\endgroup\$ Sep 20 '13 at 17:23
  • \$\begingroup\$ 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. \$\endgroup\$ Sep 20 '13 at 17:46

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.