Database input script

Question

Is this code secure and valid? If so, can it be improved?

<?php 
 // conect to the data base
 $mysqli = new mysqli('localhost','user','pass','dbname');
 if($mysqli->connect_errno >0){
 die( "problem with the connection");
 }
 // creat a function to get table information
 function fetchTableFeildNames($mysqli , $databaseName , $tableName ){
 $sql = "SELECT `COLUMN_NAME` FROM `INFORMATION_SCHEMA`.`COLUMNS` WHERE `TABLE_SCHEMA`= '$databaseName' AND `TABLE_NAME`= '$tableName' ";
 if(!$stmt =$mysqli->prepare($sql)){
 die("There is an error with mysql preperation staetment".$mysqli->error);
 }
 if (!$stmt->execute()) {
 echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
 }
 $res = $stmt->get_result();
 //looping throught the result 
 while ($array = $res->fetch_array()){
 $newArray[] =$array;
 }
 return $newArray;
 }
 //insert function 
 function insert($mysqli,$tableName,$databaseName,$array){ 
 //fetching table columns information
 $tableColumns = fetchTableFeildNames($mysqli , $databaseName , $tableName );
 // geeting table columns count
 $tableColumnsCount = count($tableColumns);
 //geting data array count 
 $dataArrayCount = count($array);
 //see if the table columns count dosenot mutch array data count we kill the script 
 if ($tableColumnsCount != $dataArrayCount){
 die("Data array dosenot mutch table columns count");
 }
 //forming the query depending on input array at same time we are geting data typs for the data array
 $sql = "INSERT INTO `{$databaseName}`.`{$tableName}` VALUES (";
 for($i=0;$i<$dataArrayCount;$i++){
 $string = gettype($array[$i]);
 //takeing the first litter example string= s ,integer =1
 $a_param_type[] = $string[0];
 $sql .= "?,";
 }
 //remove the last char ","
 $sql = rtrim($sql , ",");
 $sql .= ")";
 $a_params = array();
 $param_type = '';
 $n = count($a_param_type);
 //merging all types in one string
 for($i = 0; $i < $n; $i++) {
 $param_type .= $a_param_type[$i];
 } 
 //passing the types to the array by refrence 
 $a_params[] = & $param_type;
 //passing the data array by refrence 
 for($i = 0; $i < $n; $i++) {
 $a_params[] = & $array[$i];
 }
 //preperaing the statment
 if(!$stmt = $mysqli->prepare($sql)){
 die("error in preperation " . $mysqli->error);
 }
 //using call_user_func_array() because mysqli::bind dont work with dynamic parameters 
 call_user_func_array(array($stmt, 'bind_param'), $a_params);
 //executing the query
 if(!$stmt->execute()){
 die("error in executeing the query".$mysqli->error);
 } 
}
 $array = array ("","catName" ,"catDesecription","","","","");
 insert($mysqli,"items","yaztor",$array);
?>

• Brandon - Free Palestine: I will say a few things. Your formatting is horrible (take a look at PSR standards). You should separate the functions into their own file (e.g. db_functions.php and include it). You should avoid naming things as simple as insert, its too generic. Maybe db_insert or insert_row, etc. Just a few pointers.

Answer 1

There are some formatting and spelling issues. I fixed those (somewhat arbitrarily changing to my style), removed redundant comments, and made a few other changes:

<?php 
 function fetchTableFieldCount($mysqli , $databaseName , $tableName ) {
 $sql = "SELECT COUNT(`COLUMN_NAME`) AS FieldCount FROM `INFORMATION_SCHEMA`.`COLUMNS` WHERE `TABLE_SCHEMA`= ? AND `TABLE_NAME`= ? ";
 if ( ! ( $stmt = $mysqli->prepare($sql) ) ) {
 die("There is an error with mysql preparation statement".$mysqli->error);
 }
 $stmt->bind_param($databaseName, $tableName);
 if ( ! $stmt->execute() ) {
 echo "Execute failed: (" . $stmt->errno . ") " . $stmt->error;
 }
 $res = $stmt->get_result();
 if ( $row = $res->fetch_array() ) {
 return $row['FieldCount'];
 }
 return 0;
 }
 function insert($mysqli, $tableName, $databaseName, $array) {
 //see if the table columns count does not match array data count we kill the script 
 $tableColumnsCount = fetchTableFieldCount($mysqli, $databaseName, $tableName);
 $dataArrayCount = count($array);
 if ( $tableColumnsCount != $dataArrayCount ) {
 die("Data array does not match table columns count");
 }
 if ( 0 == $dataArrayCount ) {
 return;
 }
 $sql = "INSERT INTO `{$databaseName}`.`{$tableName}` VALUES (";
 $sql .= implode(', ', array_fill(0, $dataArrayCount, '?'));
 $sql .= ')';
 for ( $i = 0; $i < $dataArrayCount; $i++ ) {
 $string = gettype($array[$i]);
 //taking the first letter; example: string = s, integer = i
 $a_param_type[] = $string[0];
 }
 if ( ! $stmt = $mysqli->prepare($sql) ) {
 die("error in preparation " . $mysqli->error);
 }
 // make the first element of the array a string representing the types
 // of the values of the rest of the array
 array_unshift($array, implode($a_param_type));
 // use call_user_func_array to pass the variable number
 // of elements in the $array as arguments
 call_user_func_array(array($stmt, 'bind_param'), $array);
 if ( ! $stmt->execute() ) {
 die("Error in executing the query: " . $mysqli->error);
 }
 }
 $mysqli = new mysqli('localhost','user','pass','dbname');
 if ( $mysqli->connect_errno > 0 ){
 die( "problem with the connection");
 }
 $array = array("", "catName" ,"catDescription", "", "", "", "");
 insert($mysqli, "items", "yaztor", $array);

First, I moved the connection info from the top to the bottom. This puts all the commands outside of a function together. Normally we'd move the functions into a different file and require it, but I skipped that here.

Next, I removed the first redundant comment. A comment like "creat [sic] a function to get table information" doesn't tell me anything that function fetchTableFeildNames [sic] doesn't already tell me. Save comments for explaining why you chose to do something. That's what you'll forget before you have to go back to the code.

I changed the first function to get just the column count, as that's all we're using. No need to ship the column names across the wire.

I changed $array to $row. Row describes what the variable holds. Array doesn't differentiate it from any other array. I dropped $newArray entirely, but if needed, I would have called it $columnNames.

I added a check for an array with no elements. This avoids a failed SQL query in that case.

I took the $sql concatenation out of the for loop. This allows for the more efficient implode rather than a series of string concatenations.

I took out the extraneous for loop that copied the array. The $array variable itself is a copy, so I just used that. I also took out the manual splice in favor of array_unshift. I would expect a built-in function to be faster than a for loop in PHP.

I didn't remove your column count check, but I don't know that it's necessary. It removes one thing that can go wrong but leaves others. It's not like you recover from the mistake. It ends the program. The same thing would happen with an invalid INSERT to the database. I don't know that you gain much.

The code that you show here is reasonably safe and secure. However, it was not reliably so. It relied on the caller to secure $databaseName and $tableName. If a caller bases either of those on user input, there's risk.

• yasir: thanks for these awesome changing iam gona sure to change my way depending on what u presented . but there is one problem this errorWarning: Parameter 2 to mysqli_stmt::bind_param() expected to be a reference, value given in
• Elias Van Ootegem: There is a huuuge problem with the code you posted: You're using mysqli::prepare, passing variables to the string to prepare. That's just not done. When you use prepared statements (which you should) you never pass variables to the base query string. You use placeholders, then bind the params and execute the statement.