Secure row insertion using a position-based prepared statement

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("ups!". $e->getMessage());
    }

}

I'm counting the values in the array to prepare the query, and adding placeholders for each position, then subtracting the last comma from the generated string. This avoids using the field names, which leads to a table agnostic function,

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?

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.

StackExchange Code Review Q#15637, answer score: 3