Ttykuu

Could someone tell me if this is a bad idea for coding my validation for a password and it is bad? This is for registration for my site. Could you please tell me why and how you would go about it (I am a beginner and at PHP)?

$username = $_POST['username'];
$password = $_POST['password'];
$confirm_password = $_POST['confirm_password'];

$password_filter1 = '/(?=.*[a-zA-Z0-9]6)/m'; //Password contains more then 6 char
$password_filter2 = '/(?=.*[A-Z])/m'; //Capital letter
$password_filter3 = '/(?=.*d)/m'; //A digit

if (!empty($password & $confirm_password)) 
 if ($password === $confirm_password) 
 if (preg_match($password_filter1, $password)) 
 if (preg_match($password_filter2, $password)) 
 if (preg_match($password_filter3, $password)) 
 //If everything is true do this
 else 
 $password_error = "Your password must contain at least 1 number";
 
 else 
 $password_error = "You're password must have at least 1 capital letter";
 
 else 
 $password_error = "You're password must be at least 6 characters long";
 
 else 
 $password_error = "The passwords do not match";
 
 else 
 $password_error = "You must enter a password";

The only reason why I am not doing all the filters in 1 preg_match is because I would like specific errors.

First a stylistic note: you should always use at least 2 spaces of indentation for code; 1 space makes it very difficult to read nested statements in particular. PHP most often uses 4 spaces for indent.

Your regular expressions wouldn't need comments to explain them if you gave them meaningful names.

$password_filter1 = '/(?=.*[a-zA-Z0-9]6)/m'; //Password contains more then 6 char
$password_filter2 = '/(?=.*[A-Z])/m'; //Capital letter
$password_filter3 = '/(?=.*d)/m'; //A digit

There is also no need for m multiline flag for a password, since passwords shouldn't accept any newline characters.

Compare to:

$has_at_least_6_chars = '/(?=.*[a-zA-Z0-9]6)/';
$has_a_capital_letter = '/(?=.*[A-Z])/'; 
$has_a_digit = '/(?=.*d)/'; 

You may also consider making them into small functions so they are reusable elsewhere in the code, and if you needed to change them later you would just change them in one place.

function has_a_capital_letter($str) 
 return preg_match('/(?=.*[A-Z])/', $str);


//...

if (!has_a_capital_letter($password)) 
 $password_error = "Your password must have at least 1 capital letter";

There is no real value in nesting multiple if/else statements inside each other like this, when a simple sequence of if/elseif/else will work just as good. Nesting like that with even simple conditions makes the code quite hard to read, in fact. Nesting like this with more complex operations could lead to serious performance bottlenecks as it cannot break early as easily.

It's also simpler to test for the existence of conditions instead of the lack of them, and just break out of the conditionals as soon as a condition is met.

This is much simpler and cleaner:

if (empty($password & $confirm_password)) 
 $password_error = "You must enter a password";

elseif ($password !== $confirm_password) 
 $password_error = "The passwords do not match";

elseif (!preg_match($has_at_least_6_chars, $password)) 
 $password_error = "Your password must be at least 6 characters long";

elseif (!preg_match($has_a_capital_letter, $password)) 
 $password_error = "Your password must have at least 1 capital letter";

elseif (!preg_match($has_a_digit, $password)) 
 $password_error = "Your password must contain at least 1 number";

else 
 //If everything is true do this

By the way, I think there is a problem with $has_at_least_6_chars = '/(?=.*[a-zA-Z0-9]6)/m' in that it doesn't truly check for 6 characters, it checks for 6 letters and/or numbers. According to the way your error messages are written, this should be a perfectly valid entry meeting all conditions:

$password = "Hell0!";
$confirm_password = "Hell0!";

Yet your code returns "Your password must be at least 6 characters long" (which of course it is, except that your regular expression removes the ! and all other non-alphanumeric characters). It would be much simpler (and more accurate) to use the built-in strlen() PHP function.

elseif (strlen($password) < 6) 
 $password_error = "Your password must be at least 6 characters long";

That is, unless you have requirements that forbid non-alphanumeric characters, which I think would be a terrible idea, since many people use them to make for stronger passwords that are harder to break through brute force.

If you split things out into (ideally) classes or at least functions, you can have cleaner code as well as reuse things on different pages or in different functions or classes. For example see the refactor of your if/else checking for a valid password:

While this code should work, it's not tested, so please don't use in production without usual considerations and testing.

function getPasswordError($password, $confirm_password)

 $password_filter1 = '/(?=.*[A-Z])/m';
 $password_filter2 = '/(?=.*d)/m'; 

 if (empty($password) 

You can check it such as:

$password_error = getPasswordError($password, $confirm_password);

if ($password_error !== false) 
 echo $password_error; // or do whatever with the data
 

The thing about that function is:

1. Cleaner, easier to read and see what errors would be issued


2. Easier to add, remove, or alter conditions


3. Reusable - if in a separate file somewhere then the function can be used in other places (eg the login form and registration as both need the same validation)


4. Return early (as soon as something fails etc go back)

Note: That I've put your filters inside the function. You could pass them in, and will in some functions to have control, but likely you wouldn't want to ever have different criteria for those filters. That would (likely) be a different function for a different purpose (even if the same conditions).

Also the function could be better named, but given your other code there's not much to do. E.g. I would have a function that determined if valid or not and just returned bools, then if something failed then issue a specific message.