4
\$\begingroup\$

I have written a script for basic uploading of a single image to a web server using PHP. This is an example that is intended for educational purposes in a basic PHP course. It does not take extensive consideration to security as it is not the focus.

Key points to focus on the review:

  • If the code comply with the PSR-1 and PSR-2 requirements
  • If the code can be more efficiently written.
  • Recommendations for presenting success/error messages. Errors are already in an array.

<?php
    // 1 MB = 1048576 bytes
    $max_file_size = 1048576;

    // Accepted file types
    $image_types = array('gif', 'jpg', 'png');

    // Folder for uploaded images
    $upload_dir = realpath(dirname(__FILE__)) . '/images/';

    // Error codes for file upload
    // See: http://www.php.net/manual/en/features.file-upload.errors.php
    $upload_errors = array(
        UPLOAD_ERR_OK         => 'No errors.',
        UPLOAD_ERR_INI_SIZE   => 'Larger than upload_max_filesize.',
        UPLOAD_ERR_FORM_SIZE  => 'Larger than form MAX_FILE_SIZE.',
        UPLOAD_ERR_PARTIAL    => 'Partial upload.',
        UPLOAD_ERR_NO_FILE    => 'No file.',
        UPLOAD_ERR_NO_TMP_DIR => 'No temporary directory.',
        UPLOAD_ERR_CANT_WRITE => 'Can not write to disk.',
        UPLOAD_ERR_EXTENSION  => 'File upload stopped by extension.'
    );

    // Checks form request method
    if ($_SERVER['REQUEST_METHOD'] == 'POST') {

        // Return if no file has been submitted
        if (!isset($_FILES['photo'])) {
            return;
        }

        // Checks if image has been submitted
        if (isset($_FILES['photo'])) {

            // Creates array for containing errors
            $errors     = array();

            // Checks for errors in upload
            if ($_FILES['photo']['error'] > 0) {
                $error_message = $_FILES['file_upload']['error'];
                $errors[] = $upload_errors[$error_message];
            }

            // Declares file information
            $file_tmp    = $_FILES['photo']['tmp_name'];
            $file_name   = $_FILES['photo']['name'];
            $file_size   = $_FILES['photo']['size'];
            $file_sha1   = sha1_file($file_tmp);
            $file_ext    = pathinfo($file_name, PATHINFO_EXTENSION);

            // Creates unique name for uploaded file
            $target_name = $file_sha1 . '.' . $file_ext;

            // Checks if file is an image
            if (!getimagesize ($file_tmp)) {
                $errors[] = 'Ensure you are uploading an image.';
            }

            // Checks if image is a valid type
            if (!in_array($file_ext, $image_whitelist)) {
                $errors[] = 'Not a valid image. (.jpg, .png or .gif).';
            }

            // Checks if image exceed file size
            if ($file_size > $max_file_size) {
                $errors[] = 'Exceeded file size limit (1 MB).';
            }

            // Checks if file already exists
            if (file_exists($upload_dir . $target_name)) {
                $errors[] = 'File already exists.';
            }

            // Checks if there are any errors
            if (empty($errors)) {
                // Checks if upload file can be moved
                if (move_uploaded_file($file_tmp, $upload_dir . $target_name)) {
                    $success = "File uploaded successfully.";
                }
                else {
                    $errors[] = 'Error moving file to directory.';
                }
            }
        }
    }
?>
<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title>send.php</title>
  </head>
  <body>
    <?php if(!empty($success)) { echo $success; } ?>
    <form action="send.php" method="post" enctype="multipart/form-data">
      Choose photo: <input type="file" name="photo">
      <input type="submit" name="submit" value="Submit">
    </form>
  </body>
</html>
\$\endgroup\$

2 Answers 2

5
\$\begingroup\$
        // Return if no file has been submitted
        if (!isset($_FILES['photo'])) {
            return;
        }

        // Checks if image has been submitted
        if (isset($_FILES['photo'])) {

The last line there is redundant. The first if has already done the complementary check. You don't need a second if. At that point, you can just run the code, as it will always run.

                $error_message = $_FILES['file_upload']['error'];
                $errors[] = $upload_errors[$error_message];

The $errors array holds error messages. The $error_message variable holds an error type or error code. It should probably be named in accordance with that. Of course, you don't actually need a variable at all, since you never use it again. You could just say

                $errors[] = $upload_errors[$_FILES['file_upload']['error']];
\$\endgroup\$
3
\$\begingroup\$

It seems you mistyped a variable name: near the top you defined $image_types, but then later you use a yet undefined variable named $image_whitelist.

When you construct the error message for invalid image type, it would be better to build the string based on the array that contains the accepted types. Currently the message is hard coded, and if you later edit the supported types, the message will be out of sync.

$upload_dir . $target_name appears twice. It would be better to store that value in a variable and reuse it, rather than doing the concatenation twice.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for the feedback. How would I go about to build the error message based on the array? \$\endgroup\$
    – kexxcream
    Commented Aug 1, 2015 at 7:09
  • \$\begingroup\$ Something like: $errors[] = 'Not a valid image. (' . join(', ', $image_types) . ').'; \$\endgroup\$
    – janos
    Commented Aug 1, 2015 at 8:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.