PHP - Looping through $_FILES to check filetype

Question

My first question on SO, thanks. :)

I'm developing a support issue logging system for my company and it must allow for files to be uploaded aswell as any problems submitted to our database. There could be from 0-6 different uploads to check, along with a support issue. I've managed to get an accurate variable of how many files there is through having a hidden input field (imgcount) that updates via js whenever an image is selected through a type="file" input, or removed from the form.

My [input type="file"] names are image1, image2, etc. As i thought it'd be easier this way to loop through them.

When the form is submitted the following code takes a look to see if there's any files and checks that they're of valid type (gif/jpeg/png), so they can be uploaded safely. I'm not too worried about viruses as the support system has a nice secure logon and we trust our clients.

$sscount = $_POST['imgcount'];
echo $sscount; //to test the variable
if($sscount>0){
    for($i = 1; $i <= $sscount; $i++){
        if (($_FILES["image$i"]["type"] == "image/gif")
        || ($_FILES["image$i"]["type"] == "image/jpeg")
        || ($_FILES["image$i"]["type"] == "image/png" )
        && ($_FILES["image$i"]["size"] < 500000))
        {

        }
        else
        {
        $errormsg .= "Error: Image $i must be either JPEG, GIF, or PNG and less than 500 kb.<br />";
        }
    }
}

But this doesn't seem to be looping through correctly, anyone got any ideas how i can get it to loop through and return correctly?

Answer 1

Well, your boolean logic is ambiguous and likely not doing what you want. This will probably work better:

    if ((($_FILES["image$i"]["type"] == "image/gif")
    || ($_FILES["image$i"]["type"] == "image/jpeg")
    || ($_FILES["image$i"]["type"] == "image/png" ))
    && ($_FILES["image$i"]["size"] < 500000))

Though if I had my druthers, the entire thing would look like:

    $file = $_FILES['image' . $i];
    $type = $file['type'];
    if(($type == 'image/gif' || $type == 'image/jpeg' || $type == 'image/png') && $file['size'] < 500000)

Answer 2

I think your if conditional is wrong. You need brackets around the first group of booleans that are OR'd, like this:

   if ( (($_FILES["image$i"]["type"] == "image/gif")
    || ($_FILES["image$i"]["type"] == "image/jpeg")
    || ($_FILES["image$i"]["type"] == "image/png" ))
    && ($_FILES["image$i"]["size"] < 500000))

This properly means "if the file is an image of (gif or jpeg or png) AND is less than that size".

The way you had it before was not likely the logic you desired.

Answer 3

This isn't a direct answer to your question, but you can pass form values to PHP as an array which should be easier to loop through. in_array() is also useful for checking that a value is within an allowed list.

HTML:

<input type="file" name="image[]">
<input type="file" name="image[]">
<input type="file" name="image[]">
<input type="file" name="image[]">

PHP:

<?php
if (isset($_FILES['image'])) {
    foreach ($_FILES['image'] as $file) {
        if (!in_array($file['type'], array("image/gif", "image/jpeg", "image/png"))
           || $file['size'] > 500000) {
           //error
        } else {
           //ok
        }
    }
}

Answer 4

The && operator has a higher precedence than ||, so rather than (A OR B OR C) AND D as you intended, it is actually A OR B OR (C AND D)

You can use parentheses to enforce the evaluation you intended.

However, something like this might be cleaner and easier to maintain/read:

$allowed_types=array(
    'image/gif',
    'image/jpeg',
    'image/png',
);


$sscount = $_POST['imgcount'];
if($sscount>0){
    for($i = 1; $i <= $sscount; $i++){

        if (in_array($_FILES["image$i"]["type"], $allowed_types) && 
            ($_FILES["image$i"]["size"] < 500000))
        {

        }

    }
}

Answer 5

As others have mentioned, the way you had grouped your conditionals was wrong. However, rather than simply adding some parentheses, I'd suggest you seperate out the two conditions completely;

// this declaration + the use of in_array() isn't necessary,
// it just makes things a bit cleaner.
$file_types = array("image/gif","image/jpeg","image/png"); 

if($_FILES["image$i"]["size"] < 500000)
{
    if(in_array($_FILES["image$i"]["type"], $file_types)))
    {
        // do stuff
    }
    else
    {
        // error about file type
    }
}
else
{
    // error about file size
}

Having this separation makes the code more readable and suggests the condition hierarchy more readily, PLUS it allows your error messages to be more meaningful. It's good practice to separate conditional statements of different types, so that any error messages remain useful. If your code threw an error as it is, the user has no way of knowing (without having to faff about themselves) whether their image was too big or the wrong type.

Answer 6

You can combine all the ['type']==x || ['type']==y in a single call to in_array($_FILES[...]['type'], $allowed)>

$_FILES[..]['type'] contains data sent by the client that is neither checked nor sanitized by php. If the file's type is of any relevance don't rely on $_FILES[..]['type'] or the suffix of $_FILES[..]['name']. Only the actual contents matters. If needed you can test that with the fileinfo extension or mime_content_type() (which is marked as deprecated in favour of fileinfo)

Answer 7

I don't think you really need a variable that is updated via Javascript. You can use PHP to work out how many files have been uploaded by checking the error code. You could handle the file uploads by checking the file extension too, as different browsers can often send different MIME types. Here is an example of what I'm talking about:

$accepted_files = array(
    'jpg',
    'png',
    'gif',
    'jpeg'
);

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    foreach($_FILES as $key => $upload) {
     if ($upload['error'] == 0) {
      $file_parts = explode ('.',$upload['name']);
      if (in_array($file_parts[sizeof($file_parts)-1], $accepted_files)) {
       // This type of file is a-ok
      }
      else {
       // Not an accepted file type
      }
     }
    }
}