1
\$\begingroup\$

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?

\$\endgroup\$
1
\$\begingroup\$

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).

\$\endgroup\$
  • \$\begingroup\$ "the function itself sometimes prints stuff" - it only prints the $errors array. I store all the error messages there. I have to do that because that would be read by ajax. \$\endgroup\$ – morbidCode Apr 28 '16 at 2:06
  • \$\begingroup\$ "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." - I plan to add them in the function. If I do add them, will the function be fine to use? Or I should destroy the validate_file function? \$\endgroup\$ – morbidCode Apr 28 '16 at 2:08
  • \$\begingroup\$ @morbidCode If it actually validates everything that is to validate, it's fine. I would still create extra functions for checking the extension, etc, but then call them from the validate function. \$\endgroup\$ – tim Apr 28 '16 at 10:26
  • \$\begingroup\$ I will be removing the "no value" checks, but what about my usage of try catch and throwing exceptions? Am I doing fine in all cases? \$\endgroup\$ – morbidCode Apr 28 '16 at 13:48
  • \$\begingroup\$ @morbidCode If you remove the error message return, your error handling should be uniform, which is definitely better, and may be good enough for you. I always prefer my functions either doing something (uploading a file, saving something in the database, etc), or displaying something, but not doing both (it makes the application harder to test, it makes it harder to adapt the view, as stuff is printed all over the place), and it makes code harder to read. So I would just throw the exceptions upwards, and handle the displaying of them there. \$\endgroup\$ – tim Apr 28 '16 at 14:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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