1
\$\begingroup\$

I built a function that should greatly simplify working with long HTML forms, that save data to MySQL using PHP.

My forms usually have several hundred input elements that are added / altered / removed frequently from the form.

Goal of this function is to only write frontend HTML form markup, MySQL structure gets updated automatically once form is submitted. If new form is not yet being stored into a table, table is automatically created.

<?php
$mysqli = mysqli_connect('127.0.0.1', 'root', '', 'test');
$mysqli->set_charset('utf8');

function SubmitForm($mysqli, $table) {
    if(isset($_POST['submit'])) {
        // unset redundant $_POST fields
        unset($_POST['submit']);

        // create table if not exists
        $mysqli->query('create table if not exists `'.$table.'` (`id` BIGINT NOT NULL AUTO_INCREMENT PRIMARY KEY, `status` INT(11) NOT NULL DEFAULT "1", `date_time` datetime NOT NULL DEFAULT CURRENT_TIMESTAMP, `code` VARCHAR(100) NOT NULL, UNIQUE KEY `code_unique` (code)) ENGINE=MyISAM DEFAULT CHARSET=utf8') or die($mysqli->error);

        // prepare data
        $next_after = null; // used to add columns in order of $_POST fields
        $columns = $placeholders = $types = $values = $duplicates = array();
        foreach ($_POST as $key => $value) {
            // add column if not exists
            $column_query = $mysqli->query('show columns from `' . $table . '` like "'.$key.'"') or die($mysqli->error);
            if ($column_query->num_rows === 0) {
                $mysqli->query('alter table `' . $table . '` add `' . $key . '` text NOT NULL ' . $next_after) or die ($mysqli->error);
            }

            // set AFTER for next column check query
            $next_after = 'after `' . $key . '`';

            // implode arrays that are passed from select[multiple='multiple']
            $value = is_array($value) ? implode(',', $value) : $value;

            // prepare data for query
            $columns[] = $key;
            $placeholders[] = '?';
            $types[] = 's';
            $values[] = $value;
            $duplicates[] = $key . ' = values(' . $key . ')';
        }

        // insert / update data
        $stmt = $mysqli->prepare('insert into '.$table.' ('.implode(',', $columns).') values('.implode(',', $placeholders).') on duplicate key update '.implode(',', $duplicates)) or die($mysqli->error);
        $stmt->bind_param(implode('', $types), ...$values);
        $stmt->execute();

        // return success
        return 'form saved';
    }
    return null;
}
echo SubmitForm($mysqli, 'table_name');
?>
<form method="post">
    <input type="text" name="code" value="<?php echo time(); ?>" />
    <input type="text" name="apples" />
    <input type="text" name="bananas" />
    <input type="text" name="grapes" />
    <input type="submit" name="submit" />
</form>
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Incorporating advice from an answer into the question violates the question-and-answer nature of this site. You could post improved code as a new question, as an answer, or as a link to an external site - as described in I improved my code based on the reviews. What next?. I have rolled back the edit, so the answers make sense again. \$\endgroup\$ Commented Jun 14, 2021 at 6:49

1 Answer 1

5
\$\begingroup\$

It looks like you are throwing caution to the wind here. I mean you are granting full control of your database structures to the end user and offering the raw mysqli errors when they arise. These are things that I would not entertain. I will hope that this is merely an academic exercise and that this would never be deployed to a production server. What happens when someone decides that they feel like writing a script the will send an obscene amount of POST requests to your server (in a loop)? How are you planning to protect your system? I don't see any protections.

Beyond this glaring concern, smaller pieces of advice are:

  • Use ALL CAPS when writing sql keywords/functions (for clarity/readability).
  • Rather than pushing then imploding $types, just assign the types elements as the first element of $values, then use concatenation on the first element of the array (since it will always be the types string). Then you can just spread $values.
  • Imploding multi-select values as commas is another ill-considered technique. You are intentionally introducing a denormalized structure. With more effort, you can program a normalized design whereby related tables hold individual values. Think about the simple case where the form selects Cleveland, Ohio and New York, New York -- the intention is to save 2 values but because you are joining them with commas in the db column, it looks like you have 4 values for that row.

Basically, I'm seeing a lot of sacrifices for flexibility at the obvious cost of security, performance, and memory.

\$\endgroup\$
1
  • \$\begingroup\$ I agree allowing all users to alter database structure is a big security risk. On a production server the part that adds new columns will only work for users that are admins... if($this_user === 'Admin') { /* code that alters tables */ } \$\endgroup\$
    – John Doe
    Commented Jun 10, 2021 at 8:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.