Page 1 of 2

If and else statements on upload form!

Posted: Fri Dec 09, 2011 6:51 pm
by lisa evans
The code works perfectly but I cannot get the following right:
- execute only if size is not okay but type is okay.
- execute only if type is not okay but size is okay.

It just skips down as if both type and size was negative, it doesn't check if it is either or. What am I doing wrong?

Here's the code:

Code: Select all

	
if ($sizeOK && $typeOK) {
		switch($_FILES['image']['error']) {
			case 0:
				$success = move_uploaded_file($_FILES['image']['tmp_name'], UPLOAD_DIR.$file);
				if ($success) {
				$result = "$file uploaded successfully";
				}
				else {
				$result = "Error uploading $file. Please try again.";
				}
			break;
			case 3:
				$result = "Error uploading $file. Please try again.";
			default:
				$result = "System error uploading $file. Contact webmaster.";
			}
		}
	elseif ($_FILES['image']['error'] == 4) {
		$result = 'No file selected';
	}
	// execute only if size is not okay but type is okay.
	elseif (!$sizeOK && $typeOK) {
		$result = "Maximum size allowed is $max. The file \"$file\" is too big.";
	}
	// execute only if type is not okay but size is okay.
	elseif (!$typeOK && $sizeOK) {
		$result = "Only file types: gif, jpg, and png are accepted.";
	}
	// execute only if both type and size is not okay. This works. 
	else {
		$result = "The file \"$file\" is too big, the maximum size allowed is $max. <br /> The file also appears to be in a unsupported format, only file types: gif, jpg, and png are accepted.";
	}
}

Re: If and else statements on upload form!

Posted: Fri Dec 09, 2011 7:20 pm
by Celauran
You don't show how $sizeOK and $typeOK are set. Have you var_dump()'ed them before your if statements to ensure they contain expected values?

Re: If and else statements on upload form!

Posted: Fri Dec 09, 2011 8:08 pm
by lisa evans
Apparently they both return false:
boolean false
boolean false

Or both return true:
boolean true
boolean true

They are never in between

Here is the preceding code:

Code: Select all

        $sizeOK = false;
	$typeOK = false;
	if ($_FILES['image']['size'] > 0 && $_FILES['image']['size'] <= MAX_FILE_SIZE) {
		$sizeOK = true;
	}
	foreach ($permitted as $type) {
		if ($type == $_FILES['image']['type']) {
			$typeOK = true;
			break;
		}
	}

Re: If and else statements on upload form!

Posted: Fri Dec 09, 2011 8:22 pm
by lisa evans
They should be functioning independently, but one never returns false while the other returns true. I don't see where I went wrong.

Re: If and else statements on upload form!

Posted: Fri Dec 09, 2011 9:05 pm
by Celauran
I don't see any reason they wouldn't be independent.

Unrelated to the problem at hand, this might be easier:

Code: Select all

$typeOK = in_array($_FILES['image']['type'], $permitted);

Re: If and else statements on upload form!

Posted: Fri Dec 09, 2011 9:48 pm
by lisa evans
But if I did the above suggestion, how would I be able to set $typeOK to "true"?

Re: If and else statements on upload form!

Posted: Fri Dec 09, 2011 10:47 pm
by lisa evans
Any chance you could run this code in your own server and tell me if you get the same results? I been at this for hours.


Here's the full code.

Code: Select all

<?php
// define a constant for the maximum upload size
// pag1 183
define ('MAX_FILE_SIZE', 300000);
// checks whether the $_POST array contains upload, the name attribute of the submit button.
// page 179
if (array_key_exists('upload', $_POST)) {
  // define constant for upload folder
  // page 180
  define('UPLOAD_DIR', 'C:/upload_test/');
  // convert the maximum size to KB
  // page 183
  $max = number_format(MAX_FILE_SIZE/1024, 1).'KB';
  // create an array of permitted MIME types
  $permitted = array('image/gif', 'image/jpeg', 'image/pjpeg', 'image/png');
  foreach ($_FILES['image']['name'] as $number => $file) {
    // replace any spaces in the filename with underscores
    $file = str_replace(' ', '_', $file);
    // begin by assuming the file is unacceptable
    // page 184
    $sizeOK = false;
    $typeOK = false;
    // check that file is of a permitted MIME type
    // page 187
  foreach ($permitted as $type) {
    if ($type == $_FILES['image']['type'][$number]) {
      $typeOK = true;
      break;
    }
  }
    // check that file is within the permitted size
  if ($_FILES['image']['size'][$number] > 0 || $_FILES['image']['size'][$number] <= MAX_FILE_SIZE) {
    $sizeOK = true;
  }
  // if size and type are true
  // page 185
  echo var_dump ($sizeOK, $typeOK);
  if ($sizeOK && $typeOK) {
    switch($_FILES['image']['error'][$number]) {
      case 0:
        // $username would normally come from a session variable
        $username = 'davidp';
        // if the subfolder doesn't exist yet, create it
        if (!is_dir(UPLOAD_DIR.$username)) {
          mkdir(UPLOAD_DIR.$username);
        }
        // check if a file of the same name has been uploaded
        if (!file_exists(UPLOAD_DIR.$username.'/'.$file)) {
          // move the file to the upload folder and rename it
          $success = move_uploaded_file($_FILES['image']['tmp_name'][$number], UPLOAD_DIR.$username.'/'.$file);
        }
        else {
          // add time stamp to it so that it won't override the original file
          $success = move_uploaded_file($_FILES['image']['tmp_name'][$number], UPLOAD_DIR.$username.'/'.time().$file);
        }
        if ($success) {
          $result[] = "$file uploaded successfully";
        }
        else {
          $result[] = "Error uploading $file. Please try again.";
        }
        break;
        case 3:
          $result[] = "Error uploading $file. Please try again.";
        default:
          $result[] = "System error uploading $file. Contact webmaster.";
      }
    }
  elseif ($_FILES['image']['error'][$number] == 4) {
    $result[] = 'No file selected';
  }
  else {
    $result[] = "$file cannot be uploaded. Maximum size: $max. Acceptable file types: gif, jpg, png.";
    }
  }
}
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
<title>Multiple file upload</title>
</head>

<body>
<?php
// if the form has been submitted, display result
if (isset($result)) {
  echo '<ol>';
    foreach ($result as $item) {
      echo "<strong><li>$item</li></strong>";
    }
  echo '</ol>';
}
?>
<form action="" method="post" enctype="multipart/form-data" name="multiUpload" id="multiUpload">
    <p>
        <label for="image1">File 1:</label>
    <input type="hidden" name="MAX_FILE_SIZE" value="<?php echo MAX_FILE_SIZE; ?>" />
        <input type="file" name="image[]" id="image1" />
    </p>
    <p>
        <label for="image2">File 2:</label>
        <input type="file" name="image[]" id="image2" />
    </p>
    <p>
        <input name="upload" type="submit" id="upload" value="Upload files" />
    </p>
</form>
</body>
</html>

Re: If and else statements on upload form!

Posted: Sat Dec 10, 2011 6:54 am
by Celauran
lisa evans wrote:But if I did the above suggestion, how would I be able to set $typeOK to "true"?
in_array returns a boolean.

Re: If and else statements on upload form!

Posted: Sat Dec 10, 2011 6:59 am
by Celauran
lisa evans wrote:Any chance you could run this code in your own server and tell me if you get the same results?
Works fine on my server.

Re: If and else statements on upload form!

Posted: Sat Dec 10, 2011 10:41 am
by McInfo
Here is the original code plus my comments.

Code: Select all

<?php
define('MAX_FILE_SIZE', 300000);

// Initialize $result to an empty array or null here.

if (array_key_exists('upload', $_POST)) {
    define('UPLOAD_DIR', 'C:/upload_test/');

    // Should this be defined as a constant?
    $max = number_format(MAX_FILE_SIZE / 1024, 1) . 'KB';

    $permitted = array('image/gif', 'image/jpeg', 'image/pjpeg', 'image/png');
    foreach ($_FILES['image']['name'] as $number => $file) {

        // Make the file name sanitization more robust.  What will happen if the
        // user submits a value that isn't a valid file name?
        $file = str_replace(' ', '_', $file);

        // Instead of using these booleans, immediately append a message to the
        // $result array whenever you discover a problem.  Later, finalize the
        // upload only if $result is empty.
        $sizeOK = false;
        $typeOK = false;

        // Replace this foreach block with the code Celauran suggested; except,
        // instead of an assignment, use in_array() in a condition. (See below.)
        foreach ($permitted as $type) {
            if ($type == $_FILES['image']['type'][$number]) {
                $typeOK = true;
                break;
            }
        }
        // This code means, "If the submitted type is not in the list of
        // permitted types, append a message to the $result array."
        // if (!in_array($_FILES['image']['type'][$number], $permitted)) {
        //     $result[] = 'Permitted types are ' . implode(', ', $permitted) . '.';
        // }

        // When will this condition ever be false?  Study the logic with some
        // experimental values.  Hint: 2 is greater than 3 or less than 5.
        if ($_FILES['image']['size'][$number] > 0 || $_FILES['image']['size'][$number] <= MAX_FILE_SIZE) {
            $sizeOK = true;
        }

        // var_dump() itself writes to the output stream.  There is no need to
        // echo its return value.
        echo var_dump($sizeOK, $typeOK);

        // This condition is unnecessary if you rely on $result as described.
        if ($sizeOK && $typeOK) {

            // Move the mkdir() and move_uploaded_file() logic out of this
            // switch block.  Use the switch only to assign messages to $result.
            // Handle all potential errors with this switch.
            switch ($_FILES['image']['error'][$number]) {

                // Use the UPLOAD_ERR_* constants instead of magic numbers.
                case 0:
                    $username = 'davidp';

                    // You might consider defining a USER_DIR constant.
                    if (!is_dir(UPLOAD_DIR . $username)) {
                        mkdir(UPLOAD_DIR . $username);
                    }
                    if (!file_exists(UPLOAD_DIR . $username . '/' . $file)) {
                        $success = move_uploaded_file($_FILES['image']['tmp_name'][$number], UPLOAD_DIR . $username . '/' . $file);
                    } else {
                        // microtime() would give more assurance of uniqueness;
                        // but can you be sure this file doesn't exist too?
                        $success = move_uploaded_file($_FILES['image']['tmp_name'][$number], UPLOAD_DIR . $username . '/' . time() . $file);
                    }
                    if ($success) {
                        $result[] = "$file uploaded successfully";
                    } else {
                        $result[] = "Error uploading $file. Please try again.";
                    }
                    break;
                case 3:
                    $result[] = "Error uploading $file. Please try again.";
                    // No break?  Is that intentional?
                default:
                    $result[] = "System error uploading $file. Contact webmaster.";
            }
        } elseif ($_FILES['image']['error'][$number] == 4) {
            $result[] = 'No file selected';
        } else {
            $result[] = "$file cannot be uploaded. Maximum size: $max. Acceptable file types: gif, jpg, png.";
        }
        
        // Here is where you should check if $result is empty.  If it is empty,
        // make the directory and move the file.
    }
}
?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
    <head>
        <?php
        // Does the Content-Type header sent by the server agree with this meta tag?
        ?>
        <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
        <title>Multiple file upload</title>
    </head>

    <body>
        <?php
        // If you initialized $result to an empty array earlier, change this
        // condition to (!empty($result)). Note the "not" operator.  If you
        // initialized to null, this change is optional but still recommended.
        if (isset($result)) {
            echo '<ol>';
            foreach ($result as $item) {
                // Sanitize $item with htmlentities() before echoing it.
                echo "<strong><li>$item</li></strong>";
            }
            echo '</ol>';
        }
        ?>
        <form action="" method="post" enctype="multipart/form-data" name="multiUpload" id="multiUpload">
            <p>
                <label for="image1">File 1:</label>
                <input type="hidden" name="MAX_FILE_SIZE" value="<?php echo MAX_FILE_SIZE; ?>" />
                <input type="file" name="image[]" id="image1" />
            </p>
            <p>
                <label for="image2">File 2:</label>
                <input type="file" name="image[]" id="image2" />
            </p>
            <p>
                <input name="upload" type="submit" id="upload" value="Upload files" />
            </p>
        </form>
    </body>
</html>

Re: If and else statements on upload form!

Posted: Sat Dec 10, 2011 10:47 am
by Celauran
I completely missed the > 0 || < MAX SIZE. Nice catch.

Re: If and else statements on upload form!

Posted: Sat Dec 10, 2011 12:52 pm
by lisa evans
Thank you for the suggestions Celauran and McInfo. I am not an expert on this and I really appreciate the help.

I have made the implementations you guys suggested, however, it still defeats my original point which was to check and display multiple errors if they exist.
In other words, once the script checks

Code: Select all

if (!in_array($_FILES['image']['type'][$number], $permitted)) {
$result[] = 'Permitted types are ' . implode(', ', $permitted) . '.';
}
It stops and gives out the error message without checking if the size of the file is also a problem. I want it to give both error messages if both the size and type are wrong but only one correspondent error message if only the size or type is wrong. That's why I did the whole True and False method.

My logic was that I would make them originally both false. Then I would have PHP check the type first, and give me a true or false on that. Then proceed to check the size and give me a true or false on that. If they both turned out True, the operation would go into the move file, if they were false I would do a error message based on the True or False combination:

Code: Select all

// execute only if size and type are TRUE.
if ($sizeOK && $typeOK) {
       // execute file upload and give internal errors that will only occur after the file has been sent.
       }
       // execute only if size remained FALSE but type is TRUE.
       elseif (!$sizeOK && $typeOK) {
                $result = "Maximum size allowed is $max. The file \"$file\" is too big.";
        }
        // execute only if type remained FALSE but size is TRUE.
        elseif (!$typeOK && $sizeOK) {
                $result = "Only file types: gif, jpg, and png are accepted.";
        }
        // execute only if both type and size remained FALSE.
        else {
                $result = "The file \"$file\" is too big, the maximum size allowed is $max. <br /> The file also appears to be in a unsupported format, only file types: gif, jpg, and png are accepted.";
        }

The problem is sizeOK and typeOK are not behaving like they are suppose to. They will block or submit if the file is completely wrong or completely right, they will not give me any response in between.

Contrary to before, these are the results I am getting:

If size and type are okay, I get:
$sizeOK = True
$typeOK = True

If size is too big, but the type is okay, I get:
$sizeOK = True
$typeOK = False
...makes no sense, the file is being blocked by the wrong operator.

If size is Okay, but the type is wrong, I get:
$sizeOK = True
$typeOK = False
...this one seems to be right.

If both the size and type are wrong, I get:
$sizeOK = True
$typeOK = False
...same as before.


It's almost like the typeOK will only return true if sizeOK is really TRUE. Almost like it knows when sizeOK is lying. lol

Also McInfo, if you know any good articles or tutorials for me to "Make the file name sanitization more robust" please let me know.

Re: If and else statements on upload form!

Posted: Sat Dec 10, 2011 3:21 pm
by McInfo
Remember that $result is supposed to be an array. This statement replaces the array with a string.

Code: Select all

$result = "...";
This appends a new element to the array.

Code: Select all

$result[] = "...";
My suggestion was to completely remove $sizeOK and $typeOK, and rely solely on $result. The idea is to walk the input through a gauntlet of requirements and gather error messages along the way. Then, if the $result array is empty, assume that the input is valid. Here is some pseudo-code for illustration.

Code: Select all

$result = array();
if (type_is_bad()) {
    $result[] = "Incorrect type.";
}
if (size_is_out_of_bounds()) {
    $result[] = "Size is wrong.";
}
if (some_other_error_occurred()) {
    $result[] = "I just don't like it.";
}
if (empty($result)) {
    do_something_with_validated_input();
    $result[] = "All is well. Thank you for conforming.";
} else {
    $result[] = "Try again.";
}
print_r($result);

Re: If and else statements on upload form!

Posted: Sat Dec 10, 2011 4:44 pm
by lisa evans
Is this what your saying?

Code: Select all

if (array_key_exists('upload', $_POST)) {
  define('UPLOAD_DIR', 'C:/upload_test/');
  $max = number_format(MAX_FILE_SIZE/1024, 1).'KB';
  $permitted = array('image/gif', 'image/jpeg', 'image/pjpeg', 'image/png');
  foreach ($_FILES['image']['name'] as $number => $file) {
    $file = str_replace(' ', '_', $file);
  }
  $result = array();
  if (!in_array($_FILES['image']['type'][$number], $permitted)) {
    $result[] = 'Permitted types are ' . implode(', ', $permitted) . '.';
  }
  if ($_FILES['image']['size'][$number] < 1 || $_FILES['image']['size'][$number] >= MAX_FILE_SIZE) { 
    $result[] = "Size is wrong.";
  }
  if (empty($result)) {
    $result[] = "All is well. Thank you for conforming.";
  } 
  else {
    $result[] = "Try again.";
  }
print_r($result);
}

Re: If and else statements on upload form!

Posted: Sat Dec 10, 2011 6:42 pm
by McInfo
Almost.

Code: Select all

// Initialize here.
$result = array();
if (array_key_exists('upload', $_POST)) {
    define('UPLOAD_DIR', 'C:/upload_test/');
    $max = number_format(MAX_FILE_SIZE / 1024, 1) . 'KB';
    $permitted = array('image/gif', 'image/jpeg', 'image/pjpeg', 'image/png');
    foreach ($_FILES['image']['name'] as $number => $file) {
        // Depending on the operating system, certain characters are not
        // allowed in file names.  Those characters should be replaced too.
        $file = str_replace(' ', '_', $file);
        // Be sure the file handling is within the foreach loop.
        if (!in_array($_FILES['image']['type'][$number], $permitted)) {
            // Because multiple files are uploaded, $result should be a two-
            // dimensional array to keep the error messages grouped by image
            // number.  A nested loop will be required to display the messages.
            $result[$number][] = 'Permitted types are ' . implode(', ', $permitted) . '.';
        }
        // I adjusted the range boundaries.
        if ($_FILES['image']['size'][$number] <= 0 || $_FILES['image']['size'][$number] > MAX_FILE_SIZE) {
            $result[$number][] = 'Size is wrong.';
        }
        // Use a switch to handle other types of errors.
        switch ($_FILES['image']['error'][$number]) {
            case UPLOAD_ERR_OK:
                // No error
                break;
            case UPLOAD_ERR_INI_SIZE:
                $result[$number][] = 'Too big.';
                break;
            case UPLOAD_ERR_PARTIAL:
                $result[$number][] = 'Partially uploaded.';
                break;
            // Add cases for the other error types.
            // See php.net/manual/en/features.file-upload.errors.php
            default:
                $result[$number][] = 'An unknown error occurred.';
        }
        if (empty($result)) {
            // Put mkdir() and move_uploaded_file() here.
            $result[$number][] = 'All is well. Thank you for conforming.';
        } else {
            $result[$number][] = 'Try again.';
        }
    }
}