CODEWITHSUNDEEP
phppdo
Ray
Ray

Reputation: 3060

creating a flexible update query with Php and pdo - problems with bindparam

I'm updating my mysql functions to use PDO. I've got the hang of most of it but struggling with an update function to update multiple fields in a records. The function is in a class and I'm trying to keep it flexible to reuse with other tables etc.

Here's my function so far:

 public function dbUpdateRecord($table, $values, $where)
{


    $this->conn();
    $fieldNames = array_keys($values);
    var_dump($fieldNames);
    $set="";
    foreach ($fieldNames as $field) {
        $set .= " $field = :$field,";
    }

    //strip last comma
    $set = substr($set, 0, strlen($set) - 1);


    $wherefields = array_keys($where);
    $whereCondition="";
    foreach ($wherefields as $field) {
        $whereCondition .= " $field = :$field AND";
    }
    //strip last AND
    $whereCondition = substr($whereCondition, 0, strlen($whereCondition) - 3);

    $sql = "UPDATE $table SET $set WHERE $whereCondition";
    var_dump($sql);

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

    foreach ($values as $field => $value) {

        $stmt->bindParam(':$field', $value);
    }
    foreach ($where as $field => $value) {
        $stmt->bindParam(':$field', $value);
    }
    return $stmt->execute();
}

The problem is all the fields in the record are being updated by the id of the record which is contained in the $where variable.

$values contains an array of (fieldname=>value).

I think the problem lies around the bindparam and trying to make the fieldnames/placeholders dynamic

I thought I needed to use bindparam as best practice - is this correct or can I just go to execute()?

ANy help appreciated

Upvotes: 0

Views: 2709

Answers (3)

chapka
chapka

Reputation: 490

I came here because I was having the same problems, and YCS's solution was what I needed. For anyone else in this situation, here's the helper function I ended up using:

function commit_table($record_id, $changed_values) { $db = open_database();

$query = 'UPDATE table SET ';
$query_arguments = array();

$is_first = TRUE;

foreach(array_keys($changed_values) as $key)
{
    if($is_first)
    {
        $is_first = FALSE;
    }
    else
    {
        $query .= ', ';
    }
    $value_var = ':' . $key;
    $query .= $key;
    $query .= ' = ';
    $query .= $value_var;
    $query_arguments[$value_var] = $changed_values[$key];
}

$query .= ' WHERE record_id = :record_id';
$query_arguments[':record_id'] = $record_id;

$stmt = $db->prepare($query);
$stmt->execute($query_arguments);

close_database($db);

}

Upvotes: 0

Your Common Sense
Your Common Sense

Reputation: 157880

You are lifting this log from the wrong end.
Your approach is potentially insecure yet inflexible at the same time. What if you need a JOIN based update? What if you need OR in the WHERE (or IN)?

What you really need is a conventional query where only SET statement values have to be generated. So, you need a helper function to produce such a statement out of data array, returning both correctly formatted SET statement and array with variables to be bound:

$fields = array("name","email");
$sql = "UPDATE users SET ".pdoSet($fields,$values,$data)." WHERE id = :id"
// now we have $values array to be passed into query
$stmt = $dbh->prepare();
$values["id"] = $_POST['id'];
$stmt->execute($values);

With this code you'll be able to make updates for the arbitrary query. And make it safe.

As a further step you will need to start using type-hinted placeholders, to make whole code like this:

$db->query("UPDATE ?n SET ?u WHERE id IN(?a)",$table,$data,$ids);

Getting back to your problem, ONe is right - you need to use bindValue instead of bindParam (as it mentioned in the tag wiki)

Upvotes: 2

Jose Armesto
Jose Armesto

Reputation: 13759

I believe the problem is that you are using a foreach to bind the params to the query. Why is this a problem? Because when you bind a variable, you bind a reference to that variable, so if that variable changes, the value in the query will change too. Since you are using a foreach loop, the value for all the parameters will be the latest value that the variable $value referenced to.

You can read more about this foreach behavior here and here. So basically, you have 2 options:

  • Use a reference to the actual value, instead of using a reference to $value (which can change its value in the next iteration)
  • Use an auxiliar variable that references another memory position that won't change during the loop

Upvotes: 1

Related Questions