Here, I just spent a few moments to tidy up and rewrite what you had so far closer to how I'd approach it. This is raw untested code probably riddled with simple typo's -- I'm fighting off a summer cold here -- but should be enough to get you on the path.
I put both as .source files here:
cutcodedown.com/for_others/Emmanuel_Obeng
My "for others" directory is filled with snippets dating back 15 years of me helping people on various forums, feel free to look around; I always leave that directory unlocked. (and don't worry, it's behind a robots.txt so it won't steal your mojo)
I would NOT have the register or login functions in a common functions.php since that would mean you're wasting time (and memory) including them even when you're not using them. Library files are handy and powerful when you have stuff called from multiple spots, but they can incur a performance hit in interpreted / scripting languages when said code is not being used. Good idea, bad implementation/choices of what goes into it.
I added session handling to it so a generated hash can be attached to each form. This way 1) bots trying to BS the form will have a harder time of it, 2) by invalidating the hash upon use, it is impossible for someone to hit "back" and re-use the same form over again. This is why my sites don't have to screw around with header redirects on successful login/register -- but then I use "one index to rule them all" site construction.
Unfolding it so that the logic is inline might make some of the MVC types poo-poo the concept for having logic next to the markup -- but that's why if I were to make separate includes it would be for the output / template stuff, NOT the logic.
To that end -- well, lets' use the login.php as an example -- rather than doing the echo inline in production code I'd do this:
include(
$error ?
'template/login_error.template.php' :
'template/login_success.template.php'
);
Keeping in mind that PHP is a perfectly good template engine unto itself, moving that markup into those files. Said files could also then call something like "template/common_header.template.php" at their start to output the doctype/head/template heading and "template/common_footer.template.php" for the footer markup.
Though in my way of doing things I have a common.template.php file that has template_header() and template_footer() functions in it, but I also practice the "no include file should ever output anything if directly called" methodology drilled into my head.
Oh, I also raised the headings to H2 depth (I very much doubt you had H3 or H2 for these forms to be subsections of) and added a proper FIELDSET to say "yes, the user can edit the values in these" fixing a minor accessibility issue.
Remember, HTML is for saying what things ARE structurally, grammatically, and syntactically. It is NOT for saying what things look like. ...so if you chose that H4 for the font-size/weight or you omitted the fieldset because you don't like the border and padding, you're doing it all wrong.
Sadly a concept rarely even taught anymore as everything in HTML gets dragged back to the worst of pre-strict practices. It's like the folks pimping HTML 5 and stuff like frameworks the loudest really do miss the worst of browser wars HTML 3.2 and 1990's methodologies. That SO many online tutorials are outright web-rot isn't helping.
Jason Knight
The less code you use, the less there is to break
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.