1
\$\begingroup\$

I have improved my code since the last time asking for its review.

I'd like to know if it is up to the current standards.

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
error_reporting(-1); 

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

$first_name = htmlspecialchars(trim(strip_tags(filter_var($_POST['first_name'], 
    FILTER_SANITIZE_STRING))));

$last_name = htmlspecialchars(trim(strip_tags(filter_var($_POST['last_name'], 
    FILTER_SANITIZE_STRING))));  

$username = htmlspecialchars(trim(strip_tags(filter_var($_POST['username'], 
    FILTER_SANITIZE_STRING))));

$email = htmlspecialchars(trim(strip_tags(filter_var($_POST['email'], 
    FILTER_VALIDATE_EMAIL))));
$pw = '';
$pw2 = '';
$friend_array = ',';

$errors = [];

if (empty($_POST["first_name"])) {

    $errors[] = "Fill in first name to sign up";
}

if (empty($_POST["last_name"])) {

    $errors[] = "Fill in last name to sign up";
}

if (empty($_POST["username"])) {

    $errors[] = "Fill in username to sign up";
}

if (empty($_POST["email"])) {

    $errors[] = "Fill in username to sign up";
}

if (empty($_POST["pw"])) {
    $errors[] = "Fill in password to sign up";
}

if (empty($_POST["pw2"])) {
    $errors[] = "Confirm password to sign up";
}

if (!$errors) {
    //An SQL statement template is created and sent to the database
    $check_username = $con->prepare("SELECT username FROM users WHERE username=?");
    // This function binds the parameters to the SQL query and tells the database what the parameters are.
    $check_username->bind_param("s", $_POST['username']);
    // the database executes the statement.
    $check_username->execute();
    $row = $check_username->get_result()->fetch_assoc();

    if ($row && $row['username'] == $_POST['username']) {

        $_SESSION['error'] = '<b><p style="color: #000000; font-size: 25px; top: 34%;right: 30%;position: absolute;">Username exists</p></b>';
        header('Location: ../../register.php');
        exit();
    }
}

if (!$errors) {
    //An SQL statement template is created and sent to the database
    $check_email = $con->prepare("SELECT email FROM users WHERE email=?");
    // bind_param binds the parameters to the query and tells the DB what the parameters are.
    $check_email->bind_param("s", $email);
    // the database executes the statement.
    $check_email->execute();
    $row = $check_email->get_result()->fetch_assoc();

    if ($row && $row['email'] == $_POST['email']) {

        $_SESSION['error'] = '<b><p style="color: #000000; font-size: 25px; top: 34%;right: 30%;position: absolute;">E-mail exists</p></b>';
        header('Location: ../../register.php');
        exit();
    }
}

if ($_POST['pw'] !== $_POST['pw2']) {

     $_SESSION['error'] = '<b><p style="color: #000000; font-size: 25px; top: 34%;right: 30%;position: absolute;">The passwords do not match.</p></b>';
    header('Location: ../../register.php'); 
    exit();  
}

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, friend_array) 
        VALUES (?, ?, ?, ?, ?, ?)");
    $stmt->bind_param("ssssss", $_POST['first_name'], $_POST['last_name'], $_POST['username'], 
        $_POST['email'], $pw, $friend_array);
    $stmt->execute();

    $_SESSION['error'] = '<b><p id="loginnow">You can now login.</p></b>';
    header('Location: ../../register.php');
    exit();

} else {
    // The foreach construct provides an easy way to iterate over arrays. 
    foreach ($errors as $error) {
        echo "$error <br /> \n";
    }
    
    $_SESSION['error'] = '<b><p style="color: #fff; font-size: 25px; top: 15%;right: 30%;position: absolute;">An error occurred.</p></b>';
    header('Location: ../../register.php');
    exit();
}
\$\endgroup\$
0
3
\$\begingroup\$
  1. Regarding setting up your database connection and error reporting, please refer to this recent post from YCS.
  2. I'm noticing that you are unconditionally mutating several of the user submitted values. If you have any concerns that someone might legitimately access this page without submitting these variables, then you should have a condition to determine if the script should be used as a signup attempt or merely bounced. Otherwise, if the only reason that a legitimate user will be accessing this page is via a signup attempt, then I would say you should remove the empty() calls and use a function-less falsey check (e.g. if (!$_POST["first_name"]) {) because if the variables are guaranteed to be set, then empty() is doing unnecessary work.
  3. Be sure that you are calling the right encoding function for the right reason and at the right time. In short, you should not be htmencoding values before they go into the db, you should only be encoding strings just before printing them to screen. Please read: htmlentities() vs. htmlspecialchars() and PHP & mySQL: When exactly to use htmlentities?
  4. I don't see any reason to declare these empty strings as variables: $pw = ''; and $pw2 = '';. I also think it is poor naming convention to declare string data as a variable including the word array such as $friend_array = ',';.
  5. I think if ($_POST['pw'] !== $_POST['pw2']) { should be moved up the script with the other validators. You don't want to do any db fetching until the user input has clear all basic hurdles.
  6. The secondary condition in if ($row && $row['email'] == $_POST['email']) { is nonsense. You have already required this expression to be true in your sql. There is no benefit in the redundant check in php. In fact, because you only need to check if the username exists in the db, you should probably just write a COUNT() query. See this suggestion: Return row count from mysqli prepared statement
  7. I would not be bloating my $_SESSION array with the markup that you are piling into $_SESSION['error']. Trim all the fat and just keep the "white meat": $_SESSION['error'] = 'E-mail exists'. You should be moving all of your styling to an external stylesheet anyhow. Drop that <b> tag and just use styling. Never use more html markup than you need to.
  8. I don't like the idea of saving a default value of a comma in the friend_array column. If they have no friends, then the value should be NULL (or an empty string if you must). That said, if you are unconditionally hardcoding a value to be INSERTed into every row, then that argues that you should be modifying the table structure and declaring the default value for friends_array as ','. This way you don't even need to declare the $friends_array variable or bloat your prepared statement / binding with the extra syntax.
  9. "You can now login." is not an error, so it is inappropriate to write that data into $_SESSION['error']. The next developer is going to be scratching their head at your decision to write conflicting data into certain SESSION elements. Create better naming convention and/or change your SESSION structure.
  10. I don't like that you are printing the $errors then redirecting the user. Please read this: Redirect a user after the headers have been sent Not only will header() prompt problems after you have printed to screen, it doesn't make a lot of sense if you aren't going to let them see the errors that are printed. I'd say it makes better sense to push all of the errors into $_SESSION['errors'], then after the redirect you should echo implode("<br>\n", $_SESSION['errors']); in whatever format will be attractive to the user.
  11. I do like that you are sanitizing the input heavily and that you are using $_POST to transfer the form data to a database writing process.

A late note... if the friends_array is a collection of user ids/usernames, then a First Normal Form (1NF) rule should be implemented. Ditch the friends_array column. Create a new table called friends with separate rows for each relationship. The functional benefits are many.

\$\endgroup\$
3
  • \$\begingroup\$ If you don't mind, can you show me some links that made you such an expert ? I'm trying to find some things but whenever I think I learned, someone tells me it's wrong. Thanks \$\endgroup\$ – user13477176 Jul 18 '20 at 16:21
  • 1
    \$\begingroup\$ I am a self-taught php developer for over 13 years now. I am not comfortable with the label "expert". This is a bit triggering for me. I don't think I'll ever call myself an expert or even a Senior Dev because it seems no matter how much I learn there is always an enormous amount that I haven't learned. I have given my honest review and included some links. With all due respect I don't really "feel like" or "want to get into the habit of" defending every bullet point with a hyperlink. Expect a career of life-long-learning. Find multiple educational Stack Exchange users and digest their wisdom. \$\endgroup\$ – mickmackusa Jul 18 '20 at 22:18
  • \$\begingroup\$ Thanks a lot. I'll start with you \$\endgroup\$ – user13477176 Jul 18 '20 at 22:26
2
\$\begingroup\$

First of all you are doing great. This code is already above the average.

But of course it can be improved. The process is eternal, actually. So don't forget to post the next iteration as well :)

The first thing that catches my eye is inconsistent user level error reporting. Why some error messages are shown as is and some are lavishly decorated and sent through a session variable? I would stick with the former. There is no point a redirection. Just show all errors in place, then show the form and fill all the entered values in for the better usability.

Besides, PHP code should never contain any HTML. Imagine a designer would create a nifty icon to be shown next to the error message. Are you going to edit every occurrence of the error message in your code? All the decorations must be added at the output time, not at the definition. Whatever way you choose, remove all HTML from the error message. Add it at output

Another matter is redundant input treatment. Why both strip_tags() and htmlspecialchars()? Why empty() for a variable which is deliberately set? For the existing variable you can use just if (!$var). empty() should be used only if you want to know whether a variable is not set or has a falsey value.

Also, using $_POST when you already assigned the validated values to variables is flat out inconsistent.

htmlspecialchars() should be used on the output, not input.

Besides, for such a specific data as user information, you shouldn't sanitize it, but rather validate it instead, checking for the improper input and giving a warning. Hence, instead of sanitization, I would just assign input values to variables and then validate them

$first_name = $_POST['first_name'] ?? '', 
$last_name = $_POST['last_name'] ?? '', 
$email = $_POST['email'] ?? '';

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";
}

For email and password it is going to be a bit different, as for the password we don't want to verify the contents at all and for the email we have a distinct validation function

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";
}

On a side note, consider to get rid of the username. If you think of it, email just duplicates its functionality. And having two entities to serve for the same purpose (to identify a user) adds nothing but a confusion. Only if you are going to show usernames somewhere, it makes sense to keep them. But given there is a name, it makes sense to use it as a display name.

Another point is redundancy again. As you may have noticed, mysqli prepared statements are a little verbose, repeating the same prepare/bind execute over and over again.

Well, for practice, it's indeed a good thing to write some statements manually, just to get the feel, to see how it works. But for the real life application it looks redundant. Why not to write a simple function to encapsulate all the routine inside?

I wrote a mysqli helper function exactly for the purpose.

Now let's see how we can greatly reduce the amount of code.

if (!$errors) {
    $sql = "SELECT email FROM users WHERE email=?";
    $row = prepared_query($con, $sql, [$email])->get_result()->fetch_row();
    if ($row) {
        $errors[] = 'E-mail exists';
    }
}

Just like @mick said, the following line is absolutely redundant.

if ($row && $row['email'] == $_POST['email']) {

in reality, it could (and should) be shortened to just

 if ($row) {

because $row already serves as a flag. It is empty when no such email is found and not empty otherwise. No need for anything else.

The same function could be used for the insert too,

$sql = "INSERT INTO users (first_name, last_name, email, pw, friend_array)
        VALUES (?, ?, ?, ?, ?, ?)";
prepared_query($con, $sql, [$first_name, $last_name, $email, $pw, $friend_array]);

Don't you like how meaningful and tidy your code becomes at once?

For the convenience, you can put this function's definition in the same file with mysqli connection code

\$\endgroup\$
2
  • \$\begingroup\$ prepared_query Is the same a a normal mysqli prepared statement ? \$\endgroup\$ – user13477176 Jul 19 '20 at 22:48
  • \$\begingroup\$ Well, the name, the description that says "the same prepare/bind execute" and the code aren't convincing enough? \$\endgroup\$ – Your Common Sense Jul 20 '20 at 9:03

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.