更新记录失败:(1064)

I have a list of all the users in my table. And to the right of them, I have an update button. When I press the update button, I'm sent to the update page. And I have the users id with me, so the link would like something like this: /update.php?id=22

On the update page, I have my update form:

        <table>
            <tr>
                <th>First Name:</th>
                <td><input type="text" value="<?php $user->get_first_name($id);?>" name="first_name"></td>
            </tr>
            <tr>
                <th>Last Name:</th>
                <td><input type="text" value="<?php $user->get_last_name($id);?>" name="last_name"></td>
            </tr>
            <tr>
                <th>Email:</th>
                <td><input type="text" value="<?php $user->get_email($id);?>" name="email"></td>
            </tr>
            <tr>
                <th>Password:</th>
                <td><input type="text" value="<?php $user->get_password($id);?>" name="password"></td>
            </tr>
            <tr>
                <td>&nbsp;</td>
                <td><input type="submit" name="submit" value="Update" ></td>
            </tr>
            <tr>
                <td>&nbsp;</td>
                <td><a href="login.php">Already registered! Click Here!</a></td>
            </tr>                 
        </table>

The way I get the information to the value, is like this:

public function get_first_name($id){
            $sql3="SELECT first_name FROM users WHERE id = $id";
            $result = mysqli_query($this->db->mysqli,$sql3);
            $user_data = mysqli_fetch_array($result);
            echo $user_data['first_name'];
}

And so on, for each field.

I then call my function like this:

if (isset($_POST['submit'])){

        if (empty ( $_POST ['first_name'] ) || empty ( $_POST ['last_name'] )|| empty ( $_POST ['email'] ) || empty ( $_POST ['password'] )) {

            $errors [] = 'All fields are required.';
        } 
        if (empty ( $errors ) === true) {   
            extract($_POST);
            $register = $user->update_user($id, $first_name, $last_name,$password, $email);
        }
    }

And my function looks like this:

public function update_user($id, $first_name,$last_name,$password,$email){

            $sql = "UPDATE users SET first_name = $first_name, last_name = $last_name, email = $email, password = $password  WHERE id = ?";
            $result = $this->db->mysqli->query($sql);
            if (!$result) {

                echo "Update record failed: (" . $this->db->mysqli->errno . ") " . $this->db->mysqli->error;

            } else {

                // Print the table
                header("Location: ../admin.php");

            }
        }

But when I try to update a user, I'm getting an error:

Update record failed: (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 '@gmail.com, password = ASDASDASD WHERE id = ?' at line 1

There must be something wrong with my sql. I hope one of you can spot my error(s)

Use quotes around strings in queries, escape the string values and hash the password:

$sql = "UPDATE users 
    SET
        first_name = '".mysqli_real_escape_string($this->db->mysqli, $first_name)."', 
        last_name = '".mysqli_real_escape_string($this->db->mysqli, $last_name)."', 
        email = '".mysqli_real_escape_string($this->db->mysqli, $email)."', 
        password = '".mysqli_real_escape_string($this->db->mysqli, crypt($password, $salt))."'
    WHERE id = " . $id;

Use an appropriate salt for the crypt function.

Two suggestions:

  • Never ever store a password in plain text! Please hash it properly before storing it. If your database is accessed in some illegal way (hacking) those plain text passwords are an invitation to the attacker. You may not have sensitive data in your database, but people tend to use the same password over and over again, so it may be used on another sites.
  • Never ever show the current password to the user! The value attribute of the password field in your form should always be empty.

The actual problem / error is caused by the fact that you are not quoting your string values.

However, if you would do it like that, you would need to use an escape function to make sure you can insert your values safely (avoid sql injection).

You would be better off to use a prepared statement like:

UPDATE users SET first_name = ?, last_name = ?, email = ?, password = ?  WHERE id = ?

(you don't need quotes there)

And bind the values and execute the statement after that.

You already have the id setup like that (although you don't bind it either, leading to another error later on), so you already have an idea which way to go.

Apart from that:

  • Don't ever store plain-text (or encrypted...) passwords. Use a salted hash; you can find a lot more about that here on SO.
  • You should get your user information in one go, not with separate database queries.

You should change in your update query email = $ password to email = $email