0

The code below indicates my attempts to try and find out whether a row exists with the criteria gave in the code. It defaults to the else statement, correctly, but doesn't work with the 'if' statement if the if statement appears to be true (there are no emails down as ashfjks@sdhja.com), and instead the code proceeds. The latter part of this code is mostly to expand on the situation. the row can only exist or not exist so I don't understand why it's not strictly doing one or the other. I am converting into PDO for site secuirty, thats why not all is in PDO, yet. I am sorry if this question is too localised?

$stmt = $pdo->prepare("SELECT * FROM table WHERE email = ?");
$stmt->execute(array("$email"));
$row3 = $stmt->fetch(PDO::FETCH_ASSOC);

while($row = $stmt->fetch()) {

  if ( ! $row3) {
    // Row3 doesn't exist -- this means no one in the database has this email, allow the person to join
    $query = "INSERT INTO table (username, email, password, join_date) VALUES ('$username', '$email', SHA('$password1'), NOW())";
    mysqli_query($dbc, $query); 
    $query = "SELECT * FROM table WHERE username = '$username'";
    $data2 = mysqli_query($dbc, $query);
  while ($row = mysqli_fetch_array($data2)) {

  $recipent = '' . $row['user_id'] . '';

    $query = "INSERT INTO messages (recipent, MsgTit, MsgR, MsgA, sender, time, readb, reada, MsgCon) VALUES ('$recipent', '$MsgTit', '$MsgR', '$MsgA', '$sender', NOW(), '$readb', '$reada', '$MsgCon')";
    mysqli_query($dbc, $query);

    // Aftermath.
    echo '<p>Your new account has been successfully created. You\'re now ready to <a href="game2.php" target="_blank">log in</a>. After this you should implement basic character-details on your users profile to begin the game.</p>';

    mysqli_close($dbc);
    exit();
  } }  
  else {
    // An account already exists for this email, so display an error message
    echo '<p class="error">An account already exists for this e-mail.</p>';
    $email = "";
  }
}
Sam Bowyer
  • 60
  • 7
  • 1
    errr, the `while` loop already exits if `$stmt->fetch()` returns `false`, so you will never enter the `if` statement if the row doesn't exist .., use before the `while` loop something like `if($row->rowCount() > 0)` – dbf Oct 31 '12 at 21:45
  • Is there any special reason you're mixing PDO and MySQLi? – Madara's Ghost Oct 31 '12 at 22:05
  • He said he is working on converting it to PDO query-by-query. I assume he doesn't want to break it all at once. He appears to have another major problem here, so the mixing might not be a big issue. – Geoff Montee Oct 31 '12 at 22:09
  • Madara Uchiha: I'm new to PDO and learnt mysqli (lightly) after learning a bit of mysql. Friends and reviews online have told me that learning mysqli was a bit of a waste of time as PDO seems to be much better (and the future .. ), so I'm trying to use PDO instead. – Sam Bowyer Oct 31 '12 at 22:10

2 Answers2

2

+1 to answer from @Geoff_Montee, but here are a few more tips:

  • Make sure you check for errors after every prepare() or execute(). Report the error (but don't expose your SQL to the user), and fail gracefully.

  • Note that even though you checked for existence of a row matching $email, such a row could be created in the brief moment of time since your check and before you INSERT. This is a race condition. Even if you SELECT for a row matching $email, you should also use a UNIQUE constraint in the database, and catch errors when you execute the INSERT in case the UNIQUE constraint blocks the insert due to conflict.

  • SELECT email instead of SELECT *. If you have an index on email, then the query runs more efficiently because it can just check the index for the given value, instead of having to read all the columns of the table when you don't need them. This optimization is called an index-only query.

  • Likewise use SELECT user_id instead of SELECT *. Use SELECT * only when you really need to fetch all the columns.

  • Bcrypt is more secure than SHA for hashing passwords.

Community
  • 1
  • 1
Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
  • To your points: with SELECT'ing via column names rather then *, I agree and don't usually do this, I don't really know how the * ended up there, as the mysql(i) version didn't have it, I can only assume trying to add in COUNT(*) and removing it lofted me up with it. In regards to the SHA, yeah, I'm looking at using salts, keys, etc at this question [here](http://stackoverflow.com/questions/4795385/how-do-you-use-bcrypt-for-hashing-passwords-in-php). By check for errors, do you mean when rows are meant to be returned executing an 'close if rows returned = 0' kind of error stopper? – Sam Bowyer Oct 31 '12 at 22:36
  • 1
    @SamBowyer he means that you had no error checking in your original code. So, if the query failed (syntax error, server died, hurricane blew away data center), the code would continue chugging. Check out the error checking I added to my code. Lots of `if (!$stmt->prepare(...))`. – Geoff Montee Oct 31 '12 at 22:41
  • Also, in regards to Bill's points about race conditions... a good solution would be to use transactions and pick the proper transaction isolation level: http://dev.mysql.com/doc/refman/5.5/en/dynindex-isolevel.html – Geoff Montee Oct 31 '12 at 22:43
  • @Geoff_Montee, transaction isolation doesn't help in this case. MySQL doesn't lock on SELECT. Besides, if the SELECT finds no such row, there's nothing to lock to prevent another thread from inserting a new row with that value. You just have to catch the error if the unique constraint is violated. – Bill Karwin Oct 31 '12 at 22:54
  • @BillKarwin interesting. I thought the `SERIALIZABLE` transaction level would keep transactions from messing with each other. – Geoff Montee Oct 31 '12 at 22:55
  • @Geoff_Montee, the docs say SERIALIZABLE creates a shared lock on rows matching the SELECT, but if no rows match, no lock will be created. So there's nothing to prevent the subsequent INSERT by another thread. – Bill Karwin Oct 31 '12 at 23:09
  • @BillKarwin Do you know if `SERIALIZABLE` implies `REPEATABLE-READ`? So even if a new row is inserted by transaction B, transaction A won't see the new row if it's already queried the table. (Regardless of whether transaction B commits before transaction A completes.) This is only tangentially related. Your comment just got me wondering. – Geoff Montee Nov 01 '12 at 00:29
  • @Geoff_Montee, locking is arcane and confusing in MySQL. For example, see http://stackoverflow.com/questions/6269471/ – Bill Karwin Nov 01 '12 at 01:41
2

Your if statement will never be executed. You need to check the number of rows returned. This is what you want:

Note: I originally used $stmt->rowCount(), but the OP said that didn't work for him. But I'm pretty sure the cause of that error was coming from somewhere else.

if (!($stmt = $pdo->prepare("SELECT * FROM table WHERE email = ?"))) {
   //error
}

if (!$stmt->execute(array("$email"))) {
    //error
}
//The $row3 var you had was useless. Deleted that.

$count = 0;

while ($row = $stmt->fetch()) {
    $count++;
}

//The query returned 0 rows, so you know the email doesn't exist in the DB
if ($count== 0) {

    $query = "INSERT INTO table (username, email, password, join_date) VALUES ('$username', '$email', SHA('$password1'), NOW())";

    if (!mysqli_query($dbc, $query)) {
        //error
    }

    $query = "SELECT * FROM table WHERE username = '$username'";

    if (!($data2 = mysqli_query($dbc, $query))) {
        //error
    }

    while ($row = mysqli_fetch_array($data2)) {

        $recipent = '' . $row['user_id'] . '';

        $query = "INSERT INTO messages (recipent, MsgTit, MsgR, MsgA, sender, time, readb, reada, MsgCon) VALUES ('$recipent', '$MsgTit', '$MsgR', '$MsgA', '$sender', NOW(), '$readb', '$reada', '$MsgCon')";

        if (!mysqli_query($dbc, $query)) {
            //error
        }

       // Aftermath.
       echo '<p>Your new account has been successfully created. You\'re now ready to <a href="game2.php" target="_blank">log in</a>. After this you should implement basic character-details on your users profile to begin the game.</p>';

       mysqli_close($dbc);
       exit();
   }
}
//The query did not return 0 rows, so it does exist in the DB
else {
    // An account already exists for this email, so display an error message
    echo '<p class="error">An account already exists for this e-mail.</p>';
    $email = "";
}

And you should totally convert the rest of those queries to use PDO.

Geoff Montee
  • 2,587
  • 13
  • 14
  • I tried using row_count many times (along with COUNT(*)), it just doesn't work for me. the error is " Call to a member function rowCount() on a non-object". I read up that rowCount doesn't always work on SELECT queries. Thank you very much for answering though, would you know why it's identifying itself as a non-object? I thought arrays were objects. – Sam Bowyer Oct 31 '12 at 22:09
  • 1
    @SamBowyer The "call on a non-object" typically means that there is another problem somewhere. However, I see your point. Give me one moment. – Geoff Montee Oct 31 '12 at 22:11
  • @ Geoff_Montee - It was on my fault. The re-arranged code was missing a }. I'm not sure why that error resulted but the code works now. Thanks very much - as well as the other answers, this has provided insight into how PDO handles differently to mysqli (and picking up on PDO only a few days ago means I really hadn't learnt this). Thanks! – Sam Bowyer Oct 31 '12 at 22:16
  • 1
    @SamBowyer great! Glad to help. You are heading in the right direction by switching to PDO. – Geoff Montee Oct 31 '12 at 22:17
  • What about using `$rows = $stmt->fetchAll(PDO::FETCH_ASSOC); if (count($rows) == 0) ...` That should work as well. – mkey Jul 07 '13 at 11:00