Is this function properly structured, leading to a secure row insertion?

Question

I have the following function to insert a new row into a table:

/**
 * Insert row into the designated table
 * 
 * Function to add a new row into the designated table
 * 
 * @param str $table            Table name to insert the row
 * @param arr $data             Array with values
 * 
 * @return int row              Last inserted ID
 */
public function database_add_row($table, $data) {

    try {
        $fields = '';
        foreach ($data as $field) {
            $fields.= '?,';
        }
        $fields = substr($fields,0,-1);
        $sql = "INSERT INTO ".$table." VALUES(".$fields.")";
        $sth = $this->database->prepare($sql);
        $sth->execute($data);
        return $this->database->lastInsertId();

    }
    catch(PDOException $e) {
        throw new userman_Exception("<h1>ups!</h1><br/>". $e->getMessage());
    }

}

As to prevent sending the field names, leading to a table agnostic function, I'm counting the array of values to prepare the query, subtracting the last comma from the generated string.

Considerations:

• All columns values are passed with the $data array.
• The function is designed to insert on any table with any number of columns.

My question is:

Is my method adequate, leading to a secure row insertion?

Answer 1

As noted in your other question, that exception handling completely masks the true exception. It adds no value yet detracts. There's no reason to do that. Just allow the exception to throw out of the function.

---

As for 'security', assuming you properly control the table name, it is safe. (A better way to phrase that may be that SQL injection is not possible as long as you control $table.)

---

I might consider writing it a bit differently though:

public function database_add_row($table, $data) {
    $placeholders = implode(', ', array_fill(0, count($data), '?'));
    $sql = "INSERT INTO {$table} VALUES ({$placeholders})";
    $stmt = $this->database->prepare($sql);
    if ($stmt->execute($data)) {
        return $this->database->lastInsertId();
    } else {
        return false;
    }
}

---

I also might consider using associative arrays. That would allow you to specify columns (which could be useful if you had default-valued columns or auto incrementing columns).

public function database_add_row($table, $data) {
    $cols = array_keys($data);
    $columns = implode(', ', $cols);
    $placeholders = implode(', ', array_map(function($c) { return ":{$c}"; }, $cols));
    $sql = "INSERT INTO {$table} ({$columns}) VALUES ({$placeholders})";
    $stmt = $this->database->prepare($sql);
    if ($stmt->execute($data)) {
        return $this->database->lastInsertId();
    } else {
        return false;
    }
}

Then you would use it like:

$obj->database_add_row("table1", array('column1' => 'val1', 'column2' => 'val2'));

---

Depending on how far you plan to go with your DB abstractions, you might want to consider a DBAL or ORM library. The Doctrine Project has quite widely used versions of both of those.

Answer 2

As long as the $table variable is properly scrubbed / initialized you are fine as far as secure goes.

However, I don't care for the insert statement format where you do not specify the column names. For this to work for any insert statement for any table you would have to make sure that the array has been setup to include EVERY column in the table in the correct order.

For basic scenarios this may work but you are probably going to end up with bugs. You will get values inserted into the wrong columns and you will have a headache trying to figure out where it is occurring.