2
\$\begingroup\$

I wrote this class a few months ago and noticed from a few examples that it's better to break down these classes and separate them. I am not so sure what is the proper way to break it into parts.

It currently includes a creation of a System_user obj based on user id (fetching user data), login validation, logout, storing user data to session (more specifically CSRF), and I think that's all.

<?php
namespace MyApp\Models;

use \Exception;
use MyApp\Core\Database;
use MyApp\Core\Config;
use MyApp\Helpers\Session;
use MyApp\Helpers\Cookie;
use MyApp\Helpers\Token;
use MyApp\Helpers\General;
use MyApp\Helpers\Hash;



/**
 *
 *  System User Class
 *
 */
class System_user
{

/*=================================
=            Variables            =
=================================*/

    # @object database Database instance 
    private $db;

    # Users data
    private $data;

    # User user ID name
    public $user_id;

    # User first name
    public $first_name;

    # User last name
    public $last_name;

    # Username
    public $user_name;

    # User Email  
    public $email;

    # User Last logged in  
    public $last_login;

    # is user logged in
    public $isLoggedIn;

    # is user logged in
    public $login_timestamp;

    # is user IP
    private $user_ip;


/*===============================
=            Methods            =
================================*/

    /**
     *
     *  Construct
     *
     */
    public function __construct($system_user = NULL)
    {
        # Get database instance
        $this->db           = Database::getInstance();

        # If system_user isn't passed as a variable 
        if ( !$system_user ) {

            # ...so check if there is a session user id set 
            if (Session::exists(Config::$session_name)) {

                # Insert session data to system_user variable
                $system_user = Session::get(Config::$session_name);

                # Get user data
                $this->find($system_user);
            }

        } else {
            $this->find($system_user);
        }
    }


    /**
     *
     *  Find method: Find user by id or by username 
     *  @param      $user   String/Init     A username or user ID
     *
     */
    public function find($system_user = NULL)
    {
        if ($system_user) {

            // Enable search for a system_user by a string name or if numeric - so by id. 
            $field  = ( is_numeric($system_user) ) ? 'system_user_id' : 'uname';

            // Search for the system_user in the Database 'system_users' table. 
            $data   = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $system_user));

            // If there is a result
            if ( $data ) {
                // Set data 
                $this->setUserData($data);

                return $this;
            } else {
                return false;
            }
        }
        else{
            return false;
        }
    }


    /**
     *
     *  Check if user exist in 'system_users' table
     *  @param      $username       String              Get a username user input
     *  @param      $password       String              Get a password user input
     *  @throws                     Array/Boolian       Is this a signed System user?
     *
     */
    private function system_user_login_validation($username, $password)
    {
        $user_data = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));

        if ($user_data)
            return $user_data; 
        else
            return false; 
    }


    /**
     *
     *  Login method
     *  @param      $customer_name  String      Get a customer_name user input
     *  @param      $username       String      Get a username user input
     *  @param      $password       String      Get a password user input
     *  @throws                     Boolian     Is this a signed System user?
     *
     */
    public function login($customer_name, $username, $password)
    {

        # Create a Customer Obj
        $customer = new \MyApp\Models\Customer($customer_name);

        try {
            # Check if the result is an array
            # OR there is no row result: 
            if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
                throw new \MyApp\Core\Exception\Handler\LoginException("Bad company name: {$customer_name}");

           # Change localhost string to 127.0.0.1 (prevent dns lookup)
           $customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;

            # Connect to new database
            $new_connection = $this->db->customer_connect($customer->host, $customer->dbName);

            # If status is connected 
            if ($new_connection) {

                # Check for user credentials data 
                $user_data = $this->system_user_login_validation($username, $password); 

                # If the result isn't a valid array - EXEPTION  
                if ( (!is_array($user_data)) || (empty($user_data)) )
                    throw new \MyApp\Core\Exception\Handler\LoginException("Customer: '{$customer_name}' - Invalid username ({$username}) or password ({$password})");

                # Store Customer in the sesison
                Session::put(Config::$customer, serialize($customer));

                # Update host and db for the db object
                # $this->db->update_host_and_db($customer->host, $customer->dbName);

                # Set data for this System_user object
                $this->setUserData($user_data);

                # Set a login session for the user id: 
                Session::put(Config::$session_name, $this->user_id);

                # Set logged in user sessions
                $this->set_loggedin_user_sessions();

                return $this;

            } else {
                # Connect back to backoffice (current db set)
                $this->db->connect_to_current_set_db();
                throw new \MyApp\Core\Exception\Handler\LoginException('User does not exist');
                return false;
            }

        } catch (\MyApp\Core\Exception\Handler\LoginException $e) {
            $e->log($e);
            return false;
            // die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
        }
    }


    /**
     *
     *  Set sessions for the logged in user. 
     *  Tutorial: http://forums.devshed.com/php-faqs-stickies/953373-php-sessions-secure-post2921620.html
     *  
     */
    public function set_loggedin_user_sessions()
    {
        # Generate security sessions
        $this->generate_security_sessions();

        # Set login timestamp 
        Session::put(Config::$login_timestamp, $this->login_timestamp);

        # Set login flag to true
        Session::put(Config::$is_logged_in, true);

        # Set login IP 
        Session::put(Config::$login_user_ip, $this->user_ip);
    }


    /**
     *
     *  Generate system user security sessions
     *  @param      $new_session        Boolean         (optinal) Dedices if to delete the cookie session id [default is set to true]
     *
     */
    public function generate_security_sessions($new_session = true)
    {
        if ($new_session)
            # Generate a new session ID
            session_regenerate_id(true);

        # Fetch cookie session ID 
        $session_id = session_id();
        # Set the session id to the session
        Session::put(Config::$session_id, $session_id);

        # Create a secret token 
        # Set it in session (does them both)
        $secret     = Token::generate_login_token();

        # Combine secret and session_id and create a hash
        $combined   = Hash::make_from_array(array($secret, $session_id, $this->user_ip));
        # Add combined to session
        Session::put(Config::$combined, $combined);
    }


    /**
     *
     *  Check if there is a logged in user
     *
     */
    public function check_logged_in()
    {
        if ( Session::exists(Config::$secret)       &&  # Secret session exists
             Session::exists(Config::$session_id)   &&  # Session_id session exists
             Session::exists(Config::$session_name) &&  # User session exists 
             Session::exists(Config::$is_logged_in) &&  # Check if 'logged in' session exists
             Session::exists(Config::$session_name)     # Check if sys_user id is set in session
            )
        {
            # Get users ip
            $ip = $this->get_system_user_ip();

            # if the saved bombined session 
            if ( 
                 (Session::get(Config::$combined) === Hash::make_from_array(array(Session::get(Config::$secret), session_id()), $ip)) && 
                 (Session::get(Config::$is_logged_in) === true ) 
                )
            {
                # Set ip to system user object
                $this->user_ip = $ip;

                return true;

            } else {
                return false;
            }
        }
        else {
            return false; 
        }
    }


    /**
     *
     *  Check if loggin session is timeout
     *
     */
    public function check_timeout()
    {
        if (Session::exists(Config::$login_timestamp)){

            # Calculate time 
            $session_lifetime_seconds = time() - Session::get(Config::$login_timestamp) ; 

            if ($session_lifetime_seconds > Config::MAX_TIME){
                $this->logout();
                return true;
            } else {
                return false;
            }

        } else {
            $this->logout();
            return false;
        }
    }


    /**
     *
     *  Get user IP 
     *
     */
    private function get_system_user_ip()
    {
        if (!empty($_SERVER['HTTP_CLIENT_IP']))
            $ip = $_SERVER['HTTP_CLIENT_IP'];
        elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) 
            $ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
        else
            $ip = $_SERVER['REMOTE_ADDR'];

        return $ip;
    }


    /**
     *
     *  Set User data to (this) System_user object
     *  @param      $user_data      Array   User data fetched from the db (usually by the find method)
     *
     */
    private function setUserData($user_data) 
    {
        // Set data for this user object
        $this->user_id              = $user_data['system_user_id'];
        $this->first_name           = $user_data['fname'];
        $this->last_name            = $user_data['lname'];
        $this->user_name            = $user_data['uname'];
        $this->email                = $user_data['email'];
        $this->last_login           = $user_data['last_login'];

        $this->isLoggedIn           = true;
        $this->user_ip              = $this->get_system_user_ip();
        $this->login_timestamp      = time();
    }


    /**
     *
     *  Logout: Now guess what this method does.. 
     *
     */
    public function logout()
    {
        $this->isLoggedIn   = false;
        Cookie::eat_cookies();
        Session::kill_session();
        session_destroy();
        session_write_close();
    }

}

I would like to get suggestions about my current code, and if possible, about structuring it differently with more than one class. (class SystemUser, class systemUserLogin, class systemUserAuthenticator, ect')

In general, the webapp by default logs in to a general database. When a user inserts their company_name, username and password, I check if the company name actually exists, and if it does, I disconnect from the general db and connect to the customer's database and validate their username and password.

This is the new class I started writing (not tested, so I can't assure this is working code) with more classes following this example and inspired by this post I found, while trying to follow the SOLID principals and PSR standards, focusing on the structure and architecture.

<?php
namespace MyApp\Models;

use MyApp\Core\Config;
use MyApp\Helpers\Session;
use MyApp\Core\Database;


/**
 *
 *  System User Class
 *
 */
class SystemUser
{

/*=================================
=            Variables            =
=================================*/

    # @obj SystemUser profile information (fullname, profile picture... etc')
    protected $systemUserDetatils;
    # @obj SystemUser Login data
    protected $systemUserLogin;
    # @obj SystemUser Authenticator
    protected $systemUserAuthenticator;


/*===============================
=            Methods            =
================================*/


    /**
     *
     *  Construct
     *
     */
    public function __construct($systemUserId = NULL)
    {
        # If system_user passed
        if ( $systemUserId ) {

            # Create systemUserDedatils obj
            $this->systemUserDetatils = new MyApp\Models\SystemUser\SystemUserDetatils();

            # Get SysUser data
            $this->systemUserDetatils->get($systemUserId);

        } else {

            # Check for sysUser id in the session:
            $systemUserId = $this->systemUserDetatils->getUserFromSession();

            # Get user data from session 
            if ( $systemUserId ) {

                # Create systemUserDedatils obj
                $this->systemUserDetatils = new MyApp\Models\SystemUser\SystemUserDetatils();

                # Get SysUser data
                $this->systemUserDetatils->get($systemUserId);
            }
        }
    }


    /**
     *
     *  Set Login: Sets the SystemUserLogin object to $systemUserLogin variable 
     *  @param     $_systemUserLogin   SystemUserLogin     Gets a SystemUserLogin object
     *
     */
    public function setSystemUserLogin(SystemUserLogin $_systemUserLogin)
    {
        $this->systemUserLogin = $_systemUserLogin; 
    }


    /**
     *
     *  Login 
     *
     */
    public function login()
    {
        $this->systemUserAuthenticator($this);
    }


}








<?php 
namespace MyApp\Models\SystemUser;

use MyApp\Core\Config;
use MyApp\Helpers\Session;

/**
 *
 *  System User Details Class
 *
 */
class SystemUserDetails 
{

/*=================================
=            Variables            =
=================================*/

    # @object database Database instance 
    private $db;

    # Users data
    private $data;

    # User user ID name
    public $userId;

    # User first name
    public $firstName;

    # User last name
    public $lastName;

    # Username
    public $userName;

    # User Email  
    public $email;

    # User Last logged in  
    public $lastLogin;

    /*# is user logged in
    public $isLoggedIn;

    # is user logged in
    public $login_timestamp;*/

    # is user IP
    private $user_ip;


/*===============================
=            Methods            =
================================*/

    /**
     *
     *  Construct
     *
     */
    public function __construct()
    {
        # Get database instance
        $this->db           = Database::getInstance();
    }


    /**
     *
     *  Find method: Find user by id or by username 
     *  @param      $user   String / Init     A username or user ID
     *  @return 
     *
     */
    public function get(Int $systemUserId)
    {
        if ($systemUserId) {

            # Enable search for a system_user by a string name or if numeric - so by id. 
            $field  = ( is_numeric($systemUserId) ) ? 'system_user_id' : 'uname';

            # Search for the system_user in the Database 'system_users' table. 
            $data   = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE {$field} = :sys_user", array('sys_user' => $systemUserId));

            # If there is a result
            if ( $data ) {

                # Set data 
                $this->setUserData($data);

                return $this;
            } else {
                return false;
            }
        }
        else {
            return false;
        }
    }


    /**
     *
     *  Set User data to $this obj
     *  @param      $userData       Array   User data fetched from the db (usually by the find method)
     *  @return  
     *
     */
    public function set(Array $userData)
    {
        // Set data for this user object
        $this->userId               = $userData['system_user_id'];
        $this->firstName            = $userData['fname'];
        $this->lastName             = $userData['lname'];
        $this->userName             = $userData['uname'];
        $this->email                = $userData['email'];
        $this->lastLogin            = $userData['last_login'];
    }


    /**
     *
     *  Get User from session
     *  @param 
     *  @return 
     *
     */
    public function getUserFromSession()
    {
        # Check if there is a session user id set 
        if (Session::exists(Config::$session_name)) {

            # Insert session data to system_user variable
            return Session::get(Config::$session_name);

        } else {
            # Returning false cause there is no user id session
            return false; 
        }
    }
}





<?php 
namespace MyApp\Models\SystemUser;


/**
 *
 *  System User Details Class
 *
 */
class systemUserLogin 
{

/*=================================
=            Variables            =
=================================*/

    # @str Customer name 
    public $customerName;

    # @str UserName
    public $userName;

    # @str Password
    public $password;

    # @str user IP
    public $userIp;


/*===============================
=            Methods            =
================================*/


    /**
     *
     *  Construct - Set customer, username and password
     *  @param      $_customerName      String
     *  @param      $_userName          String
     *  @param      $_password          String
     *
     */
    public function __construct(String $_customerName, String $_userName, String $_password)
    {
        $this->customerName = $_customerName;
        $this->userName     = $_userName;
        $this->password     = $_password;
        $this->userIp       = $this->getSystemUserIp();
    }


    /**
     *
     *  Get user IP 
     *  @return     String      Returns the user IP that is trying to connect. 
     *
     */
    private function getSystemUserIp()
    {
        if (!empty($_SERVER['HTTP_CLIENT_IP']))
            $ip = $_SERVER['HTTP_CLIENT_IP'];
        elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) 
            $ip = $_SERVER['HTTP_X_FORWARDED_FOR'];
        else
            $ip = $_SERVER['REMOTE_ADDR'];

        return $ip;
    }

}





<?php 
namespace MyApp\Models\SystemUser;


/**
 *
 *  System User Details Class
 *
 */
class systemUserAuthenticator 
{

/*=================================
=            Variables            =
=================================*/

    # @object Database instance 
    private $db;

    # @bool Is logged in
    public $isLoggedIn = false;

    # @str Login Timestamp 
    public $loginTimestamp;


/*===============================
=            Methods            =
================================*/


    /**
     *
     *  Construct
     *
     */
    public function __construct()
    {
        # Get database instance
        $this->db           = Database::getInstance();
    }


    /**
     *
     *  Login method
     *  @param      $customer_name  String      Get a customer_name user input
     *  @param      $username       String      Get a username user input
     *  @param      $password       String      Get a password user input
     *  @throws                     Boolian     Is this a signed System user?
     *
     */
    public function login(User $user)
    {
        # Create a Customer Obj
        $customer = new \MyApp\Models\Customer($user->SystemUserLogin->customerName);

        try {
            # Check if the result is an array
            # OR there is no row result: 
            if ( (!isset($customer)) || (!isset($customer->dbName)) || (!isset($customer->host)) )
                throw new \MyApp\Core\Exception\Handler\LoginException("Bad company name: {$user->SystemUserLogin->customerName}");

           # Change localhost string to 127.0.0.1 (prevent dns lookup)
           $customer->host = ($customer->host === 'localhost') ? '127.0.0.1' : $customer->host;

            # Connect to new database
            $new_connection = $this->db->customer_connect($customer->host, $customer->dbName);

            # If status is connected 
            if ($new_connection) {

                # Check for user credentials data 
                $user_data = $this->system_user_login_validation($user->SystemUserLogin->userName, $user->SystemUserLogin->password); 

                # If the result isn't a valid array - EXEPTION  
                if ( (!is_array($user_data)) || (empty($user_data)) )
                    throw new \MyApp\Core\Exception\Handler\LoginException("Customer: '{$user->SystemUserLogin->customerName}' - Invalid username ({$user->SystemUserLogin->userName}) or password ({$user->SystemUserLogin->password})");

                # Store Customer in the sesison
                Session::put(Config::$customer, serialize($customer));

                # Update host and db for the db object
                # $this->db->update_host_and_db($customer->host, $customer->dbName);

                # Set data for this System_user object
                $this->setUserData($user_data);

                # Set a login session for the user id: 
                Session::put(Config::$session_name, $this->user_id);

                # Set logged in user sessions
                $this->set_loggedin_user_sessions();

                return $this;

            } else {
                # Connect back to backoffice (current db set)
                $this->db->connect_to_current_set_db();
                throw new \MyApp\Core\Exception\Handler\LoginException('User does not exist');
                return false;
            }

        } catch (\MyApp\Core\Exception\Handler\LoginException $e) {
            $e->log($e);
            return false;
            // die(General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' )));
        }
    }


    /**
     *
     *  Check if user exist in 'system_users' table
     *  @param      $username       String              Get a username user input
     *  @param      $password       String              Get a password user input
     *  @throws                     Array/Boolian       Is this a signed System user?
     *
     */
    private function systemUserLoginValidation($username, $password)
    {
        $userData = $this->db->row("SELECT system_user_id, fname, lname, uname, email, last_login FROM system_users WHERE uname = :username AND password = :password", array('username' => $username, 'password' => sha1($password)));

        if ($userData)
            return $userData;
        else
            return false; 
    }




}

Login controller:

<?php 
namespace MyApp\Controllers;

use MyApp\Core\Controller;
use MyApp\Models\System_user;
use MyApp\Helpers\Token;
use MyApp\Helpers\Input;
use MyApp\Helpers\Redirect;
use MyApp\Helpers\General;
use MyApp\Helpers\Validation;
use MyApp\Core\Config;

/**
 *
 *  Login Class
 *
 */
class Login extends Controller
{

    /**
     *
     *  Constructor 
     *
     */ 
    public function __construct() {}


    /**
     *
     *  Index: Login Main login Form
     *
     */
    public function index($name ='')
    {   
        // Create a new system user
        $system_user = new System_user();
        // If user is logged in - Redirect to dashboard
        if ( $system_user->check_logged_in() )
            Redirect::to('dashboard');          // Redirect to login form
        else
            // $this->view('login/pages-login', array('token'=>'banana'));  // Redirect to login form
            $this->view( 'login/pages-login', array( 'token' => Token::generate_form_token() ) );   // Redirect to login form
    }


    /**
     *
     *  User login: Creates the user login. 
     *
     */
    public function user_login()
    {
        # Check if there are any inputs submitted
        if (Input::exists()) {

            # Check if submitted token is identical to the one that's currently set to the session.
            if (Token::check(Input::get('token'))) {

                # Validation init
                $validation = new Validation();

                # Set validation requirements 
                $validation = $validation->check($_POST, array(
                    'company_name'  => array(
                                                'required'  => true, 
                                                'min'       => 3, 
                                                'max'       => 30
                    ),
                    'user_name'     => array(
                                                'required'  => true, 
                                                'min'       => 3, 
                                                'max'       => 30, 
                                                'unique'    => 'system_users'
                    ),
                    'password'      => array(
                                                'required'  => true,
                                                'min'       => 6, 
                                                'max'       => 30
                    )
                ));

                if ( $validation->passed() ) {


                    # Create a new user object
                    // $this->system_user = new System_user();

                    // # Check login 
                    // if ($this->system_user->login(Input::get('company_name'), Input::get('user_name'), Input::get('password'))) {
                    //  General::toJson(array( 'status' => true, 'message' => 'You have successfully logged in.' ));
                    // } else {
                    //  General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' ));
                    // }


                    # Create a login obj 
                    $login              = new \MyApp\Models\SystemUser\SystemUserLogin(Input::get('company_name'), Input::get('user_name'), Input::get('password'));
                    # Create a new user object
                    $this->systemUser   = new \MyApp\Models\SystemUser();
                    # Set login credentials 
                    $this->systemUser->setSystemUserLogin($login);
                    # Login
                    # Check login 
                    if ( $this->systemUser->login() ) {
                        General::toJson(array( 'status' => true, 'message' => 'You have successfully logged in.' ));
                    } else {
                        General::toJson(array( 'status' => false, 'message' => 'Bad login credentials.' ));
                    }




                }
            }

        }


    }

}

Controller class (Login class extends Controller):

<?php 
namespace MyApp\Core;

/**
 *
 *  Controller instance: 
 *
 */
class Controller
{

/*=================================
=            Variables            =
=================================*/

    # System User 
    protected $system_user;



/*===============================
=            Methods            =
================================*/


    /**
     *
     *  Constructor 
     *
     */
    public function __construct()
    {
        # Check if system user is logged in / still logged in / Validate tokens  
        $this->system_user = new \MyApp\Models\SystemUser();

        // # Redirect to login if user not logged in
        // if (!$this->system_user)
        if (!$this->system_user->isLoggedIn())
            \MyApp\Helpers\Redirect::to('login');
    }


    /**
     *
     *  Model Class: Loads a requested model
     *  @param  $model      String      Gets a model name
     *  
     */
    protected function model($model)
    {
        require_once '../MyApp/models/' . $model . '.php';
        return new $model();
    }


    /**
     *
     *  View Class: Loads a requested view
     *  @param  $view       String      Gets a view name 
     *  @param  $data       Array       (optional) Gets an array of variables to pass to the view
     *  @throws                         Plain view 
     *
     */
    protected function view($view, $data=[])
    {
        require_once '../MyApp/views/' . $view . '.php';
    } 


    /**
     *
     *  Check if a user is logged in
     *
     */
    protected function is_loggedin()
    {
        # Flag for final result: 
        $flag = false; 

        # init user obj
        $this->system_user = new \MyApp\Models\System_user();

        # Check if user is logged in 
        if ($this->system_user->isLoggedIn()) {
            # Check if the user is timed-out 
            // if (!$this->system_user->check_timeout()){
            if ( !$this->system_user->checkTimeout() ) {

                # If system user exists
                // if ( $this->system_user->find(intval(Session::get(Config::$systemUserId))) ){

                    # Re-generate users secret stuff 
                    $this->system_user->generate_security_sessions(false);
                    $flag = true;
                // }
            }
        }

        # To return true "it" must pass all "if"s
        if ( $flag )
        {
            # Return System_user object
            return $this->system_user;
        }
        else {
            # logout the user
            $this->system_user->logout();
            return false;
        }
    }



/**************************************************************************************************/


    /**
     *
     *  Automate Views
     *
     */
    protected function dashboard($optionArray, $view)
    {   
        $this->view('main/head', ['controller_name' => \MyApp\Helpers\General::remove_namespace(get_class($this))]);
        $this->view('main/body');
        $this->view('main/top_bar');
        $this->view('main/sidebar',
                        [ 
                            'first_name'    => ($this->system_user->firstName) ? $this->system_user->firstName : '', 
                            'last_name'     => ($this->system_user->lastName)  ? $this->system_user->lastName  : '', 
                            'main_menu'     => \MyApp\Helpers\General::main_menu()
                        ]
                    );
        $this->view('main/page_wrapper', ['controller_name' => \MyApp\Helpers\General::remove_namespace(get_class($this))]);
        if ( in_array('date',$optionArray) ){
            $this->view('main/datePicker');
        }
        $this->view($view);
        $this->view('main/footer');

    }

}
\$\endgroup\$
  • \$\begingroup\$ I think this needs some carefull thought. The new code is still full of mixed responsibilities - it seems like a SystemUser should be a simple data object, SystemUserDetails is superflous, the other two both do things unrelated to their names, and none of these objects should be using superglobals. \$\endgroup\$ – Steve Jan 11 at 15:02
  • \$\begingroup\$ Perhaps if you showed how you actually use these classes it would help clear things up. Are you using an MVC approach? if yes please show your LoginController (or whatever its called) \$\endgroup\$ – Steve Jan 11 at 15:06
  • \$\begingroup\$ Oh and i see from your last question you have a (common) issue with dependancy injection, namely "having to pass the DB to every class that uses it" - this only seems like a problem when you are not really using dependency injection. If the app is using it properly, you pretty much never use the new keyword outside of your composition root, or factory classes. \$\endgroup\$ – Steve Jan 11 at 15:12
  • \$\begingroup\$ @Steve Thanks for your reply, I edited my post with these code parts you requested. \$\endgroup\$ – potatoguy Jan 13 at 14:29
3
\$\begingroup\$

In my last answer for you I mention how you should use dependency injection to avoid tight coupling and promote testability. Then in the comments I go further to mention to take it a step further and use a container like Pimple. Since I don't see those changes here I'll show the container example here since I showed the basic dependency injection in the other answer.

Using Pimple, the Dependency Injection Container

I'll assume you will have installed Pimple already and have included it in your application. Their documentation covers that so I won't get into it here.

use Pimple\Container;
use MyApp\Core\Database;

$container = new Container();

$container['db'] = function ($c) {
    return Database::getInstance();
};

The above code simply:

  1. Creates a container
  2. Defines a service called db
  3. Instantiates your database class
  4. Places it in your container

You can add your session logic and other shared objects at this time as well. This is typically contained in its own file but where you put this ins entirely up to you as long as it executes as part of your bootstrap process (i.e. before your business logic).

From here you only need to include Pimple as an argument of the constructor of objects that need to use something in your container.

class System_user
{
    public function __construct(\Pimple $container, $system_user = NULL)
    {
        $this->db = $container['db'];
    }
}

Now you can easily make sure all of your classes are working with the same objects, eliminate dependencies in your code, and your code is testable.


Good job with not putting login info into the User object

A common pitfall many developers fall into is to put the login logic into a user object because the user is the one who logs in. You pass the User object into the login functionality which is a much better way to do this. An area for improvement is you place the validation and the login logic all in one method. You could break out the validation into it's own method so you separate the two concerns. You also do this like work with IP addresses again which should be separated out into its own logic.

Getting IP addresses is kind of common

You have a private method for getting the user's IP address (systemUserLogin::getSystemUserIp()). That actually is something not directly related to a user and may be something you eventually wish to use elsewhere. That probably should be broken out into its own function or into another helper class.

FYI Stuff

sha1() is obsolete for hashing passwords and should not be used. PHP provides password_hash() and password_verify(), please use them. And here are some good ideas about passwords. If you are using a PHP version prior to 5.5 there is a compatibility pack available here.

\$\endgroup\$
  • \$\begingroup\$ I made a little research about pimple and it's amazing! Thank you! I am mainly expecting a review regarding the re-construction of the class (/classes). \$\endgroup\$ – potatoguy Jan 8 at 9:31
  • 1
    \$\begingroup\$ I added some more info directly related to how you architected your classes. I'll add more if I think of anything through the day. \$\endgroup\$ – John Conde Jan 8 at 12:53
  • 1
    \$\begingroup\$ SHA1 has been obsolete since 2005 when it was first successfully attacked by researchers. \$\endgroup\$ – John Conde Jan 9 at 12:50
  • 1
    \$\begingroup\$ Those properties look like they belong ot the SystemUser class as they are properties of that user. \$\endgroup\$ – John Conde Jan 9 at 14:57
  • 1
    \$\begingroup\$ That's up to you. Usually containers store generic services. But that doesn't mean you can't put objects like users in there. If that will make your application easier to maintain then it is not a bad idea. \$\endgroup\$ – John Conde Jan 10 at 12:51

Your Answer

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

Not the answer you're looking for? Browse other questions tagged or ask your own question.