1
\$\begingroup\$

I am relatively new at using PHP, and I am looking for experienced PHP/HTML users to have a look at my script and offer up any advice on what could have been done better or info on any discovered errors.

<!DOCTYPE HTML>
<html lang="en">
<head><meta charset="UTF-8">
<title>Registration</title>
</head>
<body>

<?php

$page_title = 'Register';
include ('includes/header.html');

# conditional test to only execute contained statements if form has been submitted.
if ($_SERVER['REQUEST_METHOD'] == 'POST')
{

# statements to be inserted here.

# open database connection and initialise array for error messages.
require ('../connect_db.php');
$errors = array();

# stores error msg if firstname field remains empty, or store value in a variable.
if (empty($_POST['firstname']))
{$errors[] = 'Please enter your first name.';}
else
{$fn = mysqli_real_escape_string($dbc,
                            trim($_POST['firstname']));}

# stores error msg if lastname field remains empty, or store value in a variable.
if (empty($_POST['lastname']))
{$errors[] = 'Please enter your last name.';}
else
{$ln = mysqli_real_escape_string($dbc,
                            trim($_POST['lastname']));}

#   stores error msg if address field remains empty, or store value in a variable.                      
if (empty($_POST['address']))
{$errors[] = 'Please enter your address.';}
else
{$a = mysqli_real_escape_string($dbc,
                            trim($_POST['address']));}

#   stores error msg if town field remains empty, or store value in a variable.
if (empty($_POST['town']))
{$errors[] = 'Please enter your town.';}
else
{$t = mysqli_real_escape_string($dbc,
                            trim($_POST['town']));} 

#   stores error msg if postcode field remains empty, or store value in a variable.                     
if (empty($_POST['postcode']))
{$errors[] = 'Please enter your postcode.';}
else
{$pc = mysqli_real_escape_string($dbc,
                            trim($_POST['postcode']));}

#   stores error msg if email field remains empty, or store value in a variable.
if (empty($_POST['email']))
{$errors[] = 'Please enter your email.';}
else
{$e = mysqli_real_escape_string($dbc,
                            trim($_POST['email']));}

#   stores error msg if username field remains empty, or store value in a variable.
if (empty($_POST['username']))
{$errors[] = 'Please enter your username.';}
else
{$u = mysqli_real_escape_string($dbc,
                            trim($_POST['username']));} 

#   stores password as a variable if both password fields match, or store an error msg if not matching or first field is empty.
if (!empty($_POST['password1']))
{
    if ($_POST['password1'] != $_POST['password2'])
    {$errors[] = 'Passwords do not match.';}
    else
    {$p = mysqli_real_escape_string($dbc,
                            trim($_POST['password1']));}
}
else {$errors[] = 'Please enter your password.';}

#   stores error msg if email already exists in database.
if (empty($errors))
{
    $q = "SELECT username FROM users WHERE email='$e'";
    $r = mysqli_query ($dbc,$q);
    if (mysqli_num_rows($r) != 0)
    {$errors[] = 'Email address already registered.
                            <a href="login.php">Login</a>';}
}

#   stores user data in database and displays a confirmation message when registration is successful, closes the database connection and includes a page footer as well as exit the script.
if (empty($errors))
{
    $q = "INSERT INTO users
        (firstname, lastname, address, town, postcode, email, username, password)
        VALUES ('$fn', '$ln', '$a', '$t', '$pc', '$e', 'u', SHA1('$p'))";
    $r = mysqli_query ($dbc,$q);

    if ($r)
    {
        echo '<h1>Registered!</h1>
            <p>You are now registered.</p>
            <p><a href="login.php">Login</a></p>';
    }

    mysqli_close($dbc);
    include ('includes/footer.html');
    exit();
}

#   displays all stored error msg when registration fails and closes database connection.
else
{
    echo '<h1>Error!</h1>
    <p id="err_msg">The following error(s) occurred:<br>';
    foreach ($errors as $msg)
    {
        echo " -$msg<br>";
    }
    echo 'Please try again.</p>';
    mysqli_close($dbc);
}

}
?>

<!--Sticky HTML form-->
<h1>Register</h1>
<form action="register.php" method="POST">
<p>
First Name: <input type="text" name="firstname"
value="<?php if (isset($_POST['firstname']))
    echo $_POST['firstname'];?>">
Last Name: <input type="text" name="lastname"
value="<?php if (isset($_POST['lastname']))
    echo $_POST['lastname'];?>">
    </p><p>
Address: <input type="text" name="address"
value="<?php if (isset($_POST['address']))
    echo $_POST['address'];?>">
Town: <input type="text" name="town"
value="<?php if (isset($_POST['town']))
    echo $_POST['town'];?>">
Postcode: <input type="text" name="postcode"
value="<?php if (isset($_POST['postcode']))
    echo $_POST['postcode'];?>">
</p><p>
Email Address: <input type="text" name="email"
value="<?php if (isset($_POST['email']))
    echo $_POST['email'];?>">
</p><p>
Username: <input type="text" name="username"
value="<?php if (isset($_POST['username']))
    echo $_POST['username'];?>">
</p><p>
Password: <input type="password" name="password1"
value="<?php if (isset($_POST['password1']))
    echo $_POST['password1'];?>">
Confirm Password: <input type="password" name="password2"
value="<?php if (isset($_POST['password2']))
    echo $_POST['password2'];?>">
</p><p>
<input type="submit" value="Register"> </p>
</form>

<?php include ('includes/footer.html');?>

</body>
</html>
Improve this question
\$\endgroup\$
2
  • \$\begingroup\$ Way too "wet". DRY (don't repeat yourself) up your code and ALWAYS use prepared statements. Escape strings are a thing of the past, you'll simplify your code and make it more secure properly using prepared statements. \$\endgroup\$ – Devon Jan 17 '17 at 0:28
  • \$\begingroup\$ Don't use SHA1 for password hashing. At least use a newer algorithm (bcrypt or sha2), or just let PHP handle this for you using password_hash() and password_verify(). \$\endgroup\$ – Devon Jan 17 '17 at 0:34
2
\$\begingroup\$

This code has a number of problems.

First and foremost is that, stylistically, this code is a bit of a mess. I would suggest you look at the PHP Standards Recommendations as a good starting point for style guides you can follow.

Most importantly, you need to use indentation and think of ways to separate out your PHP code from HTML.

Don't start your output to the browser (the HTML part) until you have done all of your primary PHP logic (i.e. validating form data, interacting with database, etc.). You will find over time in working with PHP that separating core logic from display becomes very important. Once you have started output, you will no longer be able to do things like change the response headers (i.e. for redirect).

Consider using filter_input_array() as a means to better define your validation rules and apply them in one pass.

You probably should not be directly re-populating $_POST values into your HTML template. This is causing you to have to do a bunch of if(isset(...)) logic in your display template.

I would prefer to see you populate an object, associative array, or a series of variables to place value into your forms. So more a pattern like:

// before you start outputting HTML
$post_data = filter_input_array(INPUT_POST, $validation_rules);
$form = new stdClass();
$errors = new stdClass();
foreach ($post_data as $key => $value) {
    if(is_null($value)) {
        $errors[$key] = 'Invalid value for ' . $key;
        $value = '';
    }
    $form[$key] = $value;
}


// and in your form
<input type="text" name="firstname" value="<?php echo $form->firstname; ?>">

As noted in comments, you should begin to work with prepared statements when querying the database. I would also suggest using mysqli in an object-oriented fashion.

Again don't intersperse core PHP logic with display logic like you do here:

if ($r)
{
    echo '<h1>Registered!</h1>
        <p>You are now registered.</p>
        <p><a href="login.php">Login</a></p>';
}

mysqli_close($dbc);
include ('includes/footer.html');
exit();

You would be better served setting a "registered" flag and fork out you display option like:

if($registereed) {
?>

<h1>Registered!</h1>
<p>You are now registered.</p>
<p><a href="login.php">Login</a></p>

<?php
} else {
?>
<!-- your form display -->
<?php
include ('includes/footer.html');
?>
</body>
</html>

This allows you to only have to specify the footer include once vs. having multiple exit paths from this script.

One of the problems with PHP is that there are innumerable examples of bad coding practice out there (probably the vast majority of the examples out there). Based on your code, my guess is you have encountered just such bad examples and/or inherited a poorly written codebase.

As an additional resource for you, I would highly recommend PHP: The Right Way. This is, IMO, the best collection of information and examples for someone diving into PHP.

Improve this answer
\$\endgroup\$
1
\$\begingroup\$

A quick way you could DRY this up as per my comment:

<?php

$required = [
    'firstname',
    'lastname',
    'email'
];

foreach($required as $field) {
   if (empty($field)) {
       $errors[] = 'Please enter your ' . $field . '.';
   }
}

Keep your validation logic together. Don't mix everything together.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.