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);
?>

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.