Fastest way to insert large form into SQL database using PHP OOP and native SQL?

Question

First of all, I am new to PHP OOP and I'm still writing my code so I cannot paste the finished code here.

1. The form

<form method='post'>
    <?php
        print '<tr class="oddeven">';
        print '<input type="hidden" name="rowid" value="' . $row['rowid'] . '"/>';
        print '<td>' . $row['rowid'] . '</td>';
        print '<input type="hidden" name="firstname" value="' . $row['firstname'] . '"/>';
        print '<input type="hidden" name="lastname" value="' . $row['lastname'] . '"/>';
        print '<td>' . $row['firstname'] . ' ' . $row['lastname'] . '</td>';
        ... and so on ...
        print '</tr>';
?>
<input class="butAction" type="submit" name="submit2" value="<?php print $langs->trans('UpdateRecords'); ?>">
</form>

2. Post and insert the data

So if I have like 2 or 3 fields, it is no problem to write the appropriate post and method like:

$var1 = $_POST['rowid'];
$var2 = $_POST['firstname'];
$var3 = $_POST['lastname''];
$object->insertData($var1, $var2, $var3);

and

    public function insertData($rowid, $firstname, $lastname)
    {
        $this->db->begin();
        $sql  = 'INSERT ';
        $sql .= 'INTO tablename (rowid, firstname, lastname) ';
        $sql .= 'VALUES (' . $this->db->escape($rowid) . ', ' . $this->db->escape($firstname) . ', ' . $this->db->escape($lastname) . ')';
        $resql = $this->db->query($sql);
        if ($resql) {
            $this->db->commit();
        } else {
            $this->db->rollback();
            print_error($this->db);
            return -1;
        }
    }

But I have like 20 values I have to insert, plus there will be some additional calculations.

So what is the better and fastest way to do this? As mentioned above, or use a function like:

    public function __set($name, $value)
    {
        $this->$name = $value;
    }

and assign values in $_POST?

You are overthinking this. Your proposed class doesn't solve the issue at all, instead it creates another one (which you've noticed) - it inserts each column into a separate row. The core of the issue - you really don't need to create so many variables. That's what arrays are for; use them. Pass the entire `$_POST` to your method that builds the query, use keys for column names and values for... well, values. Though you really shouldn't couple your view (form input names) with your infrastructure (DB column names), but that's a different (and lengthy) topic. — El_Vanja, May 26 '21 at 11:46
And look into using [prepared statements](https://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) against SQL injection. [Escaping is not safe enough](https://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string). Plus, PDO's `execute` can take an array of parameters as its input argument, so it'd be a win-win situation for you here. — El_Vanja, May 26 '21 at 11:48
**Warning:** You are wide open to [SQL Injections](https://php.net/manual/en/security.database.sql-injection.php) and should use parameterized **prepared statements** instead of manually building your queries. They are provided by [PDO](https://php.net/manual/pdo.prepared-statements.php) or by [MySQLi](https://php.net/manual/mysqli.quickstart.prepared-statements.php). Never trust any kind of input! Even when your queries are executed only by trusted users, [you are still in risk of corrupting your data](http://bobby-tables.com/). [Escaping is not enough!](https://stackoverflow.com/q/5741187) — Dharman, May 26 '21 at 11:50
This is like an extension/plugin to existing software. Unfortunately, I cannot use prepared, as it is not implemented into the source code (I've tried before). Plus, all (hundreds) SQL queries in the source code are written this way. As described in their documentation > By default everything that goes in POST or GET is not parsed main.inc.php and throws you if something weird like SQL injection or script goes through it. You use $this->db->escape($mavariable). The "ORM" (/htdocs/core/db/…) does the rest. — milenmk, May 26 '21 at 12:04
@milenmk All PHP versions have prepared statements. Your abstraction class must have prepared statements as it's the most correct way to create SQL — Dharman, May 26 '21 at 12:05
`all (hundreds) SQL queries in the source code are written this way`...then you need to make some time to fix them. Security is a serious thing, don't just dismiss it because it sounds like a lot of work. — ADyson, May 26 '21 at 12:11
@ADyson > By default everything that goes in POST or GET is not parsed main.inc.php and throws you if something weird like SQL injection or script goes through it. You use $this->db->escape($mavariable). The "ORM" (/htdocs/core/db/…) does the rest. — milenmk, May 26 '21 at 12:22
Well we don't know exactly what your `escape()` function does but chances are there are ways to break it, even if they are obscure. "Trimming some characters" is unlikely to be foolproof, and could even corrupt valid data in some circumstances potentially (depending again on precisely what it's doing.) Parameterisation is the only 100% way to avoid injection attacks and preserve data integrity. Anything else is just hopeful guesswork. We also have no idea what "ORM" you are talking about, btw. — ADyson, May 26 '21 at 12:27
Please note that this question is on its way to deletion. If you want to preserve it so that other people can benefit from it, you must shorten it a little bit and stick to asking one straight forward question instead of "couple of questions". When editing make sure that you do not invalidate the answer. — Dharman, Aug 21 '21 at 20:46

Dharman · Accepted Answer · 2021-05-26T13:26:19.533

3

You are overcomplicating the design. Most importantly, your queries are already vulnerable to SQL injection. By overcomplicating this you are only introducing more bugs. Keep it simple!

Create a simple model class, which will have a method to process your POST request.

class UserController
{
    public function __construct(private mysqli $db)
    {
    }

    public function processPost()
    {
        // instead of an array create DTO here
        $data = [
            'firstname' => filter_input(INPUT_POST, 'firstname') ?: throw new Exception('missing firstname'),
            'lastname' => filter_input(INPUT_POST, 'lastname') ?: throw new Exception('missing lastname'),
            'gender' => filter_input(INPUT_POST, 'gender') ?: null,
            // etc.
        ];

        $userModel = new User($this->db);
        $userModel->insert($data);
    }
}

class User
{
    public function __construct(private mysqli $db)
    {
    }

    public function insert(array $data)
    {
        $stmt = $this->db->prepare('INSERT INTO users (firstname, lastname, gender) VALUES(?,?,?)');
        $stmt->bind_param(str_repeat('s', count($data)), ...$data);
        $stmt->execute();
    }
}

You can call it from your application the usual way you would call any other controller class.

$userController = new UserController($mysqli);
if($_POST) {
    $userController->processPost();
}

edited May 26 '21 at 13:26

answered May 26 '21 at 12:05

Dharman

30,962
25
85
135

I get ` unexpected 'private' (T_PRIVATE), expecting variable (T_VARIABLE)` for `public function __construct(private PDO $db)`. The constructor in other files is like ```public function __construct($db) { $this->db = $db; }``` I have to make it only `(PDO $ds)` to get rid of the error. And then I have 'unexpected 'throw'' for the `processPost()` – milenmk May 26 '21 at 12:16
You must be using an old version of PHP. This is the new syntax of PHP 8.0. For PHP 7.4 just use the old style constructor and replace `throw new exception` with `trigger_error` or something similar – Dharman May 26 '21 at 12:40
That worked, but I had to rename the classes names, as the user is already used. got `Uncaught TypeError: Argument 1 passed to BgSalaryUserController::__construct() must be an instance of PDO, string given`. Form post is ```$var2 = $_POST['user_firstname' . $i . '']; $var3 = $_POST['user_lastname']; $var4 = $_POST['user_salary']; $object = new BgSalaryUserController($var2, $var3, $var4);``` Images of the code here: https://ibb.co/ZBktkDz https://ibb.co/RzTj8kJ – milenmk May 26 '21 at 13:00
@milenmk Your constructor takes your dependencies, not your data. The data is read from the filter_input. Your database connection is your dependency. – Dharman May 26 '21 at 13:06
Your second screenshot looks like you are using mysqli API... with some other errors. In that case you might want to change the code to use positional arguments. However, I would strongly advise upgrading from mysqli to PDO. – Dharman May 26 '21 at 13:13
Upgrading to PDO depends on the system code developers. I've mentioned in the above comments that I cannot use PDO. All I have is mysqli, pgsql and sqlite3 classes. – milenmk May 26 '21 at 13:17
@milenmk I don't see that in the question, maybe you forgot to mention that. Anyway, I changed the syntax to mysqli. It's really not that much different – Dharman May 26 '21 at 13:26

Fastest way to insert large form into SQL database using PHP OOP and native SQL?

1 Answers1