Improvements To Member Registration Site Reg.php


#1

Ladies & Gentleman,

Or should I say 'Gentle Ladies' and 'Hard Men' (tough guys)! :winky:

Here is my very latest (New Code) reg.php. I have modified it by:

  • Removing outdated strip tags, mysqli_escape_string.
  • Bound input parameters on the user reg form.
  • Added htmlspecialcharacters code on output to prevent sql injection.

Look how cluttered my old code was before a lot of programmers here and other sources helped me out (thanks to all!).

Ok, my new code does not have the email confirmation code and a lot of others but I will add them soon. I took them out here to make the new code simple for you to easily understand the code. Kept just the fundamentals on the 1st impression. Will add the remaining necessities on the 2nd impression.
You are welcome to make any suggestions and critisize the coding (but do bother to show an example of an improvement to the area you critisize). Ok ?

Old Code:

<?php

//DB connection details.	
$server_name = "localhost";
$user_name = "root";
$server_password = "";
$db_name = "e-id";


//Connect to DB.
$conn = new mysqli($server_name,$user_name,$server_password,$db_name);

if($conn->connect_error)
{
    die($conn->connect_error);
}

//Site details.
$site_domain = "site-domain.com";
$site_name = "site-name";
$site_admin_email = "admin@site-domain.com";

//Perform following action when user registration "Submit button is clicked".
if  (isset($_POST['submit']))
{
	//Check if user filled-in "Username", "Password" and "Email" fields or not. If not, give alert to fill them in.
	if(!empty($_POST["member_registration_username"]) && !empty($_POST["member_registration_password"])&& !empty($_POST["member_registration_email"]))
	{
		$member_registration_username = trim(strip_tags(strtolower(mysqli_real_escape_string($conn,$_POST["member_registration_username"]))));
		$member_registration_password = trim(strip_tags(md5(mysqli_real_escape_string($conn,$_POST["member_registration_password"]))));
        
		//Check for Username match in users	table.	
		$sql = "SELECT * FROM users WHERE Usernames ='".$member_registration_username."'";
		$result = mysqli_query($conn,$sql);
		//If there is a Username match in the "Usernames" column then do the following ...
		if(mysqli_num_rows($result)!=0)
		{
			//Give alert "username" already taken.
			$_SESSION['message']="That Username $member_registration_username is already registered!";
			exit();
		}

		//Check for Email match in users table.
		$sql = "SELECT * FROM users WHERE Emails ='".$member_registration_email."'";
		$result = mysqli_query($conn,$sql);
		
		//If there is a Username match in the "Usernames" column then do the following ...
		if(mysqli_num_rows($result)>0)
		{
			//Give alert "email" already taken.
			$_SESSION['message']="That Email $member_registration_email is already registered!";
			exit();
		}
		
		//Dump new "Username", "Email" and "Password" into "users" table.
	    $sql = "INSERT INTO users(Usernames,Passwords,Emails) VALUES('".$member_registration_username."','".$member_registration_password."','".$member_registration_email."')";
        if($sql)
	    {
			//Give alert dumping new user details into db a success.
	        $_SESSION['message']="Data insertion into table success!";
        }
	    else    
	    {
			//Give alert dumping new user details into db a failure.
            $_SESSION['message']="Data insertion into table failure!";
	    }	
	}
	else
	{	//Give alert to fill-in all fields.
	    $_SESSION['message']="You must fill-in all input fields!";
	}
}

?>
<!DOCTYPE html>
<html>
<head>
<title><?php $site_name ?> Signup Page</title>
  <meta charset="utf-8">
</head>
<body>
<div class = "container">
<form method="post" action="">
<center><h2>Signup Form</h2></center>
<div class="form-group">
<center><label>Username:</label>
<input type="text" placeholder="Enter a unique Username" name="member_registration_username" required [A-Za-z0-9]></center>
</div>
<div class="form-group">
<center><label>Password:</label>
<input type="password" placeholder="Enter a new Password" name="member_registration_password" required [A-Za-z0-9]></center>
</div>
<div class="form-group">
<center><label>Email:</label>
<input type="email" placeholder="Enter your Email" name="member_registration_email" required [A-Za-z0-9]></center>
</div>
<center><button type="submit" class="btn btn-default" name="submit">Register!</button></center>
</form>
</div>
</body>
</html>

New Code:

<?php
require "conn.php";

//Connect to DB.
require "conn.php";

//Grab basic site details.
require "site_details.php";

//Perform following action when user registration "Submit button is clicked".
if  (isset($_POST['submit']))
{
	//Check if user filled-in "Username", "Password" and "Email" fields or not. If not, give alert to fill them in.
	if(!empty($_POST["member_registration_username"]) && !empty($_POST["member_registration_password"])&& !empty($_POST["member_registration_email"]))
	{
		//Check for username match in "Usernames" column in "users"	table. If there is a match then do the following ...
		$stmt = mysqli_prepare($conn, 'SELECT COUNT(*) FROM users WHERE usernames = ?');
		mysqli_stmt_bind_param($stmt, 's', $_POST['member_registration_username']);
		mysqli_stmt_execute($stmt);
		mysqli_stmt_bind_result($stmt, $rows);
		if (mysqli_stmt_fetch($stmt) && $rows) 
		{
			die(
			'That Username '.htmlspecialchars($_POST['member_registration_username']).' is already registered!'
			);
		}

		//Check for email match in "Emails" column is "users" table. If there is a match then do the following ...
		$stmt = mysqli_prepare($conn, 'SELECT COUNT(*) FROM users WHERE emails = ?');
		mysqli_stmt_bind_param($stmt, 's', $_POST['member_registration_email']);
		mysqli_stmt_execute($stmt);
		mysqli_stmt_bind_result($stmt, $rows);
		if (mysqli_stmt_fetch($stmt) && $rows) 
		{
			die(
			'That Email '.htmlspecialchars($_POST['member_registration_email']).' is already registered!'
			);
		}
		
		//Dump new "Username", "Email" and "Password" into "users" table.		
		$name = $_GET['member_registration_username'];
		$password = $_GET['member_registration_email'];
		$password = $_GET['member_registration_password'];
 
		if ($stmt = $mysqli->prepare("INSERT INTO tbl_users (name, password) VALUES (?, ?)")) 
		{ 
		// Bind the variables to the parameter as strings. 
		$stmt->bind_param("ss", $name, $password);
 
		// Execute the statement.
		$stmt->execute();
 
		// Close the prepared statement.
		$stmt->close();
		}	
	}
	else
	{	//Give alert to fill-in all fields.
	    echo "You must fill-in all input fields!";
	}
}

?>
<!DOCTYPE html>
<html>
<head>
<title><?php $site_name ?> Signup Page</title>
  <meta charset="utf-8">
</head>
<body>
<div class = "container">
<form method="post" action="">
<center><h2>Signup Form</h2></center>
<div class="form-group">
<center><label>Username:</label>
<input type="text" name="member_registration_username" required [A-Za-z0-9]></center>
</div>
<div class="form-group">
<center><label>Password:</label>
<input type="password" name="member_registration_password" required [A-Za-z0-9]></center>
</div>
<div class="form-group">
<center><label>Email:</label>
<input type="email" name="member_registration_email" required [A-Za-z0-9]></center>
</div>
<center><button type="submit" class="btn btn-default" name="submit">Register!</button></center>
</form>
</div>
</body>
</html>

Fellow programmers, looking at my 2nd code, do you think:

  • it is better;
  • clutter free;
  • more understandable;
  • sql injection free.

And, on my 2nd code, any chance you can help me convert the INSERT sql command (line 45-55) to mysqli style from pdo ?
I got that pdo code from:

Since most of my code, in my many pages script, is in mysqli or procedural style, it will look odd if 10 lines are pdo or oop style.
Yes, I know I know, I should do it in pdo and oop style but I'm still a beginner and most tutorials on basic php are in mysqli and procedural style and so I cannot just switch to pdo and oop just yet. Let me learn to walk first and then I'll hop like a Kangaroo. I'm still a toddler. have to take things one step at a time or I'll get confused and put-off from php.

Question: On my 1st (old code), you will see I don't use the "echo" but "Session Message" instead as 2 youtube tutorials showed to do it that way without giving any explanation why. Therefore, I ask:

  1. What is the difference and benefits (pros) aswell as the cons between the echo and the session message ?
  2. When should I use which one of them ?

Thanks!


#2

Two things I noticed immediately:

  1. I can't see any reason to require 'conn.php' twice at the beginning.

  2. The proper way to check for form submission is

if ($_SERVER['REQUEST_METHOD'] == "POST")

rather than checking if $_POST['submit'] is set.


#3

I see that you set a session variable called message, but I don't see where you actually do anything with it. In some cases you call exit() directly after you set it, so nothing will happen as the script terminates at that point. On the others you don't exit, but when you draw the form afterwards, you don't do anything with the session variable.

For me, the reason to use a session variable to convey an error message would be when you intend to redirect the user if they get something wrong. Let's say your user fills out a form, and you find that they've missed something out. You could set the session message, header redirect to the form again, and when the form is drawn, display the content of the message to show the user what they did wrong. But for a form, you'd probably want more than one message, so you can display it alongside each error - but a single one at the top of bottom or anywhere is better than nothing at all.


#4

Thanks droopsnoot for pointing out my old code's session message flaws and how I can make use of it.
But still, I can do all that with the echo. And so, what is the real difference between the 2 ? Or, is the session message just another fancy way of doing the echo cluttering things up ? I suspect, with it you spit-out messages related to the session such as logging in error messages or login success message and show users their details inside their accounts (homepage) their account details such as user stats or user details (passwords, emails, etc.). But my assumptions could be wrong.
I'm wary to use the session message now since I don't quite understand it. Might aswell stick to the plain old echo, afterall. What do you say ?

Anybody else have any inputs to give on this session message subject ?


#5

Keen Eyes Gandalf!

I never spotted that myself. I guess I hot the CTRL V twice when copy pasting it from another file!
And thank you for the suggestion:

if ($_SERVER['REQUEST_METHOD'] == "POST").

Might aswell try this and see how things go from now on.


#6

Ooops! Another source just brought it to my attention that I'm still breaking rules about storing passwords. Suggested me this:

http://blog.moertel.com/posts/2006-12-15-never-store-passwords-in-a-database.html

I forgot to hash it. Infact, just gonna read up on hashing now.

http://php.net/manual/en/function.password-hash.php
http://php.net/manual/en/function.password-verify.php


#7

I can't believe reddit did not encrypt their users' passwords on their database!

http://blog.moertel.com/posts/2007-02-09-dont-let-password-recovery-keep-you-from-protecting-your-users.html

Anyway, these 2 links were a waste of my time.
Anyone have any better link suggestions are welcome!


#8

You are taking inputs from url and use that as plain text in query its not smart

$name = $_GET['member_registration_username'];
$password = $_GET['member_registration_email'];
$password = $_GET['member_registration_password'];

Btw for storing password use password_hash() and to verify use password_verify() php built in function.


#9

There are a lot of problems with your 2nd snippet. Where do I start?

The first thing I noticed that @mlukac89 also mentions. There should be no reason to use the $_GET super global IF you are using a form submission. The only reason to use the $_GET super global is if you have a search form. Either way, you will GET NULL values on all of those inputs because they are being referenced wrong. If the user submits the form through a POST request and your snippet is looking for a GET request, the values from the form submission WILL NOT go through because the values are not set in the URL. When dealing with a user form submission and NOT a search form, it is recommended to use the $_POST super global for your references.


Another thing that I noticed, what is this line right here?

$password = $_GET['member_registration_email'];

Are you trying to say that the input of email is the variable of password? And then you overwrite the variable of password with the corresponding value of the GET super global of password?

See, this is why you NEED to learn HTML and CSS first because even your HTML and CSS are completely wrong. I really suggest you take a very large step back and learn the basics of HTML and CSS because whatever you have there, requires a lot of fixing to actually get it up to standards.


I won't even go into details for the other problems I see because there's no point. You can lead a horse to water, but you can't make it drink.


#10

Note only that! Look at another typo. Instead of "$email", I typed "$password". Typed the same variable twice.
Everyone overlooked that. Not only in this source but on other sources too! I only spotted it while going through SpaceShipTrooper's post partially about me mistakenly using $GET instead of $POST.
(You know, I am typing underscores after the dollar signs in the GET & POST but they are not appearing in my post after I submit the post here).


#11

SpaceShipTrooper,

$password = $GET['memberregistration_email'];

The above was a typo. You know how it is. When you got 3 lines similar, you only write-up one line and copy and paste 2 more and then change where it is is supposed to be different. In this case, I forgot to change the $password to $email.

As for the $GET. You are correct, I usually use the $POST (check my old code and all my other codes in this forum). It's just another programmer from another source suggested some code to me to prevent sql injection and his code had the $GET (I guess he uses that most over $POST). I just copied parts of his code into my new code but overlooked this part to switch it to $_POST.

Don't worry. I'm a fast learner. Here's a little proof ...
To be on the safe-side, I won't use the $GET or the $POST, I'll use the $REQUEST since that captures both $GET & $_POST. (See, I'm getting somewhere).
So, don't stop here and do carry-on pointing out more mistakes of mine. The whole purpose of me revealing my code in a forum is to get others' feedback and grow stronger (more experienced) in spotting errors. Best to prevent than cure the disease (bugs). If you know what I mean.
So go ahead and give your best shots. Other newbies who pour into this thread (who'll be following the same footsteps as me) would learn from my mistakes and your corrections. Don't let a few silly mistakes I've overlooked or a few typos put you off. I actually, like ramblings and so you're welcome to ramble as much as you want. I noticed on other sources that when an experienced programmer rambles, he doesn't ramble nonsense. Talks from years of experience. And, if I GO all EARS on him then I''ll learn from their past mistakes and become a better programmer than them (maybe) since I get a head-start thanks to their help. Thanks to your help.
I'm not here to compete with anyone. Just want to learn. Want to gain work experience as I learn along. Want to hold hands with the experienced, while I learn to walk while a toddler.
Let's talk about procedural style and mysqli language first. We'll get into oop and pdo when I'm a little more experienced in the basics of the php language and have become an intermediate programmer.

So my friend, what else you had in mind to point-out that was poor in my 2nd code ? I want to see things from your point of view as if I'm looking over your shoulder. It will help me get keen eyes to spot dangerous mistakes in my coding.

PS - When I mention the $POST, $GET and $_REQUEST, I do type the underscores after the dollar signs (so don't point that out as another typo on my part) but this forum is deleting them (apart from 2 spots in this post) as they don''t show-up after I submit the posts. Bear that in mind, EVERYONE!

Thanks!


#12

Almost every newbie mention this great idea but for some reason not a single one ever bothers to "follow the same steps", preferring to ask over and get personalized service.


#13

Never tell user that username or password exists in this way you did die('That Username '.htmlspecialchars($_POST['member_registration_username']).' is already registered!'); its enough to tell them die('Username in use, please select another one.'); same for email.

Why using 2 query's for check for username and email ? Its enough one to select username and email from it.

Make variables for $_POST so you don't need to type so much, you only enters variable. Isn't that simpler ?

Why you checking if post field is empty when you have "required" in your form, check if isset instead ?

Why you not using die(''); in 1 row ? I don't see a point here to break it.

Why are you using prepared statement in if statement if ($stmt = $mysqli->prepare("INSERT INTO tbl_users (name, password) VALUES (?, ?)"))? You can easy check if data iis inserted by calling last inserted id from database connection if($mysqli->lastInsertId())


#14

The forum software uses the underscore character to show words in italics, which I typed as _italics_, so when you put a single one in, the message goes italic until it gets another one, or perhaps until some other thing happens. Surround the word in back-ticks ` and it will display the underscore correctly.


#15

You can, but if you're going to redirect the user to the registration form so they can try again, either your echo will be lost, or you'll have to display it and then make the user hit a link to re-display the form. In that specific instance, I'd think people would prefer you'd re-display the form and show them what the problem is.


#16

So your script would need to look like this.

And you can set a one more password field so user can verify password, its implemented in most of registration scripts. And put in captcha to prevent spam on your form.

<?php

/*
*	ini_set('display_errors', 1);
*   ini_set('display_startup_errors', 1);

*	For All Error, Warning and Notice
*   error_reporting(E_ALL); OR error_reporting(-1);
*	For All Errors
*   error_reporting(E_ERROR);
*	For All Warnings
*   error_reporting(E_WARNING);
*	For All Notice
*   error_reporting(E_NOTICE);
*/
error_reporting(E_ALL);

//Connect to DB.
require "conn.php";

//Grab basic site details.
require "site_details.php";

//Perform following action when user registration "Submit button is clicked".
if  (isset($_POST['submit']))
{
	
	//Check if user filled-in "Username", "Password" and "Email" fields or not. If not, give alert to fill them in.
	if(isset($_POST['member_registration_username']) && isset($_POST['member_registration_password']) && isset($_POST['member_registration_email'])) {

		// just remove space from start of string
		$username = trim($_POST['member_registration_username']);
		$password = trim($_POST['member_registration_password']);
		$email = trim($_POST['member_registration_email']);

		//Check for username and email match in "Usernames and Email" column in "users"	table.
		$stmt = $mysqli->prepare($conn, "SELECT usernames, emails FROM users WHERE usernames = ? OR emails = ?");
		$stmt->bindParam(1, $username);
		$stmt->bindParam(2, $email);
		$stmt->execute();

		$rows = $stmt->fetch();

		// check if username exists
		if ($rows['usernames'] == $username) {
			echo "Username in use, please choose another one.";
		// check if email exists
		} elseif ($rows['email'] == $email) {
			echo "Email in use, please choose another one.";
		// validate email
		} elseif (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
			echo "Invalid email format";
		// check the lenght of password
		} elseif (strlen($password) < 8 || strlen($password) > 20) {
			echo "Password must be between 8 and 20 characters long.";	
		} else {

			// hash password
			$password = password_hash($password, PASSWORD_DEFAULT);

			// make prepared statement
			$stmt = $mysqli->prepare("INSERT INTO tbl_users (name, password, email) VALUES (?, ?, ?)"); 
			$stmt->bindParam(1, $name);
			$stmt->bindParam(2, $password);
			$stmt->bindParam(3, $email);
			$stmt->execute();

			// check if connection is returning last inserted id
			if ($mysqli->lastInsertId()) {
				echo "User registered.";
				/*
				* redirect user or send activation email
				* if you want to send activation mail make a hash and store it into database
				* when user gets email give him link to activation page + hash
				* then on activation page check hash from url against hash stored in database
				* if hash exists, remove it and that mean user is active
				*/
			} else {
				echo "User not registered.";
			}
		}
		// Close connection
		$mysqli->close();

	} else {
	    echo "You must fill-in all input fields!";
	}
}

?>
<!DOCTYPE html>
<html>
<head>
<title><?php echo $site_name ?> Signup Page</title>
  <meta charset="utf-8">
</head>
<body>
<div class = "container">
<form method="post" action="">
<center><h2>Signup Form</h2></center>
<div class="form-group">
<center><label>Username:</label>
<input type="text" name="member_registration_username" required [A-Za-z0-9]></center>
</div>
<div class="form-group">
<center><label>Password:</label>
<input type="password" name="member_registration_password" required [A-Za-z0-9]></center>
</div>
<div class="form-group">
<center><label>Email:</label>
<input type="email" name="member_registration_email" required [A-Za-z0-9]></center>
</div>
<center><button type="submit" class="btn btn-default" name="submit">Register!</button></center>
</form>
</div>
</body>
</html>

#17

Yes, I came across this problem you mention.
Anyway, is that why the youtube 2 programmers sued the session message instead of the echo ? Is that the magif of the session message ? I still got to learn the difference between the 2 so I use them correctly.
If you glance over my 1st code, I use session messages just like the 2 youtube tuts did but on my 2nd code I weed them out in favour of the echo because it seems to me that the session message and echo are the same thing and so why clutter the script with too many characters from the "session message" word ?


#18

Don't worry about the email confirmation and password confirmation and email confirmation link (account activation link auto emailed to user). I originally had them. Look-out for my old threads and you will see what I mean. They have them. But my old threads were in non-prepared statements. And programmers everywhere started showing annoyance why I'm not using prepared statements and so I opened this thread to learn how to prepare statements from you guys and so I took-out the email and password confirmation fields and any other stuffs that I thought weren't necessary at this level 1 of learning how to prepare a statement for the new script to look less cluttered. Once I've learnt the basics of how to prepare a statement, I would then start re-adding the email and password confirmations aswell as the account activation through email confirmation feature.
I was gonna ask you if you could be kind enough to provide some code examples of where I went wrong but you already did that. I am happy. I am going to now thoroughly go through your code and so don't roll your eyes if I start pouring any questions your way regarding your code if the answers are obvious as I'm still a complete beginner. A toddler in php programming. Still learning how to walk.


#19

You can put in session too, its depends of a programer and coding style. If you put errors in session you can transfer them to other scripts and back, because session exists all time until its destroyed.


#20

I really thought that was the case and I asked programmers if that was the case but no one replied and so I went on with my assumptions that the session message and the echo are still the same thing but now you have confirmed my original assumption that the session message and echo are not quite the same as the former can carry message onto other pages or parts of the script aslong as it's dealing within the same session.