1

I'm pretty new to php coding and managed to resolve a lot of problems myself, but there is 1 I can't get my head around.

$prep_stmt = "SELECT id FROM members WHERE Email = ? LIMIT 1";
$stmt = $mysqli->prepare($prep_stmt);

// check existing Email
if ($stmt) {
    $stmt->bind_param('s', $Email);
    $stmt->execute();
    $stmt->store_result();

    if ($stmt->num_rows == 1) {
        // A user with this Email address already exists
        $error_msg .= '<p class="error">A user with this Email address already exists.</p>';
        $stmt->close();
    }
            $stmt->close();
} else {
    $error_msg .= '<p class="error">Database error Line 39</p>';
            $stmt->close();
}

My guess is that the code can't get to the 2nd $stmt->close(); in the above code (the one after the if inside the if).

How can I resolve this problem? Is that $stmt->close(); really needed?

Arnab Nandy
  • 6,472
  • 5
  • 44
  • 50
Krowi
  • 1,565
  • 3
  • 20
  • 40
  • Well you could wrap that one in an `else {}` block, but honestly, none of the explicit calls to `$stmt->close()` are likely to be necessary. PHP will clean them up itself. – Michael Berkowski Sep 13 '14 at 13:10
  • @MichaelBerkowski, so it's safe to remove all of them? (Post an answer please so I can give some rep :) ) – Krowi Sep 13 '14 at 13:11
  • Probably. If there are rows to be fetched, [calling `$stmt->close()`](http://php.net/manual/en/mysqli-stmt.close.php) frees the RDBMS to execute a new query, but if you aren't going to issue a new query in this script that isn't a problem because PHP will clean them up on script termination. Plus, if I recall correctly, calling `store_result()` sidesteps that problem anyway. – Michael Berkowski Sep 13 '14 at 13:14
  • In particular, the last `$stmt->close()` in the DB error block will never successfully execute because $stmt won't be an object. Definitely remove that one because it will cause a fatal error. – Michael Berkowski Sep 13 '14 at 13:16

3 Answers3

4

Why you don't just remove the first one (in the second if statement)? Also remove the close() in your else statement because you checked if $stmt is a legal object. Basically what you say is: $stmt isn't a legal object, close it. But close what?

This will work in both situations:

$prep_stmt = "SELECT id FROM members WHERE Email = ? LIMIT 1";
$stmt = $mysqli->prepare($prep_stmt);

// check existing Email
if ($stmt) {
    $stmt->bind_param('s', $Email);
    $stmt->execute();
    $stmt->store_result();

    if ($stmt->num_rows == 1) {
        // A user with this Email address already exists
        $error_msg .= '<p class="error">A user with this Email address already exists.</p>';
        //Remove this one: $stmt->close();
    }
    $stmt->close();
} else {
    $error_msg .= '<p class="error">Database error Line 39</p>';
    //This one can be removed because $stmt isn't a legal object: $stmt->close();
}
S.Pols
  • 3,414
  • 2
  • 21
  • 42
0

Just remove the if statements and the close() method calls. Why have you added that to your code?

Th reason why you are getting this error is because you try to close a statement when its creation failed. You can't close an object which was never created. If prepare() call fails and you have your error reporting silenced this function will return false instead of an object.

If you enable proper error reporting there should be no if statements. See How to get the error message in MySQLi?

Here is how your code should look like:

$prep_stmt = "SELECT id FROM members WHERE Email = ? LIMIT 1";
$stmt = $mysqli->prepare($prep_stmt);

// check existing Email
$stmt->bind_param('s', $Email);
$stmt->execute();
$result = $stmt->get_result();

if ($result->fetch_assoc()) {
    // A user with this Email address already exists
    $error_msg .= '<p class="error">A user with this Email address already exists.</p>';
}

Try to avoid store_result() as much as possible. When you enable error reporting then there's no reason for any if statements. Otherwise, you would need to wrap each line in such statement, because any of these calls could fail: prepare(), bind_param(), execute(), ...

There's no need to close the statement if you fetch all the results. When you call get_result() you fetch everything from MySQL.

Keep it simple and don't add unnecessary code.

Dharman
  • 30,962
  • 25
  • 85
  • 135
-1

You should put store_result() after execute like bellow:

$stmt->execute();
$stmt->store_result();

if you make a method and pass query, it doesn't work

h3t1
  • 1,126
  • 2
  • 18
  • 29
Mahdi Karimian
  • 127
  • 1
  • 3