HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Upload a photo with a unique name using PHP

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
uniquewithuploadphpnameusingphoto

Problem

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.



```
'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

Solution

// 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']];

Code Snippets

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

        // Checks if image has been submitted
        if (isset($_FILES['photo'])) {
$error_message = $_FILES['file_upload']['error'];
                $errors[] = $upload_errors[$error_message];
$errors[] = $upload_errors[$_FILES['file_upload']['error']];

Context

StackExchange Code Review Q#98673, answer score: 5

Revisions (0)

No revisions yet.