You are checking a count of the result for no good reason, and have a try/catch on code that shouldn't need it.
I'd probably also use the array pass to ->execute instead of bindParam just because you're not doing POEM here.
You also don't "else" the result of the check for existing users, meaning that
You ALSO seem to be checking that the password matches, which would allow through the same username if they have the same PASSWORD.
It also seems a bit of a wonk that you're passing username/password to the function instead of handling it directly off $_POST. I'd have to see the form but with prepare/execute there's no reason to be creating "variables for nothing" much less passing them around on the stack!
... and if you have a function RETURN the result instead of slopping it around as a global.
I'd probably also \w+ on the username, skipping that ctype nonsense altogether.
A quick rewrite of the first two functions:
function register($username, $email, $password){
global $pdo;
if (preg_match('/\s/', $username))
return 'Spaces and other "whitespace" characters not allowed in user name';
if (!filter_var($email, FILTER_VALIDATE_EMAIL))
return 'Invalid E-Mail Address'
$stmt = $pdo->prepare('
SELECT count(*)
FROM signup
WHERE username = ?
');
$stmt->execute([$username]);
if ($stmt->fetchColumn())
return 'User already exists. Try again.';
// insert records into database
$hashed_password = password_hash($password,PASSWORD_DEFAULT);
$stmt = $pdo->prepare('
INSERT INTO signup (
username, email, password
) VALUES (
:username, :email, :password
)
');
$stmt->execute([
':username' => $username,
':email' => $email,
':password' => $hashed_password
]);
if ($stmt->rowCount()) return false; // false condition indicates success...
return 'Registration Failed'; // technically this should never happen
}
function login($username, $password){
global $pdo;
if (preg_match('/\s/', $username))
return 'Spaces and other "whitespace" characters not allowed in user name';
$stmt = $pdo->prepare('
SELECT *
FROM signup
WHERE username = ?
');
$stmt->execute([$username]);
if (
$data = $stmt->fetch(PDO::FETCH_ASSOC) &&
password_verify($password, $data['password'])
) {
header("Location:index.php");
return false; // false condition indicates success...
}
return 'Invalid username or password';
}
By returning a string on error and FALSE on success you have an easier way to check results without making a global, AND you have an instant way to exit out when you've met any single failure condition. The lack of exiting out once a result was set I suspect is a good chunk of where your logic was going bits-up face-down on you.
"False on success" is a very common technique in machine language, since false is also zero. That's why so many systems use 'zero' as the 'no error' return code. Then you can just:
if (!($errorMessage = login($username, $password))) {
// handle the error here
} else {
// we were successful
}
Remember you can check results on assignment, saving yourself a number of headaches and in some cases even cutting down on the 'variables for nothing'. (typically lowering your code's memory footprint in the process).
Note I also would NOT give any hint as to which (password or username) was invalid, as that could be used by hackers to probe the system. If you're going to reject on the grounds of either, you do not say which one failed.