Skip to main content
1 of 3
Roland Illig
  • 21.4k
  • 2
  • 34
  • 83

First of all, your code looks rock-solid, obvious in what it does and very clean. This is not something that I would expect from someone "new in development". Whoever you learned all this from, you should keep them.

You got all of the major topics of a login system right:

  • Your code uses parameterized statements for all database access, to protect against SQL injections.
  • Your code saves passwords only in their hashed form, to protect against data breaches.
  • You use a decent password hashing algorithm. Good that PHP provides a sane default here and makes it easy to use. Throughout its long history, PHP had not always made it easy to write correct and secure code. For password hashing it succeeded to provide a good API, and you use it perfectly as well.
  • You use the full available character set from Unicode by connecting to the MySQL database using the utf8mb4 encoding. This allows emojis in user names 🙂 to be saved correctly.
  • All HTML that is printed carefully avoids to reflect any user-provided data. This prevents cross-site scripting.

Next, I'm looking at the details of the code, from top to bottom, see if there are any unnecessary stylistic variations.

database.php

For me, the comment on the DatabaseConnection is redundant. The name of the class already expresses clearly that the DatabaseConnection connects to the database. Therefore I would not need that comment. Since you are still new in development, it's perfectly fine to keep that comment until you (and whoever else works on your project) don't need it anymore because it's obvious.

Same for the will be inherited part in the comment. My IDE lets me quickly see the type hierarchy of classes, which makes this comment redundant as well. Plus, will be refers to the future and is probably outdated by now.

The constructor of DatabaseConnection initializes the field $this->database, but when looking at the class DatabaseConnection alone, that field is not used anywhere. This feels strange at first. I'm somehow missing the private $database declaration, like you wrote it in the class SignUp.

Still in the constructor of DatabaseConnection, you wrote try{ without a space in between. It is more common to write try { with a space in between, just like there is a space after if and catch. If you are using an IDE, it has a command to "format the source code", so you don't have to do that yourself. There are many different styles of formatting the code, it doesn't matter which one you choose. Just choose one that you like and apply it consistently to all of the code.

Still in the constructor of DatabaseConnection, I don't see a reason for the try catch block at all. If you remove that block and only keep the single line $this->database = ... instead, the original stack trace will be preserved, including all information about the actual cause of the exception. By wrapping the PDOException in a plain Exception and only copying the message, you give away this context information.

registration.php

The comment This is a registration script is redundant. Keeping the author information is good if you plan on passing the project to someone else later, so that they have someone to contact. Whether or not the copyright 2021 is necessary or helpful is something I cannot decide for you.

Declaring strict_types=1 is good style, you should add it to database.php as well.

The class SignUp has no comment above it. This leaves a reader a bit unclear about what a SignUp really is. It might be "all data that is collected on the sign-up page", or it might be "the data that is actually required to perform a sign-up operation". The difference between these two is that the former would include the password confirmation field, but the latter would not.

You declare SignUp extends DatabaseConnection, which sounds strange. It's not that "every sign-up is a specialized database connection", but that's what the keyword extends usually means. Instead, it's more likely that the class SignUpPage would use a database connection. Therefore I would drop the extends DatabaseConnection from the class SignUp.

It's a nice trick that by declaring private $database in the class SignUp, you make that previously undeclared field of the class DatabaseConnection visible. Having tricks like these in the code may be confusing for readers, or they may even expect them. Instead of this trick, I would rather declare a field private $database of type DatabaseConnection and initialize this field in the constructor.

I like the field name $passwordHash since it makes it obvious that the password is never stored in its plain text form. I hope that you use that field in exactly this way, I haven't looked at the actual code yet. But even if at some place there were some code that read $passwordHash = $_POST['password'], this would immediately alert any careful reader that something is wrong here, which is a good second-line defense.

I'm not sure what the purpose of the field $signUpPage is. It could be a complete URL like https://localhost:3000/signup, it could be the HTML that makes up the sign-up page, or it could be the simple word signup, as in your code. A more precise name would be $relativeSignUpPageUrl, but that name is rather long, so it's understandable you chose a shorter name. In that case it might help the reader to add a small comment describing an example value for the variable. From this example value it is usually simple enough to infer all allowed values.

The function setName has the comment Assign the username. First, when I let my IDE render that comment, it looks like this:

Screenshot of rendered comment

The word Assign is listed in the section of the parameter $name while it really describes the function, not the parameter. The comment should rather be:

    /**
     * Assign the username
     * @param string $name
     */

If you were to rename the function to setUsername, each word of the comment would be redundant, allowing you to leave out the comment completely.

In the function setPassword, the comment contains the words & $confirm. The & should be written as & since the comments contain HTML, just like Javadoc (different programming language, same idea). When you search for @param in that document, you will find in match 16/36 that each parameter has a @param of its own. It's the same for PHP.

In the comment for signUpPage, I don't see a need to write Page with a capital P. Same for returnWithError.

The comment for existingName says "Checks whether a name exists". The return type is void, which is unexpected to me. I would have expected bool here since that's the correct type for "whether". Instead, the function exits if the name does not exist. Therefore I would rename that function to checkNameExists. Function names starting with check usually behave this way, throwing either an exception on failure or exiting plainly.

In the function ValidateNameLength you started the name with a capital V, that is unusual. The other function names start with a lowercase letter.

In ValidateNameLength you check the length of the name using mb_strlen. This is very careful, many other programmers would have used the wrong strlen instead.

In addUser, you use a \w in the regular expression. The documentation for the \w was a bit hard to find since it is not directly referenced in the documentation of preg_match, but anyways. The \w includes all "word" characters, even outside [A-Za-z]. This may include Cyrillic, Hangul and several other alphabets. If you want to avoid usernames that look equal but are not (the Latin a and the Cyrillic а), you should rather restrict the allowed characters to [A-Za-z] first. You can always extend the character set later, restricting it is more work since it might affect existing usernames.

Still in addUser, the SQL statement currently depends on the order of the column names in the database. If the order of the columns should change in the future, your code will put the data in the wrong columns. To prevent this, you can list the column names explicitly, as in INSERT INTO users(name, email, password_hash, remote_addr, client_ip, forwarded_for, user_agent). This change, of course, will make your code dependent on the names of the columns instead of their position. Choose either way, I'm not sure which change is more likely to happen.

main.php

The call to new SignUp with the database connection parameters looks strange. This is because a Page object should not need to know these connection parameters as individual values, it should rather only have a single parameter of type DatabaseConnection. Assume that the database connection needs more more parameter, for whatever reason. Then you would have to adjust the call to new SignUp, and that feels wrong because a page should be independent from this low-level change.

Summary

Even after these many detailed remarks, the code still looks very solid to me. The remarks can be fixed easily, and the overall structure of the code is very nice and easy to understand. Very well done.

Roland Illig
  • 21.4k
  • 2
  • 34
  • 83