1

I am trying to check if the email or password is taken. When I type in a taken username, it says username taken, if I type in a taken email, it says email taken but if I type a taken Email AND Username, it says "Good" instead of "Username and email taken." Does anyone know why it isn't working?

    $userSql = "SELECT * FROM members WHERE username='$username'";
    $emailSql = "SELECT * FROM members WHERE email='$email'";
    $result = mysql_query($userSql);
    $result2 = mysql_query($emailSql);

    $count = mysql_num_rows($result);
    $count2 = mysql_num_rows($result2);

    if (!empty($first_name) && !empty($last_name) && !empty($email) && !empty($username) && !empty($password)) {
    if ($count != 1) {
        echo "<p style=\"color: red\">Email taken, try another. You may already have an account</p>";
    }
    else if ($count2 != 1) {
        echo "<p style=\"color: red\">Username taken, try another. You may already have an account</p>";
    }
    else if ($count != 1 && $count2 != 1) {
        echo "<p style=\"color: red\">Username and email taken, try another. You may already have an account</p>";
    }
    else {
        echo "<p>Good</p>";
    }

It's really frustation because I have no idea why it wouldn't work.

  • 2
    Instead of `if ($count != 1)` do `if ($count > 0)` to check if it exists, then use an `else`. Always check if it first exists instead of checking if it doesn't exist. – Funk Forty Niner Apr 30 '14 at 21:00
  • Take `var_dump` and check every variable in your script – zerkms Apr 30 '14 at 21:01
  • A single query will do if you use `OR`, also your using and old API use PDO or mysqli with prepared query's. – Lawrence Cherone Apr 30 '14 at 21:02
  • @Loz Cherone: `OR` may perform worse (and with `OR` you likely will not be able to know if it's a name or email duplicate). `UNION ALL` would be better. – zerkms Apr 30 '14 at 21:05
  • Since the answer is already given, I can only recommend using prepared statements instead of plain queries, since your code is vulnerable to SQL Injection attacks. – MrByte Apr 30 '14 at 21:10
  • @DemVoids I already used stripslashes() and mysql_real_escape_string() for my variables, is there anything else I need to add? – user3504199 Apr 30 '14 at 22:22
  • @user3504199 It's not very useful, when you use prepared statements, you won't need to replace any characters, while stripslashes and mysql_real_escape_string do. Also, prepared statements are much safer. You can Google it and find all the info you need. – MrByte May 01 '14 at 08:24

1 Answers1

5

What you should do is set a constraint in your database for unique usernames and e-mail addresses. Then, try to do an insert and catch the exception when it fails. Otherwise, you could have a condition where nearly simultaneous users try to register at the same time, and between your SELECT and INSERT statements, the username or e-mail address might be used by someone else.

ALTER TABLE `members` ADD UNIQUE INDEX `email` (`email`);

You also have a real serious problem with SQL injection. Never concatenate data directly into a query, or you risk having the data getting confused with the command. The data must be escaped. The proper way to handle this is with prepared/parameterized queries which are available with PDO.

Brad
  • 159,648
  • 54
  • 349
  • 530
  • What if the given script is a pre-check that is done while user is still filling the form? Like an ajax request that reports "sorry, this name is already taken" – zerkms Apr 30 '14 at 21:02
  • @zerkms Then you do a SELECT. – Brad Apr 30 '14 at 21:03
  • that's right. My point was that "Then, try to do an insert and catch the exception when it fails" --- would satisfy only a specific case. At the moment your answer looks like: "you're doing it wrong - setup UQ constraint and use INSERT instead" – zerkms Apr 30 '14 at 21:04
  • 2
    @zerkms You're correct, and that is a fair statement. I'm taking a guess as to what user3504199 is trying to do. – Brad Apr 30 '14 at 21:05
  • Could you give an example on how to add this to my existing code? I'm relatively new to MySQL, and I'm not so sure how to put that code to use. – user3504199 Apr 30 '14 at 22:39
  • @user3504199 Adding the constraint isn't something you do in your code... you will use a MySQL client to do that. Once the constraint is added, follow the PDO tutorial I linked you to and be sure to do the part where you enable exceptions for PDO errors. Then, wrap your PDO query for inserting a new user in a try/catch block so you can catch the exception if there is a duplicate. – Brad Apr 30 '14 at 22:43