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!

• Pimgd: Where's $query coming from?
• twodee: the function usage is like this : insert_into("tablename (stuff1, stuff2, stuff3)","sss",$val1,$val2,$val3);, here $query is "tablename (stuff1, stuff2, stuff3)" @Pimgd
• Pimgd: Do you control that string? Can user input affect it?
• twodee: No, user input won't be affecting that $query, it will be defined at the codes. User inputs will be go through the values of $val1, $val2, $val3....
• Mike Brant: Why do you think you need a function like this? What value does it add?

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.

• twodee: My idea while making the function was to lessen the effort while making an insert query. Using it I can accomplish something for which I would have to write about 6-10 lines extra.
• Mike Brant: @CoderDudeTwodee But is writing a handful of extra lines when you need to do inserts really that big of a deal if it makes you code more readable and easier to maintain? Honestly would you should be thinking about doing is writing proper classes around your application objects, with each of these classes having the ability to interact with the database as necessary to perform CRUD operations for that type of object.
• twodee: Upvoting this after a year and nine months realizing how horrible code I used to write and how good the answer is and how patient you were dealing with my unreadable code. @MikeBrant
• Mike Brant: @2dsharp learning how to write good software is a journey. Glad that you have reached the point in yours where you can begin looking at code you have written in the past and understanding the mistakes you made in writing it. :)