HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Database input script

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mysqliphpcodereviewstackoverflow
databaseinputscript

Problem

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

``
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;$iprepare($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(!

Solution

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: 

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

Code Snippets

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

Context

StackExchange Code Review Q#68125, answer score: 2

Revisions (0)

No revisions yet.