Could really use some feedback on this registration code in php

Question

I was hoping someone could give me some feedback on my code. I am still new to php and I'm sure I have messed up somewhere. The code pasted is for a registration page where users will submit their information which will then be posted to the database.

<?php>
include 'PasswordHash.php';


$sql = new mysqli('localhost', '#######', '#####', '######');

// Create an array to catch any errors in the registration form.
$errors = array();

/**
 * Make sure the form has been submitted before trying to process it. This is
 * single most common cause of 'undefined index' notices.
 */
if (!empty($_POST))
{
    // First check that required fields have been filled in.
    if (empty($_POST['username']))
    {
        $errors['username'] = "Username cannot be empty.";
    }


    // Restrict usernames to alphanumeric plus space, dot, dash, and underscore.
    /*
    if (preg_match('/[^a-zA-Z0-9 .-_]/', $_POST['username']))
    {
        $errors['username'] = "Username contains illegal characters.";
    }
    */
    if (empty($_POST['firstname']))
    {
        $errors['firstname'] = "First Name cannot be empty.";
    }

    if (empty($_POST['surname']))
    {
        $errors['surname'] = "Surname cannot be empty.";
    }

    if (empty($_POST['password']))
    {
        $errors['password'] = "Password cannot be empty.";
    }


    if (strlen($_POST['password']) < 8)
    {
        $errors['password'] = "Password must be at least 8 charcaters.";
    }


    // Force passwords to contain at least one number and one special character.
    /*
    if (!preg_match('/[0-9]/', $_POST['password']))
    {
        $errors['password'] = "Password must contain at least one number.";
    }
    if (!preg_match('/[\W]/', $_POST['password']))
    {
        $errors['password'] = "Password must contain at least one special character.";
    }
    */

    if (empty($_POST['password_confirm']))
    {
        $errors['password_confirm'] = "Please confirm password.";
    }

    if ($_POST['password'] != $_POST['password_confirm'])
    {
        $errors['password'] = "Passwords do not match.";
    }

    $email = filter_var($_POST['email'], FILTER_VALIDATE_EMAIL);
    if (!$email)
    {
        $errors['email'] = "Not a valid email address.";
    }

    /**
     * Escape the data we're going to use in our query. Never trust user input.
     */
    $username = $sql->real_escape_string($_POST['username']);
    $email    = $sql->real_escape_string($email);
    $firstname    = $sql->real_escape_string($_POST['firstname']);
    $surname    = $sql->real_escape_string($_POST['surname']);
    $addressline1    = $sql->real_escape_string($_POST['addressline1']);
    $addressline2    = $sql->real_escape_string($_POST['addressline2']);
    $city    = $sql->real_escape_string($_POST['city']);
    $county    = $sql->real_escape_string($_POST['county']);
    $postcode   = $sql->real_escape_string($_POST['postcode']);

    /**
     * Check that the username and email aren't already in our database.
     *
     * Note also the absence of SELECT *

     */
    $query  = "SELECT username, email
               FROM users
               WHERE username = '{$username}' OR email = '{$email}'";
    $result = $sql->query($query);

    /**
     * There may well be more than one point of failure, but all we really need
     * is the first one.
     */
    $existing = $result->fetch_object();

    if ($existing)
    {
        if ($existing->username == $_POST['username'])
        {
            $errors['username'] = "That username is already in use.";
        }
        if ($existing->email == $email)
        {
            $errors['email'] = "That email address is already in use.";
        }
    }
}



if (!empty($_POST) && empty($errors))
{
    /**
     * Hash password before storing in database
     */
    $hasher = new PasswordHash(8, FALSE);
    $password = $hasher->HashPassword($_POST['password']);

    $query = "INSERT INTO users (firstname, surname, username,email, password, addressline1, addressline2, city, county, postcode, created)
              VALUES ('{$firstname}','{$surname}','{$username}','{$email}', 
              '{$password}','{$addressline1}','{$addressline2}','{$city}','{$county}','{$postcode}', NOW())";
    $success = $sql->query($query);

    if ($success)
    {
        $message = "Account created.";
    }
    else
    {
        $errors['registration'] = "Account could not be created. Please try again later.";
    }
}
?>

Answer 1

Well, here are my notes from looking through the code:

1. You create a connection to the database, and include PasswordHash.php, when it is not yet needed. Database connections are not "free of charge" and should be used conservatively.

2. You use two if blocks where you could use just one.

3. There is no real checking/validation/sanitation of the incoming data. Sure, you escape it before you INSERT it, but you never check if it is valid. For example, the $county variable is just assumed to be a county... it could be anything.

4. The first SELECT query may not be entirely necessary. If your database schema is set up properly (i.e. using UNIQUE on the username and email fields), then you will be told that the user already exists. You can then take it from there. If is always a good deal if you can eliminate database calls.

5. The $errors array is never used. You probably know this, so I will not use any time on it.

The main problem (if you want to call it that) is that you need to be a little more systematic. You need to think about the whole process in steps, or more as an algorithm:

1. Sanitize the input
2. Validate the input
   1. If the input does not validate, stop, let the user correct it
   2. If the input is valid, go to #3
3. Set up database connection
4. Insert data
   1. If there is a duplicate record error, stop, the user already exists!
   2. If there is no error, go to #5
5. Success, user was created. DONE.

That is about it.