Please Review my code and highlight my mistake

[Ehsan_Nawaz]

Hi Dear Team 

Fatal error: Uncaught PDOException: SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens in C:\xampp\htdocs\demo\register.php:43 Stack trace: #0 C:\xampp\htdocs\demo\register.php(43): PDOStatement->execute() #1 {main} thrown in C:\xampp\htdocs\demo\register.php on line 43

$sql = "SELECT * FROM register WHERE username:username AND email:email";
    $stmt = $cn->prepare($sql);
    $stmt->bindValue(":username",$username);
    $stmt->bindValue(":email",$email);
    $stmt->execute();
    $user = $stmt->fetch(PDO::FETCH_ASSOC);
    if($user){
        if($user["username"]==$username){
            echo "Username Already exits";
        }elseif($user["email"]==$email){
            echo "Email Adress Already in USe";
        }
    }else {
        $sql ="INSERT INTO register(name,username,email,password,ip_adress)VALUES(:name,:username,:email,:password,ip_adress)";
        $stmt = $cn->prepare($sql);
        $stmt->bindParam(':name',$name);
        $stmt->bindParam(':username',$username);
        $stmt->bindParam(':password',$password);
        $stmt->bindParam(':ip_adress',$ip_address);
        if($stmt->execute()){
            echo "Register Success";
        }
    }

 

Edited February 28, 2023 by Ehsan_Nawaz

[Strider64]

first of all you should use an unique index for email and I don't understand the also having for username (though that too). Though I now can see both...tired.

 

Second take a look at this

$sql = "SELECT * FROM register WHERE username:username AND email:email";

See anything missing? I give you a hint it's between username :username and also email :email.

 

Here's a good link https://phpdelusions.net/pdo and I even still use it from time to time.

Edited February 28, 2023 by Strider64

[mac_gyver]

the error is due to a counting problem. you have 5 place-holders in the sql query statement, but are only supplying 4 values.

as to the posted code -

1. don't attempt to SELECT data first to determine if it exists. there's a race condition between multiple concurrent instances of your script, where they will all find from the existing SELECT query that the data doesn't exist, then try to insert duplicate values. instead, define the username and email columns as unique indexes, just attempt to insert the data, and detect if a duplicate index error (number) occurred. if the data was successful inserted, the values didn't exist. if you did get a duplicate index error, and since there is more than one unique index defined, it is at this point where you would query to find which column(s) contain duplicate values.
2. the SELECT query, in addition to needing = (comparison operators) needs an OR between the WHERE terms, to find any matching rows with either value, not just one row with both values.
3. if you use implicit binding, by supplying an array of values to the ->execute([...]) call, you can eliminate all the bindValue and bindParam calls, simplifying the code.
4. if you set the default fetch mode to assoc when you make the database connection, you  won't need to specify it in each fetch statement, simplifying the code.
5. you should use exceptions for database statement errors and only catch and handle a database exception in your code when inserting/updating duplicate user supplied values (which is what you are trying to do.) the PDO extension always uses exceptions for errors for the connection, and in php8+ the default is to use exceptions for all the other database statements that can fail. you would have exception try/catch logic for the insert query, then test if the error number is for a unique index error, then query to find which/both of the two unique columns already contain the submitted values. for all other error numbers, just rethrow the exception and let php handle it.

edit: you should also be using php's password_hash() and password_verify() to hash and test the password value.

Edited February 28, 2023 by mac_gyver

[Psycho]

8 hours ago, mac_gyver said:

1. don't attempt to SELECT data first to determine if it exists. there's a race condition between multiple concurrent instances of your script, where they will all find from the existing SELECT query that the data doesn't exist, then try to insert duplicate values. instead, define the username and email columns as unique indexes, just attempt to insert the data, and detect if a duplicate index error (number) occurred. if the data was successful inserted, the values didn't exist. if you did get a duplicate index error, and since there is more than one unique index defined, it is at this point where you would query to find which column(s) contain duplicate values.

@mac_gyver is correct about the proper way to prevent duplicates. However, when it comes to account creation you should never inform the user that there are existing values for user IDs or email addresses. Malicious users use such "errors" to data-mine a site to find out what are valid usernames. They can then do all sorts of nefarious activities from trying to brute force passwords, sending users fishing emails purporting to be from the company of the site, etc.

Best practice is that when a user registers for a site (assuming the data they entered was valid) is to provide a message to them stating something along the lines of "Thank you for registering, an email has been sent to you with instructions to complete your registration". If the username & email are not duplicates, then send an email with a link to complete the registration. If the email is an existing one (whether or not the username is the same), then send an email with a statement that a registration was attempted with the email address, but one already exists (and probably include a link with instruction on how to reset their password as they may have forgotten they registered previously). If it was a malicious attempt the user will know that someone else tried to register with their email address. What I remember was based on the email address being the User ID. So, I'm not certain what you would want to do if only the User ID is a duplicate. But, you don't want to allow a malicious user to harvest your application for valid User IDs.