Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Hello users of codereview,

I have written a basic image upload function that handles up to five file inputs, in PHP, intended to be hosted live on the internet very soon.

As far as I know it's secure & works well (minus checking the range of the first 100 bytes & looking for magic numbers), but please tell me if you can see any flaws or areas where I can improve this function, thanks.

<?php
function fihHomeIndex() {
global $conf, $DBH;

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    $errors = array();

    if (empty($_POST['adult'])) {
        $errors[] = 'Please choose whether this image is ADULT content or family safe!';
    } else {
        if ($_POST['adult'] == 'yes' || $_POST['adult'] == 'no') {

        } else { $errors[] = 'Possible hacking attempt. Upload aborted.'; }
    }

    if ($spamIP = isSpamIP($_SERVER['REMOTE_ADDR'])) {
        $errors[] = 'Sorry, your IP is listed in one of the spammer lists we use.';
    }

    if (count($errors) >= 1) {
        fihDisplayHead();
        fihDisplayFirstColumn();
        fihDisplayError($errors);
        fihDisplayFoot();
    } else {
        $upload_errors = array(); 
        $empty_fields = 0;
        $fields_submitted = count($_FILES['fihImageUpload']['name']);

        foreach ($_FILES['fihImageUpload']['name'] as $index => $name) {
            if ($_FILES['fihImageUpload']['error'][$index] == 4) {
                $empty_fields++;
            }
        }

        $all_fields_empty = ($fields_submitted == $empty_fields) ? true : false;

        if ($all_fields_empty) {
            fihDisplayHead();
            fihDisplayFirstColumn();
            fihDisplayError('Please choose atleast one file to upload!');
            fihDisplayFoot();
        } else {
            $files_to_process = $fields_submitted;
            // TODO if a image was rejected due to an error, the script may
            // break if it tries to process a empty $_FILES array..
            // print_r($_FILES);
            foreach ($_FILES['fihImageUpload']['name'] as $index => $name) {
                if ($_FILES['fihImageUpload']['error'][$index] == 4) {
                    $files_to_process--;
                    continue;
                }

                if ($_FILES['fihImageUpload']['error'][$index] == 0) {
                    if (filesize($_FILES['fihImageUpload']['tmp_name'][$index]) > 5242880) {
                        $upload_errors[] = htmlspecialchars($name) . ' exceeded file size limit.';
                        $files_to_process--;
                        continue;
                    }
                }

                if (false !== ($fileInfo = @getimagesize($_FILES['fihImageUpload']['tmp_name'][$index]))) {
                    if (strrchr($_FILES['fihImageUpload']['name'][$index], '.') == FALSE) {
                        $upload_errors[] = 'Files must have an extension.';
                        $files_to_process--;
                        continue;
                    } elseif (!in_array(substr(strrchr($_FILES['fihImageUpload']['name'][$index], '.'), 1), $conf['upload']['file_types']) ||
                              !in_array($fileInfo['mime'], $conf['upload']['mime_types'])) {
                        $upload_errors[] = htmlspecialchars($name) . ' is not an image.';
                        $files_to_process--;
                        continue;
                    }
                } else {
                    $upload_errors[] = htmlspecialchars($name) . ' is not an image.'; 
                    $files_to_process--;
                    continue;
                }
            }

            if (count($upload_errors) > 0 || $files_to_process == 0) {
                fihDisplayHead();
                fihDisplayFirstColumn();
                fihDisplayError($upload_errors);
                fihDisplayFoot();
                die();
            } else {
                foreach ($_FILES['fihImageUpload']['name'] as $index => $name) {
                    $orig_name = sanitize(explode('.', $_FILES['fihImageUpload']['name'][$index])[0]) . '.' . explode('.', $_FILES['fihImageUpload']['name'][$index])[1];
                    $new_name = sanitize(explode('.', $_FILES['fihImageUpload']['name'][$index])[0]) . '_' . time() . '.' . explode('.', $_FILES['fihImageUpload']['name'][$index])[1];

                    # Upload the file first
                    if (move_uploaded_file($_FILES['fihImageUpload']['tmp_name'][$index], $conf['storage']['folder'] . 'full/' . $new_name)) {
                        $ii = getimagesize($conf['storage']['folder'] . 'full/' . $new_name);

                        # Second tell the database that we uploaded a file
                        if (!$DBH->query("INSERT INTO `".$conf['db']['table_prefix']."images` (`image_id`, `image_orig_filename`, `image_filename`, `image_adult`) VALUES 
                                         (NULL, '" . $orig_name . "', 
                                                '" . $new_name . "', '" . $_POST['adult'] . "');")) {
                            die('Database error');
                        }

                        $li = $DBH->insert_id;
                        $image_ext = $ii[0] . 'x' . $ii[1];
                        $image_size = filesize($conf['storage']['folder'] . 'full/' . $new_name);

                        createThumbnail($new_name, $conf['storage']['folder'] . 'thumb/', 200, 200, $li);

                        if (!$DBH->query("INSERT INTO `".$conf['db']['table_prefix']."images_meta` (`meta_id`, `image_id`, `image_ext`, `image_size`) VALUES
                                         (NULL, '" . $li . "', '" . $image_ext . "', '" . $image_size . "');")) {
                            die('Database error');
                        }

                        $template_info[$index] = array(
                            'thumb_url' => $conf['storage']['url'] . 't/' . $li,
                            'image_absolute_url' => $conf['storage']['url'] . 'f/' . $li,
                            'image_page_url' => $conf['base_url'] . 'view-image/' . $li,
                            'filename' => $orig_name
                        );

                        $sti = base64_encode(serialize($template_info));
                        header('Location: ' . $conf['base_url'] . 'upload-success/' . $sti);
                    }
                }
            }
        }
    }
} else {
    # Display the header
    fihDisplayHead();

    # Display the first column, which contains a login form and social networking tools
    fihDisplayFirstColumn();

    # Display the upload section
    fihDisplayUpload();

    # Footer
    fihDisplayFoot();
}
}
?>

The function is located here

share|improve this question
1  
The code must be posted within the question (not as a link). See the faq. –  Paul Dec 17 '12 at 11:21

1 Answer 1

up vote 5 down vote accepted

Immediately I see globals. Avoid globals at all cost. Strike them from your memory. You will never need them. Ever. Globals are one of the worst things PHP has ever done. It is not always obvious when a variable is a global thus it is very easy to miss and sometimes impossible to trace. It is also really easy to change the value of a global from anywhere, either maliciously or accidentally, meaning they are insecure and definitely shouldn't be used with something as important as a database handle.

Instead you should inject those variables into your function as arguments. You can even use type hinting to ensure the right kinds of arguments are passed. The below function requires a $conf array and a $DBH PDO resource. If the wrong variable type is passed the program will immediately fail. This is just one way to ensure that your application can do what you expect it to do.

function fihHomeIndex( Array $conf, PDO $DBH ) {

The next thing I noticed is the heavy level of indentation. Heavy indentation is bad because it reduces legibility. There is an anti-pattern that explains this. The arrow anti-pattern simply states that excessively indented code, illustrated as code that comes to points like arrows, is bad form and should be avoided. Though I should point out that the arrow shapes are unnecessary, it is the heavy indentation that is important. There are a number of ways to avoid violating this principle, chief among them is avoiding unnecessary if/else statements, not using else statements when unnecessary, and breaking from if statements early if possible.

The following block of code reverses the logic of your original, unused, if statement and removes the else. The if statement was unnecessary because it wasn't being used, but without it the else could not be used, thus the reversal of the logic.

if( $_POST[ 'adult' ] != 'yes' && $_POST[ 'adult' ] != 'no' ) {
    $errors[] = 'Possible hacking attempt. Upload aborted.';
}

The above if statement can then be combined with the parent else statement to create an else-if statement. This removed two levels of indentation and unnecessary code.

if( empty( $_POST[ 'adult' ] ) ) {
    //etc...
} else if( $_POST[ 'adult' ] != 'yes' && $_POST[ 'adult' ] != 'no' ) {
    //etc...
}

You should avoid assigning variables in statements. There are some exceptions, such as in loops, but for the most part it is considered bad form as it is easily missed during debugging. PHP allows it and doesn't know any better, but it could just as easily have been meant to be a comparison rather than assignment and there is no way to know other than with context. However, in the below instance a variable is not even necessary as you are not using it except as an expression. Just comparing the return value of the function is enough.

//proper way to define variables before validating
$spamIP = isSpamIP( $_SERVER[ 'REMOTE_ADDR' ] );
if( $spamIP ) {

//if variable wont be used
if( isSpamIP( $_SERVER[ 'REMOTE_ADDR' ] ) ) {

Let's look at a fundamental principle, "Don't Repeat Yourself" (DRY). As the name implies your code should not repeat itself. I've only scanned your code at this point, but already I have noticed some repetition. Let's look at it in its simplest form first, I'll return to the other aspects as they come up. When you have an array and you are pointing at a specific value inside of it and you need to use that value multiple times, you can assign that value to a variable. This has many benefits, first being that you don't have to type out that path again and again and another being that should that path change you will only have to change it once. For example:

$names = $_FILES[ 'fihImageUpload' ] [ 'name' ];
$fields_submitted = count( $names );

foreach( $names as $index => $name) {
    //etc...
}

When you need the boolean value of an expression, you don't need to explicitly set it, you can just set the value of the variable to the return value of the expression. It is already a boolean. However, this is unnecessary. As mentioned above, you do not need to set a variable if you are not going to use it. Just use the expression in the if statement.

//without the ternary it is the same
$all_fields_empty = $fields_submitted == $empty_fields;
//just declare in if statement
if( $fields_submitted == $empty_fields ) {

Let's return to the DRY principle. When you have a chunk of code that is repeated in multiple parts of your application then you should either find some way to refactor it so it only exists once, or you should create a function to perform that task for you. Let's create a function real quick.

function render( $content ) {
    fihDisplayHead();
    fihDisplayColumn();
    fihDisplayError( $content );
    fihDisplayFoot();
}

//now in fihHomeIndex()
if( count( $errors ) >= 1 ) {
    render( $errors );
}

Sometimes you can violate DRY simply by repeating something unnecessarily. For example: At the very beginning you looped over the $names to determine which files had errors and incremented a counter. Now you are looping over the same array and performing the same check to determine if a file should be skipped. If you unset said file in the first loop it will be unnecessary to perform the check again. So let's modify the first loop.

foreach( $names as $index => $name) {
    if( $_FILES[ 'fihImageUpload' ] [ 'error' ] [ $index ] == 4 ) {
        //$empty_fields++;//unnecessary
        unset( $names[ $index ] );
    }
}

Now, with the above modification the $empty_fields incremental is unnecessary as we can now just check to see if $names array is empty. This also means that the $files_to_process incremental will be unnecessary.

//all fields empty
if( empty( $names ) ) {

Of course we are still repeating our loop, and those are the worst things to repeat as they take up so much memory (relatively). Let's look at a neat little built in function of PHP called array_diff_key(). With this function we can input two arrays and get a returned array of the difference between their keys. The best part about this function is that it wont include any of the extra keys that may exist in the second array that don't also exist in the first. It uses the first array as a template.

$names = $_FILES['fihImageUpload' ] [ 'name' ];
$errors = $_FILES[ 'fihImageUpload' ] [ 'error' ];

$difference = array_diff_key( $names, $errors );
if( empty( $difference ) ) {

Let's look at another fundamental principle. The Single Responsibility Principle is the one that is most being violated here. Remember the function, render(), we created earlier? Notice how it is easy to determine what that that function is doing. Its short, sweet, and does exactly what it says it does. That's what this principle is all about. Functions should do one thing. So look at your function and try and see what all it is doing. Quite a bit. Try breaking your function up into multiple smaller functions. This will also aid in following the DRY and arrow anti-pattern.

There's probably a lot more I can go into, but this should be a good start.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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