2
\$\begingroup\$

As a self-taught student of Web-Development and Web Technologies, I created a simple sign in/out form in PHP, just for practice purposes and some fun!

I read the O'reilly's book about PHP and I found one sentence there (I really don't remember where it was though):

A novice web developer will program procedurally, but an advance one, will know that everything need to be handle as an object.

So it is. I never done an OOP programming before and I though that this will be an excellence motive to do so. In parallel I study about MVC model and I wanted to be sure that I have understand the OOP programming, so it can be used for MVC apps. Is my code clean, well-written to support a big scale app? What changes do I have to make?

The Classes

I developed two classes. One called Main and it used for procedures that need to be used in the hole app, like connected to DB, get user info and error handling procedures. The other called UserAuth holds the Login/Register/Logout methods as well as some other ones.

In the next paragraph I post the code from an index.php test page, and the other two files that hold the Main and UserAuth classes respectively.

The files

Index.php

<?php 
    session_start();
    require_once 'userAuth.class.php';

    $USER = new userAuth();

    # Check login state
    if( $USER->isLogin()) {
        echo $USER->showEmail();
    }


    # Login
    if (isset($_POST['login-button'])) {
        $login = $USER->Login($_POST['Lemail'], $_POST['LPass']);
        if ($login['State'] == true) {
            echo $USER->showEmail();
        }
        else {
            echo $login['Msg'];
        }
    }

    # Register
    if (isset($_POST['register-button'])) {
        $register = $USER->Register($_POST['Remail'], $_POST['RPass1'], $_POST['RPass2'], $_POST['g-recaptcha-response']);
        if ($register['State'] == true) {
            echo "We have register your email to our databases.";
        }
        else {
            echo $register['Msg'];
        }
    }

    # Logout
    if (isset($_POST['logout-button'])) {
        $USER->Logout();
    }
?>

<!DOCTYPE html>
<html>
<head>
    <title>Simple Sign In/Out</title>
</head>
<body>
    <form id="login" method="POST">     
        <input id="Lemail" name="Lemail" placeholder="Email" required="required" type="email" autocomplete="off">
        <input id="LPass" name="LPass" placeholder="Password" required="required" type="password" autocomplete="off">
        <button type="submit" id="login-button" name="login-button">Dive</button>
                
    </form>

    <form id="register" method="POST">
        <input id="Remail" name="Remail" placeholder="Your_Email@secretsea.com" required="required" type="text" autocomplete="off" >
        <input id="RPass1" name="RPass1" placeholder="Your Password" required="required" type="password" autocomplete="off">
        <input id="RPass2" name="RPass2" placeholder="Your Password" required="required" type="password" autocomplete="off">     
        <button type="submit" id="register-button" name="register-button">Register</button>
    </form>

    <form id="logout" method="POST">
        <button id="logout-button" name="logout-button">Logout</button>
    </form>
</body>
</html>

userAuth.class.php

<?php
    require_once 'main.class.php';
    require_once '../lib/bcrypt.php';

    class userAuth extends Main
    {

        private $L_UserEmail    =   NULL;
        private $L_UserPass     =   NULL;

        private $R_UserEmail    =   NULL;
        private $R_UserPass1    =   NULL;
        private $R_UserPass2    =   NULL;
        private $ReCaptcha      =   NULL;


        public function showEmail() {
            return $this->getUserEmail();
        }

        public function isLogin() {
            if($this->UserIsLogin()) {
                return true;
            }
            else {
                return false;
            }
        }


        private function UserIsLogin() {
            if (isset($_SESSION['login'])) {
                return true;
            }
            else {
                return false;
            }
        }


        public function Login($UserEmail, $UserPass) {
            $this->L_UserEmail = $UserEmail;
            $this->L_UserPass = $UserPass;

            if (!$this->exist_LoginEmail()) {
                return $this->setReturnState('User email does not exist in our databases.');
            }

            if (!$this->correct_LoginPass()) {
                return $this->setReturnState('Password is wrong.');
            }

            return $this->doLogin();
            
        }

        public function Logout(/*$emails*/) {
            $this->clearUserEmail();
            session_destroy();
        }

        public function Register($UserEmail, $UserPass1, $UserPass2, $ReCaptcha) {
            $this->R_UserEmail = $UserEmail;
            $this->R_UserPass1 = $UserPass1;
            $this->R_UserPass2 = $UserPass2;
            $this->ReCaptcha = $ReCaptcha;


            /*if (!$this->valid_Captcha()) {
                return $this->setReturnState('Please verify yourself as human.');
            }*/

            if (!$this->valid_RegisterEmail()) {
                return $this->setReturnState('Only letters and nubers are allowed for email. Please see <a href="#">security</a> for more details.');
            }

            if (!$this->valid_RegisterPass()) {
                return $this->setReturnState('Passwords do not match.');
            }

            if ($this->exist_RegisterEmail()) {
                return $this->setReturnState('Email already registered.');
            }

            return $this->doRegister();

        }



        private function exist_LoginEmail() {
            try {
                $STH = $this->DHB->prepare("SELECT UserEmail FROM Users WHERE UserEmail = :user_email");
                $STH->bindParam(':user_email', $this->L_UserEmail);
                $STH->execute();

                # Get the user info
                $row = $STH->fetchAll();

                # Check if username exist
                if (!$row) {
                    return false;
                } 
                else {
                    return true;
                }
            }
            catch(PDOException $e) {
                file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);
                return false;
            }
        }

        private function correct_LoginPass() {
            try {
                $STH = $this->DHB->prepare("SELECT UserPass FROM Users WHERE UserEmail = :user_email");
                $STH->bindParam(':user_email', $this->L_UserEmail);
                $STH->execute();

                # Get the user info
                $row = $STH->fetchAll();

                $isGood = password_verify($this->L_UserPass, $row[0]['UserPass']);

                # Check if password is good
                if ($isGood) {
                    return true;
                }
                else {
                    return false;
                }
            }
            catch(PDOException $e) {
                file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);
                return false;
            }
        }

        private function valid_Captcha() {
            # FIRST WE CHECK IF THE FORM WAS POSTED BY A HUMAN
            if ($this->ReCaptcha == NULL) {
                return false;
            }

            # HAS THE USER BEEN AUTHORIAZED BY GOOGLE ?
            $response = file_get_contents("https://www.google.com/recaptcha/api/siteverify?secret=6Lc6mxcTAAAAAABnITaUtxp3pbH_xUf8fEtj_f7p&response=".$this->ReCaptcha."&remoteip=".$_SERVER['REMOTE_ADDR']);
            if($response.success == false) {
                return false;
            }

            return true;
        }

        private function valid_RegisterEmail() {
            # CHECK IF EMAIL CONSISTS ONLY a-z A-Z 0-9 characters
            if (!ctype_alnum($this->R_UserEmail)) {
                return false;
            }

            return true;
        }

        private function valid_RegisterPass() {
            if ($this->R_UserPass1 == $this->R_UserPass2) {
                return true;
            }

            return false;
        }

        private function exist_RegisterEmail() {
            $this->R_UserEmail .= '@secretsea.com';
            try {
                # STH means "Statement Handle"
                $STH = $this->DHB->prepare("SELECT * FROM Users WHERE UserEmail = :user_email");
                $STH->bindParam(':user_email', $this->R_UserEmail);
                $STH->execute();
                
                if($STH->rowCount() <= 0) {  # Check if username is already registered
                    # Email has not registered yet
                    return false;
                }
                
                return true;
                
            }
            catch(PDOException $e) {
                file_put_contents('../lib/PDOErrors.txt', $e->getMessage(), FILE_APPEND);
                return false;
            }
        }

        private function doRegister() {
            $hashedPassword = password_hash($this->R_UserPass1, PASSWORD_BCRYPT, array("cost" => 13));                

            $STH = $this->DHB->prepare("INSERT INTO Users(UserEmail, UserPass) values(:user_email, :user_pass)");
            $STH->bindParam(':user_email', $this->R_UserEmail);
            $STH->bindParam(':user_pass', $hashedPassword);
            $STH->execute(); 

            if (!$STH) {
                return $this->setReturnState('We could not process your order. Please try again later.');
            }

            return $this->setReturnState(null, true);
        }

        private function doLogin() {
            $this->setUserEmail($this->L_UserEmail);
            return $this->setReturnState(null, true);
        }

        private function setUserEmail($email) {
            $_SESSION['login'] = $email;
        }

        private function clearUserEmail() {
            unset($_SESSION['login']); 
        }

    }   # End of User Auth Class
?>

main.class.php

<?php 

    # The symbolic constants of databse connection
    require_once '../lib/config.php';

    class Main
    {
        protected $DHB  =   NULL;

        function __construct()
        {
            try {
                # DHB : Database Handle
                $this->DHB = new PDO("mysql:host=".DB_HOST.";dbname=".DB_DATABSE, DB_USER, DB_PASSWORD);
                $this->DHB->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
            }
            catch(PDOException $e) {
                file_put_contents('../PDOErrors.txt', $e->getMessage(), FILE_APPEND);
            }
        }

        protected function getUserEmail() {
            return $_SESSION['login'];
        }

        protected function setReturnState($msg, $state = false) {
            return array('State' => $state, 'Msg' => $msg);
        }

    }
?>
\$\endgroup\$
1
\$\begingroup\$

Single Responsibility Principle

I think your code does too much. Try split it into classes, that will focus on doing one thing, for example:

  • User class, that will handle all user variables.
  • Authentication class, that will handle all Database queries.

Class called Main also does too many things. It will connect to database, return $_SESSION variable and set return state. It has no clue. You can create:

  • Session class, that will handle your session variables.
  • Database handler class, that will connect and send queries to your database.
  • Application class, that will do all the global jobs like set return state.

PHP Standards Recommendations

PSR is something that will make your work more cleaner and readable. It is composed of basic code standars. There are some links, get familiar with them!

PHP Standards Recommendations

Github Accepted Fig-standards

Try to use one MySQL query

Why are you using multiple MySQL queries where you have the same statement?? Look at this login function:

public function Login($UserEmail, $UserPass) 
{
    $this->L_UserEmail = $UserEmail;
    $this->L_UserPass = $UserPass;

    try {
        # One statement to rule them all
        $STH = $this->DHB->prepare("SELECT UserEmail FROM Users WHERE UserEmail = :user_email");
        $STH->bindParam(':user_email', $this->L_UserEmail);
        $STH->execute();

        # Get the user info
        $row = $STH->fetchAll();

        # Check if username exist
        if (!$row) {
            return $this->setReturnState('User email does not exist in our databases.');

        # Verify password
        if (!password_verify($this->L_UserPass, $row[0]['UserPass'])) 
            return $this->setReturnState('Password is wrong.');

        # Happy end
        $this->setUserEmail($this->L_UserEmail);
        return $this->setReturnState(null, true);
    } catch(PDOException $e) {
        file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);
        return false;    
    }

}

It's better, cleaner, more readable and, first of all, much much faster! Think about 10 tousand visitors on your site. It will be 20 tousands queries!

Also..

  • I recommend you to read about SOLID principles. Grasp SOLID!
  • Read more about the rules of OOP in PHP.
  • Good luck!
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy