Validation of uploading images and adding records to the database

Question

I am doing a php project with the codeigniter framework. In my controller, I have a function that does the following.

• Uploads an image to the server
• Add a row to the database. The data would be retrieved from the form that calls the controller.

The best approach might be to put these two things in separate functions, but it has to be atomic (If either the upload or the insert fails, the other should not complete) so I decided to put these in one function.

First here are the helper functions I used:

// I created this so I can use it an an argument to functions
function is_empty($var) {
    return empty ( $var );
}

// has the functionality of array_map and array_filter
function array_map_filter($orig, $pred, $apply) {
    $result = array ();
    foreach ( $orig as $key => $value ) {
        if ($pred ( $value )) {
            $result [$key] = $apply ( $value );
        }
    }
    return $result;
}

// checks the status of the new uploaded file saved in a temporary folder. The error code and full path of the file is taken from the $_FILES variable
function validate_file($full_path, $error_code) {
    if (is_file ( $full_path )) {
        throw new Exception ( 'This file already exist.' );
    }
    if ($error_code != 0) {
        throw new Exception ( $error_code );
    }
}

Here is my function.

public function add() {
    if (empty ( $_POST )) {
        $data ['title'] = 'add form';
        $this->load->view ( 'templates/header', $data );
        $this->load->view ( 'content/add' );
        return;
    }
    $errors = array ();
    try {
        $errors = array_map_filter ( $_POST, 'is_empty', function ($val) {
            return "no value";
        } );
        if (empty ( $errors )) {
            $root = $_SERVER ['DOCUMENT_ROOT'];
            $upload_path = $root . 'images/';
            $file_field = 'image_path';
            $file = $_FILES [$file_field];
            $file_name = $file ['name'];
            $full_path = $upload_path . $file_name;
            // a file should already be uploaded in a tmp folder. The following function checks the status of the upload
            // throws an exception, so the rest cannot execute
            validate_file ( $full_path, $file ['error'] );
            $rest = array (
            // ...
            // throws an exception if the record already exist in the database, so the rest cannot execute
            $this->gmodel->insert ( 'neo_content', array_merge ( $_POST, $rest ), array (
                    'title' => $_POST ['title'] 
            ) );
            move_uploaded_file ( $file ['tmp_name'], $full_path );
        }
    } catch ( Exception $e ) {
        // catch everything and add them to the $errors array
        $errors ['error'] = $e->getMessage ();
    }       
    header ( 'Content-Type: application/json' );
    // send to ajax
    echo json_encode ( $errors );
}

As you've noticed, the way I tried to insure the operation is atomic is to throw an exception so I can guarantee the program will not continue to the next part of the code. I've read that doing this is generally bad because users giving wrong inputs should always be expected and the flow of the program should not be interrupted because of that. But I believe I can use exceptions in this case because I wish to preserve atomicity.

Is my code wrong? Are there better ways of doing this? How can I make this code better?

Answer 1

validate_file

I would get rid of your validate_file function. Generally, I'm a big supporter of creating as many functions as possible, as it makes code easier to read, reuse, and test.

But in this case, there are two problems: 1) It handles two somewhat different functionalities (does the file already exist, and was there an error on upload) and 2) It's named poorly (likely because of point 1)): It's not really clear what the function does, because "validate" is such a general term. If I read something like that, I would at a minimum also expect a file extension check, a mime type check, maybe a check for an acceptable file name, a check if it's really an image, etc.

File Check

It doesn't seem that you have any actual checks on the uploaded file, so it seems that a user could just upload eg a PHP file and gain code execution.

You should definitely check the file extension, and if you can move the uploaded file outside the web root, and perform some kind of check on the file itself.

Error Handling and Function signature

Throwing exceptions is fine, but I wouldn't return a string error message on failure (the "no value" return). It's difficult to handle (and it leaves you with the unnecessary if error check, and nesting which makes it a bit hard to follow when what is happening).

Your function signature also doesn't seem all that clear. add is quite generic (uploadImage would be better), and it doesn't accept arguments, but works on global variables directly (I would pass them on).

The return value is also quite unclear, and the function itself sometimes prints stuff, which makes it difficult to use. I would let the calling code show responses (if you must, the alternative would be to show all responses in this function, instead of sometimes returning a string).