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>
2 Answers 2
// 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']];
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.
-
\$\begingroup\$ Thanks for the feedback. How would I go about to build the error message based on the array? \$\endgroup\$kexxcream– kexxcream2015年08月01日 07:09:48 +00:00Commented Aug 1, 2015 at 7:09
-
\$\begingroup\$ Something like:
$errors[] = 'Not a valid image. (' . join(', ', $image_types) . ').';\$\endgroup\$janos– janos2015年08月01日 08:11:08 +00:00Commented Aug 1, 2015 at 8:11