Function that inserts values for an array of fields into a database table

Question

public function insert($table, $fields = array(), $values = array()) {
    $fields = '`' . implode ( '`,`', $fields ) . '`';
    $values = "'" . implode ( "','", $values ) . "'";
    $sql = "INSERT INTO {$table} ($fields) VALUES($values)";

    if ($this->_pdo->prepare ( $sql )) {
        if ($this->_pdo->exec ( $sql )) {
            return true;
        }
    }
    return false;
}

This is my first PHP OOP PDO insert function I have ever written. My function is working well, but I believe this is not the best way I would have written. Can somebody guide me on what I might have missed?

Answer 1

There's a couple of issues with it, for start your nested if blocks are unnecessary: instead of manually checking and returning true or false, just return the result of those methods, like so:

return $this->_pdo->prepare($sql) and $this->_pdo->exec($sql);

Additionally, you have made $fields and $values into optional parameters by making them auto-initialise to empty arrays, however you cannot have an INSERT INTO statement without specifying at least one field and value, therefore instead of making them optional array parameters, you should use type hinting to ensure they are arrays, and then check to make sure that the arguments are correct before you send the query off to the server, something like this:

public function insert($table, array $fields, array $values) {

    $numFields = count($fields);
    $numValues = count($values);

    if($numFields === 0 or $numValues === 0)
        throw new Exception("At least one field and value is required.");
    if($numFields !== $numValues)
        throw new Exception("Mismatched number of field and value arguments.");

    $fields = '`' . implode('`,`', $fields) . '`';
    $values = "'" . implode("','", $values) . "'";
    $sql = "INSERT INTO {$table} ($fields) VALUES($values)";

    return $this->_pdo->prepare($sql) and $this->_pdo->exec($sql);
}

Answer 2

Assuming the class property $_pdo is an instance of the PDO class, this SQL in vulnerable to SQL-injection attacks. When inserting data into a database it is quite common this is user input, which should never be trusted and sanitized carefully.

The reason this is vulnerable is because you inject the actual values directly into the SQL query. You are using prepared statements (great), but unfortunately wrong. The SQL query passed to the prepare method should take placeholders of the actual value instead. I recommend using named parameters. An example of such could be:

$sql = "INSERT INTO users (username, password) VALUES (:username, :password) LIMIT 1";

Here the placeholders are :username and :password. You should then bind the actual values to the query after is has been prepared. An example when using the SQL from above could be:

$username = 'awesomeness';
$password = 'secret';

$stmt = $this->_pdo->prepare($sql);

/*
 * Note: the actual value passed to the PDO::bindParam method
 *       must be a variable. You should therefore store the actual
 *       data inside a variable first.
 */
$stmt->bindParam(':username', $username, PDO::PARAM_STR);
$stmt->bindParam(':password', $password, PDO::PARAM_STR);

/*
 * Check if the query was executed. This does not check if any data was actually
 * inserted as MySQL can be set to discard errors silently.
 */
if(!$stmt->execute()) {
    throw new ErrorException('Could not execute query');
}

/*
 * Check if any rows was actually inserted.
 */
if(!$stmt->rowCount() == 0) {
    throw new ErrorException('Could not insert data into users table.');
}

return true;

This will greatly increase your resistence against SQL injection attacks. For your method to support this it should be restructured.

public function insert($table, array $data)
{

    /*
     * Check for input errors.
     */
    if(empty($data)) {
        throw new InvalidArgumentException('Cannot insert an empty array.');
    }
    if(!is_string($table)) {
        throw new InvalidArgumentException('Table name must be a string.');
    }

    $fields = '`' . implode('`, `', array_keys($data)) . '`';
    $placeholders = ':' . implode(', :', array_keys($data));

    $sql = "INSERT INTO {$table} ($fields) VALUES ({$placeholders})";

    var_dump($sql);

    // Prepare new statement
    $stmt = $this->pdo->prepare($sql);

    /*
     * Bind parameters into the query.
     *
     * We need to pass the value by reference as the PDO::bindParam method uses
     * that same reference.
     */
    foreach($data as $placeholder => &$value) {

        // Prefix the placeholder with the identifier
        $placeholder = ':' . $placeholder;

        // Bind the parameter.
        $stmt->bindParam($placeholder, $value);

    }

    /*
     * Check if the query was executed. This does not check if any data was actually
     * inserted as MySQL can be set to discard errors silently.
     */
    if(!$stmt->execute()) {
        throw new ErrorException('Could not execute query');
    }

    /*
     * Check if any rows was actually inserted.
     */
    if($stmt->rowCount() == 0) {

        var_dump($this->pdo->errorCode());

        throw new ErrorException('Could not insert data into users table.');
    }

    return true;

}

This code has not been tested thoroughly, but the basic functionality should be there.

The usage of the above function has been altered slightly as there are only 2 arguments now. The $data argument takes an associative array of the field name as key and field-value as array value.

$table  = 'users';
$fields = [
    'username' => 'awesomeness',
    'password' => 'secret'
];

try {

    /*
     * Have used the word 'object' as I could not see the actual 
     * class name.
     */
    $object->insert($table, $fields);

}catch(ErrorException $exception) {

    echo $exception->getMessage();  // Should be handled with a proper error message.

}

Hope this can help guide you, happy coding!