Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Hey been at this for a day can't fix the problem. This function updates user info in the db [MySQL] it keeps throwing the exception Couldn't update user

<?php
function update_user($user_id, $update_data) {
    global $db;
    $update = array();//array_walk($update_data, 'array_sanitize');

    foreach ($update_data as $field=>$data) {
        if ($field != 'allow_email') {
        $update[] = '`' . $field . '` = \'' . $data . '\'';
        } else {
            $update[] = '`' . $field . '` = ' . $data;
        }
    }
    $updateImploded = implode(', ', $update);
    var_dump($updateImploded);
    $stmt = $db->prepare("UPDATE `users` SET :updateImploded WHERE `user_id` = :user_id");
    try {
        $success = $stmt->execute(array(':updateImploded'=>$updateImploded, ':user_id'=> $user_id));
        var_dump($stmt);
    } catch (PDOException $e) {
        die ('Couldn\'t update user');
    }
    //mysql_query("UPDATE `users` SET" . implode(', ', $update) . " WHERE `user_id` = '$user_id'");
}

EDIT 2 After revising it to use php to prepare the statement

    foreach ($update_data as $field=>$data) {
        $update[] = '`' . $field . '` = \'' . $data . '\'';
    }
    $updateImploded = 'UPDATE `users` SET' . implode(', ', $update) . 'WHERE `user_id` =' .$user_id;
    $stmt = $db->prepare($updateImploded);
    try {
        $success = $stmt->execute();
        var_dump($stmt);
    } catch (PDOException $e) {
        die ('Cant update user');
    }
}
share|improve this question
 
Add $e->getMessage() to your die message. –  Sam Selikoff May 10 '13 at 3:19
 
Im getting this error now >> SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''first_name = \'Saeeb\', last_name = \'Msarwa\', email = \'[email protected]' at line 1 –  Saeb Msarwa May 10 '13 at 3:43
1  
I'm afraid since the code does not it's off-topic on this site (according to the FAQ). –  palacsint May 10 '13 at 4:47
 
Maybe your fields need ' around them, not `. Also, using implode like this is not really in the spirit of named parameters. It may seem to make the implementation easier, but it's less maintainable and more subject to errors in the future. Try to get it working by hard-coding the field names in, and using a separate :var for each value. –  Sam Selikoff May 10 '13 at 5:33
 
Thanks sam, tried that didn't work, i var dumped to make sure it's ok, I'm trying to switch this login system from using MySQL to PDO, this is how I learned php through this small script. –  Saeb Msarwa May 10 '13 at 6:45
add comment

closed as off topic by Corbin, Brian Reichle, palacsint, Glenn Rogers, svick May 10 '13 at 10:45

Questions on Code Review Stack Exchange are expected to relate to code review request within the scope defined by the community. Consider editing the question or leaving comments for improvement if you believe the question can be reworded to fit within the scope. Read more about reopening questions here.If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer

up vote 1 down vote accepted

I guess the attribute name is missing before the :updateImploded parameter, that is:

UPDATE ... SET attr = :attr1 WHERE ...

because AFAIK the binded parameters can contain only values (not other SQL commands and attribute names). That's the point of avoiding SQL injections.

So the query should be something like this:

UPDATE `users` 
    SET att1 = :attr1, attr2 = :attr2, attr3 = :attr3 
    WHERE `user_id` = :user_id
share|improve this answer
 
I see, is there a way to tweak this code to fix it? –  Saeb Msarwa May 10 '13 at 4:52
 
@SaebMsarwa: Your code might generate the SQL query too (as well as the parameters array). –  palacsint May 10 '13 at 5:40
 
Thanks @palacsint, that's the only way and worked fine. –  Saeb Msarwa May 10 '13 at 6:37
 
Does this need escaping [EDIT 2] –  Saeb Msarwa May 10 '13 at 6:43
 
@SaebMsarwa: I think it needs (although it depends on where your data come from). –  palacsint May 10 '13 at 7:18
show 1 more comment

Not the answer you're looking for? Browse other questions tagged or ask your own question.