Function for inserting data into database

Question

I have this function to ease out the task of inserting data into databases. I am not very sure if it is secure to use it this way.

Any suggestions on improving it?

function insert_into($query,$datatype) {



        //$i=0; num=1; $i=1; num=2; for(i=1;)
        $num=func_num_args()-1;
        global $CONN;//set the global connection variable on.
        for($i=2;$i<=$num;$i++) //assign the 'n' variables to an array
        {
            $values[$i]=mysqli_real_escape_string($CONN,func_get_arg($i));

            if(empty($values[$i])|| !isset($values[$i])|| $values[$i]=="")
            {
                die("<p><b>Fatal Error:</b> One or more parameters have an uninitialized or empty variable. You can't use an empty variable as a parameter.</p>");
            }
        }

        $blanks=str_repeat("?,", count($values)); //get the parameterized values as "?".
        $blanks=trim($blanks,","); //remove the extra ","

         $sql="INSERT INTO  ".$query." VALUES (".$blanks.");"; //make the query

         //set the connection

        $stmt= $CONN->prepare($sql); //prepare the query

        $bind=array();
        foreach ($values as $key => $val) {

            $bind[$key]=&$values[$key];
        }

        array_unshift($bind,$datatype);
        call_user_func_array(array($stmt,'bind_param'),$bind); // bind the variables

        #Replaced with above line ^ $stmt->bind_param($datatype, $xid);

        if($stmt->execute())    // set parameters and execute
    {
            $stmt->close();
        return 1; //sucessful

        } else {
            echo mysqli_error($CONN);
            $stmt->close();
            return 0;
        }
            $stmt->close();
            //close anyway

    }


     //SAMPLE: set_conn(); insert_into("tablename (stuff1, stuff2, stuff3)","sss",$val1,$val2,$val3);
    // set_conn() is another function which sets the connection to the database.
    //The above function returns 1 on success.

Does this have some serious issues like SQL injection or any other vulnerabilities? Thanks!

Answer 1

My suggestion is to simply abandon this piece of code. I don't know what value that it is adding. Trying to create some super flexible function that can query any table in any manner is just adding complexity to your application. How is making this function call easier than just working directly with mysqli in your calling code?

The sending of partial queries and concatenating them to the rest of the query inside this function just serves to obfuscate your code. Your query is now written in two places rather than one!

Additionally, you have to do things like parse apart the string passed as parameter to figure out how to formulate the query that is to be made. That is added complexity (and potentially fragility) that is just not needed.

I will also offer some thoughts around general coding style that you should considered applying whether or not you use this code.

• Avoid using global to make a variable available in function scope. You are much better off passing the dependency to the code that needs it as a parameter. This allows you to do things like type hinting the parameters to validate you have the proper dependency before working with it (like a valid mysqli object for example).
• Make sure you do not just consider happy path. For example when working with your DB object, what happens if prepare() fails? How does your code recover?
• You have too much vertical white-space (i.e. empty lines) in your code where there isn't really a need for it.
• Some of your lines of code are too long, making your code harder to read. Consider trying to limit line length to ~80 characters.
• Your code indentation (especially around brackets) is inconsistent. Do this in a consistent fashion so it is clear at a glance where code is nested.
• Don't echo out errors to standard output. Log them!
• Don't have unreachable lines of code like you have with your last $stmt->close() call. Your if-else covers all cases here, so this is totally unnecessary.
• You should not use mysqli_real_escape_string() in combination with parametrized prepared statements. You are going to potentially munge your data by doing this.
• A function should only do one thing. Don't have a function both change data in a database as well as provide HTML-formatted display for error conditions like you have here. In this case if you have "fatal error", don't die in this function. Have the function simply report the failure to the calling code (ideally via throwing an Exception) and let the calling code figure out what to do about the error.