Image upload script

Question

Listening to mseancole's advice in my previous post, I have rewritten the code.

function fihHomeIndex() {
    global $conf, $DBH;

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

        if (@$_POST['ss'] != $_SESSION['shared_secret']) {
            array_push($prelim_check_errors, 'Possible hacking attempt. Upload aborted.');
        }

        if (empty($_POST['adult'])) {
            array_push($prelim_check_errors, 'Please choose whether this image contains ADULT content or is family safe!');
        } elseif ($_POST['adult'] != 'yes' && $_POST['adult'] != 'no') {
            array_push($prelim_check_errors, 'Possible hacking attempt. Upload aborted.');
        }

        if (isSpamIP($_SERVER['REMOTE_ADDR']) !== FALSE) {
            array_push($prelim_check_errors, 'Sorry, your IP is listed in one of the spammer lists we use, which aren\'t controlled by us. More information is available at <a href="http://www.dnsbl.info/dnsbl-database-check.php?IP=' . $_SERVER['REMOTE_ADDR'] . '">http://www.dnsbl.info/dnsbl-database-check.php?IP=' . $_SERVER['REMOTE_ADDR'] . '</a>.');
        }

        if (count($prelim_check_errors) >= 1) {
            fihRenderErrors($prelim_check_errors);
        } else {
            $upload_errors = array(); 
            $names = $_FILES['fihImageUpload']['name'];

            foreach ($names as $index => $name) {
                if ($_FILES['fihImageUpload']['error'][$index] == UPLOAD_ERR_NO_FILE) {
                    unset($names[$index]);
                    continue;
                }

                if (filesize($_FILES['fihImageUpload']['tmp_name'][$index]) > $conf['upload']['max_file_size']) {
                    array_push($upload_errors, htmlspecialchars(strip_tags(utf8_decode($name))) . ' exceeds filesize limit.');
                    unset($names[$index]);
                    continue;
            }

            if (FALSE !== ($fileInfo = getimagesize($_FILES['fihImageUpload']['tmp_name'][$index]))) {
                if (strrchr($name, '.') == FALSE) {
                    array_push($upload_errors, htmlspecialchars(strip_tags(utf8_decode($name))) . ' is missing a file extension.');
                    unset($names[$index]);
                    continue;
                } elseif (! in_array(substr(strrchr($name, '.'), 1), $conf['upload']['file_types']) ||
                          ! in_array($fileInfo['mime'], $conf['upload']['mime_types'])) {
                    array_push($upload_errors, htmlspecialchars(strip_tags(utf8_decode($name))) . ' is not an image.');
                    unset($names[$index]);
                    continue;
                }
            } else {
                array_push($upload_errors, htmlspecialchars(strip_tags(utf8_decode($name))) . ' is not an image.');
                unset($names[$index]);
                continue;
            }
        }

        if (empty($names) || count($upload_errors) >= 1) {
            $error_m = empty($upload_errors) ? 'Please choose aleast file to upload!' : $upload_errors;
            fihRenderErrors($error_m);
        } else {
            foreach ($names as $index => $name) {
                $org_name = sanitize(explode('.', $name)[0]) . '.' . explode('.', $name)[1];
                $new_name = sanitize(explode('.', $name)[0], true) . '_' . time() . '.' . explode('.', $name)[1];

                if (move_uploaded_file($_FILES['fihImageUpload']['tmp_name'][$index], $conf['storage']['folder'] . 'full/' . $new_name)) {
                    $image_info = getimagesize($conf['storage']['folder'] . 'full/' . $new_name);

                    if (! $DBH->query("INSERT INTO `{$conf['db']['table_prefix']}images` (`image_id`, `image_orig_filename`, `image_filename`, `image_adult`) VALUES " . 
                                      "(NULL, '{$org_name}', '{$new_name}', '{$_POST['adult']}');")) {
                            die('Database error');
                        }

                        $image_last_id = $DBH->insert_id;
                        $image_dimensions = $image_info[0] . 'x' . $image_info[1];
                        $image_filesize = filesize($conf['storage']['folder'] . 'full/' . $new_name);

                        createThumbnail($new_name, $conf['storage']['folder'] . 'thumb/', $conf['thumbnail']['width'], $conf['thumbnail']['height'], $image_last_id);

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

                        $template_info[$index] = array('id' => $image_last_id);
                        header('Location: ' . $conf['base_url'] . 'upload-success/' . base64_encode(serialize($template_info)));
                    } else {
                        die('Possible hacking attempt. Upload aborted.');
                    }
                }
            }
        }
    } 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();
    }
}

Now please tell me anywhere where I can further improve this code. Thanks

Answer 1

You ignored mseancole's first bit of advice about not using globals. Note that the globals are not only $conf and $DBH but also $_POST, $_SESSION, and $_FILES. All of these globals should be passed to your functions. And your code is still very difficult to read mostly due to heavy indentation. You have heavy indentation because the function is doing too much. It is concerned with ip addresses, input validation, input filtering, db access, view presentation, and probably more. Yes, all those things need to be dealt with. But, dealing with them all in one place makes one huge piece of non-reusable, non-testable, hard to maintain piece of code.

Answer 2

Ummmm... what happened? I see you've changed and added a few things, but very little is anything I recommended.

I mentioned the globals in my last answer, so I wont go over these again. I'll just remind you that they are bad.

There is no need to use strtoupper() on the request method. It should work just fine how you had it, especially if you are using the loose comparison == instead of the absolute one ===. The only thing I may have mentioned is that you might want to reverse the logic for it to get that else out of the way first. That way you could return early and avoid the heavy indentation.

if( $_SERVER[ 'REQUEST_METHOD' ] != 'POST' ) {
    fihDisplayHead();
    fihDisplayFirstColumn();
    fihDisplayUpload();
    fihDisplayFoot();
    return;//this return will make an else unnecessary
}
//now the rest of the code can be unindented by one level

Honestly, this function shouldn't even be concerned with this anyways. Do that check outside of the function to determine if you should run the script for uploading an image or displaying the page. Those should be separate concerns. Remember the Single Responsibility Principle I mentioned in my last answer. A function should be responsible for one task and that task should be readily identifiable by the function's name.

if( $_SERVER[ 'REQUEST_METHOD' ] == 'POST' ) {
    fihHomeIndex();
} else {
    fihDisplayHead();
    fihDisplayFirstColumn();
    fihDisplayUpload();
    fihDisplayFoot();
}

You should avoid the error suppressor @ at all costs. Manually perform the checks necessary to avoid the errors. This avoids confusion later and ensures your code runs as expected. Besides, as your skills progress and you add things such as error logs and the like, you will find these check statements will make the task of debugging much simpler if you consistently check for things like this.

if( ! isset( $_POST[ 'ss' ] ) || $_POST[ 'ss' ] != $_SESSION[ 'shared_secret' ] ) {

Another possibility is to use filter_input() to automatically check and sanitize the input for you. If it doesn't exist you will get a FALSE return. Sanitizing is always a big part of using user input and should be done regularly.

$secret = filter_input( INPUT_POST, 'ss', FILTER_SANITIZE_STRING );
if( $secret != $_SESSION[ 'shared_secret' ] ) {

Why have all of your array appending been redone to use array_push()? There is no reason for you to use this function unless you are adding a bunch of elements to an array at once. Use the method you were before. Calling a function is actually slower and less legible. Though the speed is negligible and the legibility is more important here.

$prelim_check_errors[] = 'one element';
array_push( $prelim_check_errors,
    'multiple',
    'elements',
    'makes',
    'this method',
    'cleaner'
);

Your function is still doing entirely too much. If statements, especially large ones, are usually pretty good indications for the need for separation. I'm not saying every if statement, mind, just the major ones. For instance, the request method if statement mentioned previously. It was a determining if statement that took the code in one of two completely different directions. Notice how I was able to separate that statement from the function? It didn't create a new function, but it did demonstrate how it could be separated.

Take a look at your code and determine what tasks you need to do, make a very detailed list, and create a separate function for each task, especially if that task is being repeated. If the task is already a function, one of your own or PHP's, then you shouldn't need to create another, but make sure you keep the Single Responsibility Principle in mind as well. A challenge you may face while doing this will be the sharing of resources; Remember my advice from my previous answer about injecting those resources and you should be fine. Typically an application such as this would immensely benefit from an OOP structure, but I would be more concerned with getting those principles down pat first. Doing so should naturally lead into a more OOP approach. Let me help get you started with that list:

• Determine if image has been uploaded (Demonstrated with request method above).
• Verify user (IP address and shared secret, etc...)
• Verify content (Adult, size, etc...).
• Render Errors (Already a function).
• Upload Image.
• etc...