CODEWITHSUNDEEP
phpmysql
JohnyFonno
JohnyFonno

Reputation: 31

How come this php if statement does not work?

I wrote some php code to take care of adding a new user to my database. If a user already exists, it will not add a new user. To check if there is already a user, I am checking if the userID is already in the User table which would mean the user already exists. I attached it to $count variable. If $count is 0 that means that the user does not already exist so I want to create the user. It seems like even if the user exists my code is always executing the query to add new user, even if the user is already in the database. Can anyone tell me where I am going wrong in my code? I am very new to php. Here is my code:

<?php

$con=mysqli_connect("editout","editout","editout","editout");

if (mysqli_connect_errno())
{
  echo "Failed to connect to MySQL: " . mysqli_connect_error();
}

$userID = $_GET["userID"];

$count = mysqli_query($con,"SELECT COUNT(*) FROM User WHERE userID=$userID");
if($count == 0) 
{
    mysqli_query($con,"INSERT INTO User VALUES ('$userID', 0, 0, 0, 0)");
}

mysqli_close($con);

?>

Upvotes: 0

Views: 64

Answers (4)

Djordje
Djordje

Reputation: 186

Correct your code for SQLi as others have noted - also an alternative way to achieve what you want may be using SELECT and then mysqli_affected_rows($con).

Upvotes: 0

Martijn
Martijn

Reputation: 16123

That is because the $count variable holds the query (known as a resource), not the amount of rows. In order to get the info from the result, you have to fetch the info:

$count = mysqli_query($con,"SELECT COUNT(id) as foundRows FROM User WHERE userID=$userID");
$fetch = $count->fetch_assoc();
echo $fetch['foundRows'];

I've taken the liberty of changing it to COUNT(id). Try and make it a habbit to select as little as possible. Please read the bottom about security.

You can also use num_rows:

$count = mysqli_query($con,"SELECT id FROM User WHERE userID=$userID");
echo $count->num_rows;

Because of this I suggest you rename your variable to $result or $users (or something more broad like $item). This way it's easier to understand what the code does:

echo $users->num_rows; // You can now understand what this does without context

You also might want to user LIMIT 1, highly likely you only need to check for 1 ID. LIMIT 1 tells the query to stop if 1 match is found (which, in this case, is enough).

Warning

The way you have the code, `$_GET['userId']` can be used directly to change your query. Never asume anything a user can change is safe (like GET and POST). You can perform a simple check with [`ctype_digit()`][2]: 
if( ctype_digit($_GET['userID']) ){ echo "It only consists of number"; }
else{ "nope, security problem"; }

You could also use, as Ohgodwhy mentioned, use prepared statements. While those are very safe, they're also a little more resource extensive. To keep your code fast, you shopuld always perform, the small checks first.

And a _important_ tip

You're adding all these zero's in your query. If a user (almost) always stars with those values, you can make them default value in your database. When data gets inserted, the column for which the valuesare not given will use their default value. Continuing this, the column `id` should be auto_increment. That means every row that gets inserted automaticly gets that value incremented (hence auto_increment). That would result in a query like: 
mysqli_query($con,"INSERT INTO User (id) VALUES (NULL)"); // null makes it go to default, AI

Now you don't need your count queries anymore!

Upvotes: 2

N.B.
N.B.

Reputation: 14091

On top of the answers that tell you why you get something you don't expect, I'll tell you why your approach to handling (possible) duplicate records isn't safe enough.

You shouldn't check for userID from PHP. Between PHP and MySQL server exists network response latency or lag if you will. Even if it's a small value (a millisecond or even half a millisecond) in question, that means you can get two users at the same time that are dealing with inserting the same userID.

The approach you have in place leaves this possibility of inserting duplicate records. The way we handle it is not checking from PHP. We place constraints on the database. In your case, you want to make userID a UNIQUE index. That way you are sure that sneaky people can't slip by the database constraint.

Doing that even simplifies your code. All you have to do is perform an insert. You don't even need to check whether the record is already in. If it is in, you'll receive an Exception - and that's easy to handle.

Combining what I wrote with already provided answers about prepared statements will yield a solid code that's going to be resilient and safe.

Upvotes: 2

Ohgodwhy
Ohgodwhy

Reputation: 50798

First, you are vulnerable to sql injection. please consider using mysqli prepared statements when your queries will accept user data. Please observe the following transition into a safer method:

$con = new mysqli('host', 'user', 'pass', 'database');

/* check connection */
if (mysqli_connect_errno()):
    printf("Connect failed: %s\n", mysqli_connect_error());
    exit();
endif;

$userid = $_GET['userID'];
$stmt = $con->prepare('select * FROM User WHERE userID=?');
$stmt->bind_param('i', $userID);
$stmt->execute();
if($stmt->num_rows == 0):
    $ins = $con->prepare('insert into User VALUES(?,0,0,0,0)');
    $ins->bind_param('i', $userID);
    $ins->execute();
endif;

Please observe that in the above code, we are binding the value of $userID to ? by leveraging prepared statements with bound parameters.

Also, please note that COUNT(*) seems to be wrong for your particular statement and has been omitted to achieve your desired result.

Resources

Upvotes: 4

Related Questions