Not only is there code that does this type of stuff already out there, freely available. This question has been asked and answered a couple of times before on this site already. This is one such example.
However, given that the answers currently posted here are either incomplete or, IMHO, wrong (suggesting bad practices, or providing incomplete info), I'll post a short review of mine here, too.
You have a simple, procedural script. There's nothing wrong with that, per se. Although, nowadays, people will tell you to pour your code into objects and let each class do its own, specific task.
Why should you use objects here? Well, that's really down to what kind of project you're putting together: is the site going to be a fully dynamic LAMP driven site? Is the code likely to grow, or is it probable that, as you go along, new features will be added to your site? Then yes, you should look into writing your own classes, or adopt a framework (ZendFW, Symfony2,... there are tons to choose from).
If the amount of PHP you'll be using is relatively small, and its role won't exceed simple user input checks and some DB connectivity, then OO code is just going to add clutter, and bloat.
On the code
Connecting to the DB, and executing queries alongside code that actually generates output for the user/client is one of the main reasons why PHP is said to be a badly designed language. Yes, PHP allows you to write that kind of code, but you should know that it's actually seen as bad practice. A saw doesn't prevent you from sawing off the branch you're sitting on, but that doesn't mean it's a good idea to do so... Same thing applies to programming.
If you don't require object-oriented code (yet), at least create functions to handle the DB stuff, put them in a separate file, and call the functions when you need them:
require 'db_functions.php';//or require_once
$db = getDbConnection(
array(
'user' => 'username',
'pass' => 'yourpass'
)
);
Perhaps look into ways to keep the actual login credentials oustide of your code.
Escaping and assigning
I cannot, for the life of me understand why you would do anything like:
$name = $con->real_escape_string($_POST['name']);
with all of your post values. All those values belong together: they represent the data the user posted to your script, to validate and process. Why would you assign them to individual variables? keep that data as a whole.
If you want to check if an array has all values/keys set, then please use isset
and then compare the values to an empty string. empty
has a couple of oddities you should be aware of.
In this case, validating the post data can be easily done in a single loop:
$keys = array('name', 'username', 'email', 'pass1', 'pass2');
foreach ($keys as $k)
{
if (!isset($_POST[$k]) || $_POST[$k] == '')
exit();//<-- more on this later
}
Code like this can be easily poured into a function, to which you can pass both the array to check ($_POST
) and the keys to check ($keys
). Just 2 simple arguments, and you never need to write code assigning and checking variables again, just a single function call will do.
Now, the real_escape_string
calls really are a bad idea. Quote chars, slashes... they're all allowed in email addresses. calling real_escape_string
means that there is a chance that you're screwing up perfectly valid user input, and rendering it invalid. don't.
Using prepared statements here would be the only proper way to go: don't process the input a user gave you. If the input is anything other than perfect, assume the input is invalid or possibly malicious.
Avoid too many regex's
Checking for a password that contains both digits and characters can be done through regex, but that might not be the best tool for the job. The actual benchmarking is your job, but a regex often is slower than a simple loop, checking if a char is numeric or not.
Either way: you are using 2 regular expressions for something that can be easily done with just one expression. I've posted an answer here which goes into more details on the matter.
re-assigning again?
After validating the input (that, as I explained above, you may have rendered invalid yourself!), you then check variables like $emailValid
again. That really ought to set off alarm bells.
Just think about it: you check some things. If the very first thing you checked showed you the user sent invalid input, you still procede to check, validate, reassign all other values, only to then again check if all input is valid.
That's doing the same thing twice, isn't it? DRY (Do not Repeat Yourself) is a common expression/piece of advice in programming. Your code basically repeats the most basic input validation task. That can't be good, now can it?
That's why throwing an exception is preferable, and that is also why separating output code, and logic/db related code into (at least) functions. These functions (functional units, as their name suggests) could be written to throw exceptions, which will then determine the output message the client will get to see.
Here's a basic example to clear this up. Remeber how I mentioned a function to check if all POST params are set? Here's how that would work in your case:
function checkKeys(array $data, array $keys)
{//note the type-hints!
foreach ($keys as $k)
{
if (!isset($data[$k]) || $data[$k] == '')
throw new InvalidArgumentException($k.' parameter not set');
}
return true;
}
//your code:
try
{
checkKeys($_POST, array('name', 'username', 'email', 'pass1', 'pass2'));
}
catch( InvalidArgumentException $e)
{//catch Exception works, too
echo 'Error: ', $e->getMessage();
}
That should be enough to get you started for now ;-P