Forms - Ordered fields vs. dynamic iterations

I've made about 2 forms that have a server side action using PHP. For the first one, I made a variable for every form field and mapped it to $_POST["name"]. For small amount of fields, it's not a problem, but it's tiring for large amounts.

For my second form, I used the scripts below. The problem I had with it was how unflexible it was, due to the order of the HTML fields and DB column names having to match. At the end of this post, I also attempt to provide a fix to it.

Here's a simplified example to the forms, and how I've been approaching them.








label {
line-height: 200%;
}


First Name:




Last Name:




Street Address:




State:




Zip Code:




Phone Number:








Here is the database table associated with the form (the columns aren't necessarily supposed to be in order with the form fields):

+------------------------+
| column_name            |
+------------------------+
| applicant_id           |
| first_name             |
| last_name              |
| address                |
| city                   |
| state                  |
| zip_code               |
| phone_number           |
+------------------------+

I have a loop in a separate script that iterates through, and adds each $_POST value from the form (or $_POST array) in order into an empty array:

$input_array = []; 
//testInput() is the sanitation
foreach ($_POST as $key => $val)
{   if ($_SERVER["REQUEST_METHOD"] == "POST") {
        if (isset ( $_POST [$key] ) && ! empty ( $_POST [$key] )) {
            $input_array[] = testInput($val); 
        }
    }
}

This $input_array is the the array I provide as the $postArray parameter in the buildArr($postArray, $handle, $tableName) function shown later.

This is the script that:

• First gets the column names from the database and builds an ar

There's a few things I noticed that could be improved:

In the following code snippet, checking isset and !empty are literally the same, exact thing.

If the content isn't empty, then it's set.

Also, you shouldn't be checking $_SERVER["REQUEST_METHOD"] for every item in $_POST.

You can take the request method check out the front, and return false in the case it doesn't meet your conditions.

foreach ($_POST as $key => $val)
{   if ($_SERVER["REQUEST_METHOD"] == "POST") {
        if (isset ( $_POST [$key] ) && ! empty ( $_POST [$key] )) {
            $input_array[] = testInput($val); 
        }
    }
}

With those changes in mind, the following code snippet would be a much better alternative:

if ($_SERVER["REQUEST_METHOD"] == "POST"){ die("UNALLOWED ACCESS METHOD"); }
foreach ($_POST as $key => $val){
    if (!empty($_POST[$key])){
        $input_array[] = testInput($val);
    }
}

Next, getColumnStrings(); this function doesn't actually get strings (plural), but builds a string (singular).

You shouldn't be iterating over every element in $columnArray like that, for future reference, you can use a foreach loop, but, you should use a implode() here, rather than a loop.

Using an implode() lets you join all the strings together, at those joining points, append a string (',').

You shouldn't build SQL Queries as strings like that.

Use bind_param() instead.

function getColumnStrings($tableName, $handle) {

   $columnArray = getColumnNames($tableName, $handle);
       $size = sizeof($columnArray);
       $sql = "(";
       for($i = 0; $i < $size; $i++) {
           $sql .= $columnArray[$i] . ",";

       }
       $sql = rtrim($sql, ",");
       $sql .= ")";
       $values = "VALUES (";
       for($i = 0; $i < $size; $i++) {
           $values .= ":" . $columnArray[$i] . ",";

       }
       $values = rtrim($values, ",");
       $values .= ")";

       return "INSERT INTO " . $tableName . " ". $sql . " " . $values;

   }

Commenting solely on the $values building as I'm not as experienced with bind_param() as I'd like to be:

function getColumnStrings($tableName, $handle) {
        $columnArray = getColumnNames($tableName, $handle);
        $sql = "(". implode(", ", $columnArray) . ")";
        $values = "VALUES (";
        $valueArray = [];
        foreach($columnArray as $item){
            $valueArray[]= ":" . $item;
        }
        $values = "VALUES (" . implode(", ", $valueArray) . ")";

        return "INSERT INTO {$tableName} {$sql} {$values}";

    }

That is, simply an improvement of what you have: IT IS JUST AS SAFE AS YOUR IMPLEMENTATION, WHICH IS: not very.

Rather than having sizeOf()s everywhere, use foreach.

StackExchange Code Review Q#97161, answer score: 3