2

Im stuck on this one, recently switched to PDO to learn myself.

/**
* update
* @param string $table A name of a table to update into
* @param string $data An associative array.
* @param string $where WHERE = ?.
*/
public function update($table, $dataArr, $where)
{
    $fieldDetails = NULL;

    foreach( $dataArr as $key => $value)
    {
        $fieldDetails .= "`$key` =:$value, ";
    }
    $fieldDetails = rtrim($fieldDetails,', ');

    echo "UPDATE $table SET ($fieldDetails) WHERE (`id`=:$where)";


    $stmt = $this->prepare("UPDATE $table SET ($fieldDetails) WHERE (`id`=:$where)");
    foreach($dataArr as $key => $value) 
    {
        //Binder key till värde.
        $stmt->bindValue(":$key", $value);
    }
    $stmt->bindValue(":$where", $where);
    $stmt->execute();

}

My insert function works like a charm, but this update function won't work. I think it has to do with the id no being bind. I have searched in documentation and threads but can't find an solution.

My function call.

public function update()
{
    $this->db->update(
    'testtable',
    array(
    'text' => 'exempel',
    'name' => 'exempel',
    ), 0);
}

Warning: PDOStatement::execute() [pdostatement.execute]: SQLSTATE[HY093]: Invalid parameter number: parameter was not defined in

How do I correctly bind the integer value I pass in with the function, to so that the statement can be executed ?.

egilviking
  • 29
  • 2

2 Answers2

0

Edit:
Well, I overlooked the code, my apologies.
Anyway, your code is open to SQL injection, and it would be better to use a solution from the PDO tag wiki.

By the way, when using not a stone-age libraries like PDO, but something more useful, you won't need no update() function at all, as you can write simply

$db->query("UPDATE ?n SET ?u WHERE id=?i",$table, $dataArr, $id);

please note that this latter code is much safer than yours, yet way more flexible:

  • it does protect table name
  • it does protect field names
  • it lets you not only id-based WHERE but whatever condition.
  • it lets you whatever syntax (even JOINs) actually
Community
  • 1
  • 1
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • What libs do you recommend that not has a too high learning curve ? – egilviking Mar 12 '13 at 13:02
  • One I linked to - SafeMysql. In my opinion, it has no learning curve at all - you just write usual SQL, substituting dynamical parts with placeholders and get results in one function call. – Your Common Sense Mar 12 '13 at 13:23
0
$fieldDetails .= "`$key` =:$key, ";

You need to put the key name as placeholder here, not the value.

deceze
  • 510,633
  • 85
  • 743
  • 889
  • what about the last `,` ? – MatRt Mar 12 '13 at 12:50
  • What about it? It's trimmed afterwards. Actually, an `implode` call would be better overall, but that's irrelevant to the problem. – deceze Mar 12 '13 at 12:51
  • This code is prone to SQL injection. – Your Common Sense Mar 12 '13 at 13:01
  • 1
    @You If the keys are arbitrary user supplied values, yes. If they're whitelisted, no. – deceze Mar 12 '13 at 13:02
  • There is no word `'if'` in terms of injection protection. Either your code vulnerable or not. I'd be happy to remove a downvote if you bother to format the field name properly – Your Common Sense Mar 12 '13 at 13:38
  • No because I want the $value to be the data that goes in to the database. – egilviking Mar 12 '13 at 13:42
  • @egilviking But you need the placeholder to be the name of the key, because you're later binding the value using the name of the key! – deceze Mar 12 '13 at 13:47
  • @egilviking in this part deceze is right. you need your SET to look like `name=:name, age=:age` and so on. Keys, not values. – Your Common Sense Mar 12 '13 at 13:48
  • @You Fair enough. I typically never write code like this with dynamic field names, my field names are always present in the query I write. I'd suggest that over such overly dynamic queries to begin with. – deceze Mar 12 '13 at 13:48