Checking if username is already taken

It’s still letting me register the same username over and over again, what Am i doing wrong here? thanks!

<?php
	function register(){
		if(isset($_POST['username'], $_POST['password'])){
			require 'core/connect.php';

			$query = dbConnect()->prepare("INSERT INTO users (username, password) VALUES (:username, :password)");

			$query->bindValue(':username', $_POST['username']);
			$query->bindValue(':password', $_POST['password']);

			if($_POST['username'] == 1){
				echo 'Username already taken.';
			}
			if(empty($_POST['username']) || empty($_POST['password'])){
				echo 'Please fill out all fields.';
			} else {
			if($query->execute()){
				header("Location: home.php");
			}
			}
		}
	}
?>

How is the table defined? Do you have username defined as unique in the table?

id int(11) NOT NULL AUTO_INCREMENT,
username varchar(32) NOT NULL,
password varchar(32) NOT NULL,
PRIMARY KEY (id)


ALTER IGNORE TABLE mytbl ADD UNIQUE (columnName);

:slight_smile:

What is the id field for? If the username is supposed to be unique then that should be the primary key. The id field is only needed to allow multiple entries with the same username.

`username` varchar(32) NOT NULL,
 `password` varchar(32) NOT NULL,
 PRIMARY KEY (`username`)

Also why is the password field a varchar rather than a char field - whether the password is one character long or a million characters long the hash you convert it to before storing it in the database will always be the same length.

I’m not sure

            if($_POST['username'] == 1){
                echo 'Username already taken.';
            } 

is what you want. Shouldn’t that be something like numrows returned from the query?

If you need an ‘id’ for use in JOIN queries etc. you could still use it, but felgall is correct, it shouldn’t be the primary key in this table.

So something like this?

$stmt = $conn-&gt;prepare("SELECT COUNT(*) AS count FROM `user` WHERE username=?");
$stmt-&gt;execute(array($username));
while ($row = $stmt-&gt;fetch(PDO::FETCH_ASSOC)) {
  $username_count = $row["count"];
}
if ($username_count &gt; 0) {
  echo "That username is already taken";
}

Seriously, just make the username column unique in the database and save yourself the hassle. If you use phpMyAdmin it should just be a checkbox (I think).

Well no. Lot’s of times you need to link other tables to the user table. You might also want to give users the ability to change their username. Keep the autoinc id.

This is a good practice but it’s not a real solution. Adding a duplicated key will result into an error. I know, you can use ON DUPLICATE KEY UPDATE/IGNORE but, again, this is not a good practice. You must check for existing records first, before the INSERT.

So, as @Mittineague said, you have the wrong IF.
You must check if you have any user with that username
You just need results from

SELECT `iduser` FROM `users` WHERE `username` = :username

And, check if $record[‘iduser’] is empty before the INSERT. If it’s not empty return some error.

If the column username is a varchar and unique this should be an instant query.

You solution is not 100% complete in a web application environment. Consider the case where two users with the same username push the create user button at pretty much the same time. It’s possible that the user 1 request might check for an existing user per your code, find there is none and then attempt to create one. Meanwhile, the second request has slipped in, also checked for an existing user and is then able to create a new user in between the time that the first request checks the username and inserts a new one.

You could try locking tables but that has it’s own set of problems. The only reasonable approach is to create a unique index and then check for errors(or catch the exception) after the insert.

Prechecking the username is still a good idea but it’s not a replacement for dealing with unique constraints at the database level and dealing with database errors.

I know what you’re saying and I didn’t deny the unique key idea.
It’s just not an elegant solution, to add an error into your MySql logs for each failed username.
This is not a professional way of doing this (my opinion).

The unique key exception should trigger only in case a user will send the request exactly in the small gap between MySQL select check and making the good and valid insert (where MySql locks the table for writing). This gap should take something like 0.001 seconds or less. And I’m not saying it’s impossible to hit a request into this gap but, like I said, it’s a good practice to check first.

As example, all big companies will make an Ajax request that checks the username before you hit the submit.

I guess the question is if @Godz06; is “stuck” with the database architecture?

If not, I think it could be better designed, if yes, then will need do the best as can be done with what’s there.

It is NOT good practice to check if a value already exists before inserting it.

Best practice is to try to insert it and to capture and do appropriate processing of the error if it fails (which might be to ignore the error).

Apart from the potential timing problems when two people try to insert simultaneously, it is also far more efficient to do one database call than two.

With username being only 32 characters long it is perfectly appropriate in 99% of cases to use that as a key in other tables associated with the user as well so the only reason for including an id would be if you want people to be able to change their username on a regular basis. If users will only rarely change their username then you can use the username as the primary key and handle the change as a rare exception condition - the way almost all forum software does.

If you definitely want a numeric primary key then make the username a number - I have seen a number of sites that allocate the next available number as the username as each person signs up.

You could use transactions. The first query would check to see if the chosen name exists, the second query would insert a new row if the username was not found by the first query

I agree, but this should be an exception, because you have to show some message to the user. You are talking about avoiding duplicates, as a general matter but this is not just it. You will need to add the username AND the email as unique key. Maybe there is another unique key to consider (let’s say personal code or other stuff). How will you do it? What will you tell to the user?

edit:

If users will only rarely change their username then you can use the username as the primary key
I disagree with this. First of all, varchar will be way slower (size matters). Second, the username should be changeable, by business logic. I wouldn’t recommend to “truncate” this option with the table design because the “client” can change its mind very easy. Just a personal opinion.

In fact, all my comments are “what I would do” but, every programmer (db designer) has own choices and preferences :slight_smile: it does not mean my way is the best

totally agree

normally a natural key, when one exists, is better than a surrogate key, but not if there are many other tables linking to it – which is exactly what happens with a user table

so yeah, keep the id

well, you know what they say, everyone’s entitled to an opinion, even if it’s wrong :slight_smile:

bah

you’ve heard of solving the wrong problem? this is applying the wrong solution

wha?

no, the username being defined as unique in the database is really all that’s required

trapping the “duplicate key” database error message and giving the user a friendly message is NOT THAT HARD, guys

:slight_smile:

You should be setting unique on both your username and email fields anyway, that’s just good database design. To handle your registration I’d consider something like this.


$isError = false;
$errorMessage = '';

$query = $conn-&gt;prepare("SELECT COUNT(*) AS count FROM `user` WHERE username=:username");
$query-&gt;bindValue(':username', $_POST['username']);
$query-&gt;execute();

while ($row = $query-&gt;fetch(PDO::FETCH_ASSOC)) {
  $user_count = $row["count"];
}
if ($user_count &gt; 0) {
  $isError = true;
  $errorMessage = $username.' already taken, please choose another.';
}

$query = $conn-&gt;prepare("SELECT COUNT(*) AS count FROM `user` WHERE email=:email");
$query-&gt;bindValue(':email', $_POST['username']);
$query-&gt;execute();

while ($row = $query-&gt;fetch(PDO::FETCH_ASSOC)) {
  $email_count = $row["count"];
}
if ($email_count &gt; 0) {
  $isError = true;
  $errorMessage = $email.' already registered, please login.';
}

if(empty($_POST['username']) || empty($_POST['password']) || empty($_POST['email'])){
  $isError = true;
  $errorMessage = 'Please fill in all fields.';
}

if(isError) {
  echo $errorMessage;
} else {
  $query = dbConnect()-&gt;prepare("INSERT INTO users (username, email, password) VALUES (:username, :email, :password)");

  $query-&gt;bindValue(':username', $_POST['username']);
  $query-&gt;bindValue(':email', $_POST['email']);
  $query-&gt;bindValue(':password', $_POST['password']); // hash this!

  $query-&gt;execute();
  echo 'Registration successful.';
}

Could be much cleaner but am typing this on my iPad without an editor so will have to do for now. :slight_smile:

i don’t wanna hammer the point, but using a SELECT before deciding to do the INSERT opens you up to race conditions

really, the only sane solution is to let the database control uniqueness… and in that case, doing the SELECT before the INSERT is silly

I appreciate this, however if you have two unique fields and try to insert, how can you capture which constraint failed to inform the user? I’m not overly familiar with MySQL’s error notification but assume this would throw a 1062 error. Reading the documentation this will provide error message ‘Duplicate entry ‘%s’ for key %d’. This should be fairly easy to capture and handle accordingly.

Godz06, take a look at PDOStatement::errorInfo in the PHP manual.