Simple PHP image upload with error handling

Question

I wrote this method to upload a floor plan image:

function upload_image($path, $file) {
    $errors = [];
    $ext = pathinfo($file["name"])['extension'];

    //randomly generate filename to avoid conflicts
    $filename = uniqid().'.'.$ext;

    if ($file["size"] > 2000000) {
        $errors[] = "The image cannot be greater than 2MB";
    }

    if (!in_array($ext, ["jpg", "jpeg", "png"])) {
        $errors[] = "The image can only be of .jpg, .jpeg or .png format";
    }

    if (empty($errors)) {
        move_uploaded_file($file["tmp_name"], $path.$filename);
    }

    return array(
        "name" => $filename,
        "errors" => $errors
    );
}

Implementation:

if (isset($_POST['create'])) {
    $image = upload_image(IMG_PATH."/floorplans/", $_FILES['floorplan']);
    $floorplan = new FloorPlan($_POST['floorplan']);

    if (empty($image['errors'])) {
        $floorplan->image = $image['name'];
    } else {
        foreach ($image['errors'] as $error) {
            $floorplan->errors[] = $error;
        }
    }
}

I'm very happy with it and everything seems to work fine, but I am just wondering if there is a better/more efficient way to do this.

Answer 1

• Rather than generate the full array of elements that pathinfo() generously offers only to access one value, use the PATHINFO_EXTENSION flag/option (the second parameter) to only ask for what you need.

• For best efficiency, don't perform any unnecessary processes for disqualified incoming data.

• If you are going to declare errors at the start as an empty array, you won't need to call empty() which checks if the variable does not exist or if it is empty/falsey/null/zero-ish. You can more simply use !$errors. Alternatively, don't declare the empty errors array and then use empty to perform the double check.

• I assume that there is no point to generating a filename for an invalid submission, so I don't think that I'll support passing it with errors.

• I try to avoid declaring single-use and temporary variable where possible. In this case, I'd avoid temporary variable by always dealing with the array to be returned at the end.

How my recommendations boil down...

function upload_image($path, $file) {
    if ($file["size"] > 2000000) {
        $outcome['errors'][] = "The image cannot be greater than 2MB";
    }

    $ext = strtolower(pathinfo($file["name"], PATHINFO_EXTENSION)); // force lowercase for uniformity

    if (!in_array($ext, ["jpg", "jpeg", "png"])) {
        $outcome['errors'][] = "The image can only be in .jpg, .jpeg or .png format.";
    }

    if (empty($outcome['errors'])) {
         // for the record, randomness does not guarantee uniqueness
        $outcome['name'] = uniqid() . '.' . $ext;
        move_uploaded_file($file["tmp_name"], $path . $outcome['name']);
    }

    return $outcome;
}

If you like rabbit holes, here's a good read regarding the security aspects of your task: https://stackoverflow.com/q/6484307/2943403

Answer 2

There's not a lot of code here, but here's something I noticed.

foreach ($image['errors'] as $error) {
    $floorplan->errors[] = $error;
}

Since $floorplan->errors is expecting and array of errors, and $image['errors'] is already an array of errors, why iterate through them and assign them to $floorplan->errors one by one when you can just assign $image['errors'] to $floorplan->errors?

} else {
    $floorplan->errors = $image['errors'];
}