2
\$\begingroup\$

I asked this question before and mickmackusa and Your Common Sense gave me some good answers. I went over them and made as much changes as I could because some of the code didn't work properly for me. So again i want to know if this is a good way to have my registration code for people to sign up.

//check if form is submitted
if ( $_SERVER['REQUEST_METHOD'] != 'POST' || ! isset($_POST['Register'])) {

    // looks like a hack, send to index.php
    header('Location: ../../index.php');
    die();
}

require '../../config/connect.php';
$con = new mysqli(...$dbCredentials);

$first_name = $_POST['first_name'] ?? '';
$last_name = $_POST['last_name'] ?? '';
$username = $_POST['username'] ?? '';
$email = $_POST['email'] ?? '';
$pw = $_POST['pw'] ?? '';
$pw2 = $_POST['pw2'] ?? '';

$errors = [];

if (!trim($first_name)) {

    $errors[] = "Fill in first name to sign up";
}
if (!ctype_alnum($first_name)) {
    $errors[] = "Invalid first name, it only may contain letters or digits";
}

if (!trim($last_name)) {

    $errors[] = "Fill in last name to sign up";
}
if (!ctype_alnum($last_name)) {
    $errors[] = "Invalid last name, only letters and/or digits.";
}

if (!trim($username)) {

    $errors[] = "Fill in last name to sign up";
}
if (!ctype_alnum($username)) {
    $errors[] = "Invalid last name, only letters and/or digits.";
}

if (!trim($pw)) {
    $errors[] = "Fill in password to sign up";
}
if (!trim($pw2)) {
    $errors[] = "Confirm password to sign up";
}

if (!trim($email)) {
    $errors[] = "Fill in email to sign up";
}
if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
    $errors[] = "Invalid email";
}

if ($pw !== $pw2) {

    echo "Your passwords do not match";
}

if (!$errors) {

    $check_email = $con->prepare("SELECT username FROM users WHERE username=?");
    $check_email->bind_param("s", $username);
    $check_email->execute();
    $row = $check_email->get_result()->fetch_assoc();

    if ($row) {

        echo "That username is already in use";
        die();
    }
}

if (!$errors) {

    $check_username = $con->prepare("SELECT email FROM users WHERE email=?");
    $check_username->bind_param("s", $email);
    $check_username->execute();
    $row = $check_username->get_result()->fetch_assoc();
    
    if ($row) {
        echo "That email is already in use";
        die();
    }
}

if (!$errors) {

    $pw = password_hash($_POST['pw'], PASSWORD_BCRYPT, array('cost' => 14));

    $stmt = $con->prepare("INSERT INTO users (first_name, last_name, username, email, pw)
        VALUES (?, ?, ?, ?, ?)");
    $stmt->bind_param("sssss", $first_name, $last_name, $username, $email, $pw);
    $stmt->execute();
    
    $_SESSION["id"] = $_POST['username'];
    header("Location: ../../index.php");
    exit();

} else {
    // The foreach construct provides an easy way to iterate over arrays. 
    foreach ($errors as $error) {
        $errors[] = 'An error occurred.';
    }
    
}
\$\endgroup\$
11
  • \$\begingroup\$ You should really invest some effort in testing your code before posting it. What the last foreach loop is supposed to do? \$\endgroup\$ – Your Common Sense Jul 21 '20 at 4:18
  • \$\begingroup\$ I think it puts the $errors in an array. \$\endgroup\$ – user13477176 Jul 21 '20 at 12:28
  • \$\begingroup\$ isn't $errors already an array? \$\endgroup\$ – Your Common Sense Jul 21 '20 at 12:29
  • \$\begingroup\$ I thought this $errors = []; puts it in an array \$\endgroup\$ – user13477176 Jul 21 '20 at 12:30
  • \$\begingroup\$ Oh wait sorry. I was paying attention close enough \$\endgroup\$ – user13477176 Jul 21 '20 at 12:31
1
\$\begingroup\$

While it is sensible to some people to precisely specify the cause of the submission failure, I prefer to return compound messages rather than a length error message and a quality error message. I would probably save on screen space (versus piling upto a dozen separate email messages on the user), and merge some responses like:

First name is a required field and may only contain letters and/or digits.

Also, you are repeating yourself a few times with these error messages and you can easily implement a looped battery of checks -- and it is simpler to do that within the $_POST array. Not only does this avoid some code bloat, you are assured to have consistent error message and reduce your chances of copy-pasting typos. (Maybe you didn't realize that you wrote last name in the username errors 😊 -- this is the kind of thing that really perplexes users!)

Note: ctype_alnum() will return false on a zero-length string so the !trim() can be omitted.

$alnums = ['first_name', 'last_name', 'username'];  // whitelist of required alphanumeric fields
foreach ($alnums as $field) {
    if (!isset($_POST[$field]) || !ctype_alnum($_POST[$field])) { 
        $errors[] = "$field is a required field and may only contain letters and/or digits.";
    }
}

if (!isset($_POST['email']) || !filter_var($_POST['email'], FILTER_VALIDATE_EMAIL)) {
    $errors[] = "Email is a required field and must be valid.";
}

if (!isset($_POST['pw'], $_POST['pw2']) || $pw !== $pw2) {
    $errors[] = "Password and Password 2 are required fields and must be identical.";
}

Ultimately, all of these server-side checks are to protect you and your database -- NOT to help your users. For the best possible user experience (UX), you need to duplicate all of these validations with javascript onsubmit of the form. This way you inform the user as quickly as possible AND avoid making a fruitless trip to your server.

You can check for unique usernames and email addresses in a single trip to the database. Because the earlier validation assures no leading or trailing whitespace, the posted data will match the respective result set values (iow, no trimming is needed).

if (!$errors) {
    $stmt = $con->prepare("SELECT username, email FROM users WHERE username=? OR email=?");
    $stmt->bind_param("ss", $_POST['username'], $_POST['email']);
    $stmt->execute();
    foreach ($stmt->get_result() as $row) {
        foreach (['username', 'email'] as $field) {
            if ($row[$field] == $_POST[$field]) {
                $errors[] = "Sorry, the submitted $field is already in use.";
            }
        }
    }
}

Once you have fully validated all incoming data as valid, THEN you can happily insert the new user into your database and save the SESSION data. By the way, I don't recommend changing the naming convention from username to id.

If any of the checkpoints are failed then present your $errors. As already pointed out, you are not actually displaying your error messages. In simplest terms, you could create <div> tags as you loop.

if (!$errors) {
    $pw = password_hash($_POST['pw'], PASSWORD_BCRYPT, ['cost' => 14]);
    $stmt = $con->prepare("INSERT INTO users (first_name, last_name, username, email, pw) VALUES (?, ?, ?, ?, ?)");
    $stmt->bind_param(
        "sssss",
        $_POST['first_name'],
        $_POST['last_name'],
        $_POST['username'],
        $_POST['email'],
        $_POST['pw']
    );
    $stmt->execute();
    
    $_SESSION["username"] = $_POST['username'];  // I always expect an id to be an integer
    header("Location: ../../index.php");
    exit();
}

foreach ($errors as $error) {
    echo "<div class=\"error\">$error</div>";
}

None of the above snippets have been tested; I make no guarantees that they will work "out of the box".

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for suggesting the trim() in my snippet, @YourCommonSense. I do, however, deliberately use the result set object directly in the loop to avoid calling any fetching functions. I have just tested my technique locally to make sure that it works on result sets with and without rows. I do not like to use fetchAll() unless I am performing result set processing in a different "layer". \$\endgroup\$ – mickmackusa Jul 27 '20 at 9:06
  • \$\begingroup\$ My bad I completely forgot that result is iterable \$\endgroup\$ – Your Common Sense Jul 27 '20 at 9:07
  • \$\begingroup\$ I've thought a bit more about it and I've rolled back the looped trims too because the ctype_alnum() and email validation will rule out leading and trailing whitespaces as well as empty values. \$\endgroup\$ – mickmackusa Jul 27 '20 at 12:19
  • \$\begingroup\$ My bad again I didn't think of it \$\endgroup\$ – Your Common Sense Jul 27 '20 at 12:27
0
\$\begingroup\$
  1. Every other condition (for user input validation) adds to the errors array, whereas password matching simply does an echo?

     if ($pw !== $pw2) {
         echo "Your passwords do not match";
     }
    
  2. When checking for existing username/email, you don't really need to fetch the associated row itself. You can use the num_rows to check if it is \$ \ge 1 \$. These check for existing conditions also have echo statements, instead of appending to the errors array.

  3. If you're on php 5.4+, the arrays can be declared using square brackets. No need for the array() method:

     $pw = password_hash($_POST['pw'], PASSWORD_BCRYPT, ['cost' => 14]);
    

You generate the list/array of errors occurred during processing of the user input, but at the end, do nothing useful with it. Ideally, these errors should be returned back to the application and shown to the user so that they may update values as needed.

\$\endgroup\$
2
  • \$\begingroup\$ If there is an error they will see it on that page. \$\endgroup\$ – user13477176 Jul 21 '20 at 0:57
  • 1
    \$\begingroup\$ it's on the contrary. When checking for existing username/email, you don't really need no num rows, fetch is enough. It just makes no sense to select some data but not to fetching it. \$\endgroup\$ – Your Common Sense Jul 21 '20 at 4:17
0
\$\begingroup\$

Some thoughts:

One thing I liked is the null coalescing operator (??). If I'm not wrong it's one of those new features in PHP7.

Clumsy redirect

header('Location: ../../index.php');

Use a full (absolute) URL:

header('Location: https://www.yoursite.com/index.php');

In fact this is even stipulated in RFCs like 2616, for good reasons. Otherwise the browser is left to interpret what you mean. In practice the browser will use the current URL to determine the destination but you are relying on the browser to "guess" what the correct location should be. Avoid ambiguity.

The root URL for your site should be defined as a constant in your application. Then all you have to do is append the desired page to the root URL to perform the redirect.

Use absolute URLs for the links on your site too. Speaking of relative links, be aware there is an HTML <base> tag.

Internationalization is lacking

I have the impression that your code is not character set-aware. Have you ever thought about accents ?

This is an arbitrary decision:

if (!ctype_alnum($first_name)) {
    $errors[] = "Invalid first name, it only may contain letters or digits";
}

Why not allow more characters like apostrophes ? What if my name is Sarah O'Connor or Jean-Michel Jarre ? I cannot register on your site ? The bigger problem is that you are forcing users to use Latin characters whereas they might want to use Arab script or Chinese characters or whatever. Lots of people are not comfortable with ASCII. Likewise, you would struggle on a Chinese keyboard.

Embrace Unicode now. Even if you focus on a narrow audience of native English speakers, people are going to post non-ASCII characters in comments anyway (think about emojis). So your script should handle Unicode, and the database should store the data properly so that it can be rendered as it was typed in. If you garble the comments of users they will be unhappy.

And in fact, your HTML pages probably have a content-encoding already, if not UTF-8 it might be ISO-8859-1 or something like that - I would check. This is an important detail because it dictates how your server will receive the form data.


Breaking it up

I would also break up the code in small functions (I must have said that lots of times), for example one function that validates the input, another one that registers the user in the DB. Your functions can return a boolean value or an array of errors. Or they can be implemented as classes.

It makes the code more manageable and you can also move the functions to include files to declutter that file. You don't want to scroll a page that is 5000 lines long. The more code you add, the more tedious your job becomes. And the code becomes ugly.

Believe me, you are going to add a lot more code to have a decently-working application (if you don't lose motivation in the meantime). I do more Python these days, but I usually avoid having more than 400 lines in a single file. Smaller files are more manageable. Right now you have only 110 lines but wait.

And in fact, after registration it is customary to send an E-mail (hint: another template), with a link to be clicked to verify the E-mail. So there will be more logic involved in the registration process.

So I would probably ditch this:

$_SESSION["id"] = $_POST['username'];

because I don't want the user to be logged in (have a valid session) and able to post until they have actually verified their E-mail (but where is session_start ?). I would rename the session variable $_SESSION["username"] to be consistent, unless you wanted to use the newly-created user ID instead.


I have said it before, but frameworks exist for a reason: to relieve developers of complexity and avoid reinventing the wheel. Although it is good to have "low-level" understanding of how things work under the hood, one has to keep pace with modern development.

Another benefit would be to use templates to better separate presentation from logic. Mixing HTML with PHP (or other code) is ugly. To be honest showing something like this to a user is a bit terse:

if ($row) {
    echo "That email is already in use";
    die();
}

In 2020 people are normally expecting a good-looking HTML page, with alerts, colors, images and all that stuff. Good presentation is important. You already have the page, you just have to show errors when and if they occur.


One suggestion: look at open-source forum software, analyze the code, and see how it's done. Don't always try to reinvent the wheel. I am not saying that you should blindly copy from other people, no the point is to learn from others and improve where you can. Yes, you will find bad stuff too but hopefully you will recognize it. You learn a lot by looking at other people's code because it makes you think about why they did this or that. I myself am never satisfied with my code, I think code can always be improved.

\$\endgroup\$
1

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.