PDO update method

Question

I am in my first object-oriented project and wrote this method for updating the database. The PDO include is just for convenience, so I could work on a single file temporarily. I'm not entirely clear on how or why to use the __constructor for this particular situation.

<?php

include 'pdo.php';

class database{

function update($table, $columns, $data, $where, $pdo) {

    foreach ( $columns as $column ) {
        $update .= " " . $column . " = :" . $column . ", ";
    }
    $update = trim($update, ", ");
    $where_prepare = " " . $where[0] . " = :" . $where[0];
    $prepare = "UPDATE {$table} SET {$update} WHERE {$where_prepare}";

    $counter = 0;
    $execute_data = array();
    foreach ( $columns as $column ) {
        $execute_data[":{$column}"] = $data[$counter];
        $counter++;
    }
    $where_0 = $where[0];
    $execute_data[":{$where_0}"] = $where[1];

    $stmt = $pdo->prepare($prepare);
    $stmt->execute($execute_data);
    $affected_rows = $stmt->rowCount();

    if ( $row_count < 1 ) {
        return false;
    } else {
        return true;
    }
}

}

$db = new database;
$is_updated = $db->update('image', array('imgTitle', 'orientation'),  array('new title','V'), array('img_id', '4'), $pdo);

Answer 1

SQL Injection

Multiple parameters can be used for SQL injections: $columns, $where (the first entry), and $table.

You could use a whitelist to filter the input, filter out all non-alphanum characters, or properly document that there is no security in place for these parameters.

You might argue that those parameters aren't supposed to be user supplied anyways. But such a generic method as this needs to be secure because it can be used in so many different ways, and especially the $where or possibly $columns values may be user supplied in the future (I wouldn't worry too much about $table, as there are likely other problems than SQL injection if it is user supplied, but it is still a good idea to secure it as well).

Usage

update('image', array('imgTitle', 'orientation'),  array('new title','V'), array('img_id', '4'), $pdo)

This isn't really nice to read. Without actually looking at the method signature, it is really difficult to see what which parameter is supposed to do.

There are two things that may help here:

1. Named array indices
2. A query builder

An example for 1. may be this:

update($pdo, 
    ["table" => "myTable",
    "columns" => ["col1", "col2"],
    "values" =>["val1", "val2"],
    "where" => [["column" => "col", "value" => "val"]]]);

The advantage of this approach is that it is rather simple to write for you. The disadvantage is that it is rather difficult to use, especially without very good documentation.

An example for a query builder may be this:

$builder->update("myTable")
        ->where("col", "val")
        ->set("col1", "val1")
        ->set("col2", "val2")
        ->execute();

or maybe

$builder->update("myTable")
        ->where("col", "val")
        ->set(["col1" => "val1", "col2" => "val2"])
        ->execute();

These calls would be a lot easier to read than your initial call.

Misc

• be consistent with your names. Same things should have the same name(-structure). For example, $where_prepare contains _prepare, but $update, which functions the same way, does not.
• $update isn't a great name. I'm having trouble coming up with a better name myself, but something like $setValuePairs could work.
• one-time variables are only needed if they have a good name that increases readability. This isn't the case for $where_0 = $where[0] though.
• class names should be upper-cased.
• $data would be better named as $values.
• I'm not entirely clear on how or why to use the __constructor for this particular situation: You could pass the pdo object in the constructor, if you'd like. It's the same for each query, so you would not need to pass it to the functions anymore.