Am I correctly implementing PHP/PDO and the MVC/DAO pattern?

Question

Context:

I am working on the login module of a PHP web-application and I am trying to implement this using PHP/PDO and the DAO pattern. I would like someone to review the code below and let me know whether my understanding is correct or not,i.e, am I implementing it correctly.

The application would require the user to provide his/her username and password and then it would just check the database whether he/she is a valid user or not.

user_master:

Project Structure:

login_handler.php:

<?php

require ("include_dao.php");

// Fetch the supplied username and password.
$username = $_POST['username'];

$password = $_POST['password'];

// Instantiate and populate UserMaster bean.
$usermaster = new UserMaster;

$usermaster->setUsername($username);

$usermaster->setPassword($password);

// Instantiate and invoke the desired DAO method.
$usermasterDAO = new UserMasterMySqlDAO;

$usermasterDAO->queryByUsernameAndPassword($username, $password);

?>

queryByUsernameAndPassword():

public function queryByUsernameAndPassword($username, $password) {

        // Initiate DatabaseHandler instance.
        $databaseHandler = new DatabaseHandler;

        // Establish a connection
        $pdoConnection = $databaseHandler -> connect();

        try {
            $query = $pdoConnection -> prepare('SELECT * FROM cbiz.user_master WHERE username = ? AND password = ?');

            $query -> bindParam(1, $username, PDO::PARAM_STR);

            $query -> bindParam(2, $password, PDO::PARAM_STR);

            $query -> execute();

            if ($query -> execute()) {
                echo "TRUE";
            } else {
                echo "FALSE";
            }

        } catch (PDOException $pdoException) {// Write into log and show the exception message.

            $databaseHandler -> closeConnection();

            echo $databaseHandler -> logException($pdoException -> getMessage());

            die();
        }
    }

DatabaseHandler - Constructor:

/*
     * ###################### Constructor.######################
     *
     * 1. Instantiate Log class
     * 2. Connects to the database.
     * 3. Creates the parameters array.
     */
    public function __construct() {
        $this -> log = new LogHandler();
        $this -> connect();
    }

DatabaseHandler - connect():

/**
     *  ########## METHOD 1 - ESTABLISHES A CONNECTION TO THE DATABASE #########
     *
     * 1. Reads the database settings from an ini file.
     * 2. Tries to connect to the database.
     * 3. If connection failed, exception is displayed and a log file gets created.
     */
    public function connect() {

        $path_to_config_file = $_SERVER['DOCUMENT_ROOT'] . '/SMS/core/config/' . 'config.ini';

        // Read settings from INI file.
        $this -> settings = parse_ini_file($path_to_config_file);

        // Set the Data Source name - mysql:host=127.0.0.1;dbname=cbiz
        $dsn = 'mysql:dbname=' . $this -> settings["dbname"] . ';host=' . $this -> settings["host"] . '';

        $username = $this -> settings["db_user"];

        $password = $this -> settings["db_password"];

        // Set UTF8 .
        $options = array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES utf8', );

        try {

            $this -> pdoObject = new PDO($dsn, $username, $password, $options);

            // Log any exceptions on Fatal error.
            $this -> pdoObject -> setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);

            // Disable emulation of prepared statements, use REAL prepared statements instead.
            $this -> pdoObject -> setAttribute(PDO::ATTR_EMULATE_PREPARES, false);

            // Connection succeeded, set the boolean to true.
            $this -> isConnected = true;

            // echo "Connection established!";          

            return $this->pdoObject;

        } catch (PDOException $pdoException) {// Write into log and show the exception message.

            echo $this -> logException($pdoException -> getMessage());

            die();
        }

    }

Any help, suggestions, criticisms are appreciated.

Answer 1

• You have way too many newlines. Newlines should separate logical units of code, not each line from each other, because this actually makes your code harder to read.
• Be consistent with your spacing. Either always surround -> with spaces, or never do so.
• Don't echo or die in classes, as it takes options away from the calling classes (they can't change the output, they can't recover from errors, etc).
• Most of your comments don't add all that much, they just repeat the code, which actually leads to less readable code (a reader has to find the comments that actually matter).
• Hash your passwords.
• Do you expect the need for a different DBMS in the near future? Because if not, I wouldn't design for it now. Having UserMasterDAO and UserMasterMySqlDAO without the need for it just adds additional complexity, with little benefit (if you do need to support a different DBMS in the far future, extracting the interface of the DAO can be easily done with any IDE).
• You call connect in the constructor of the handler, but after constructing it, you call connect a second time.
• You also execute twice.
• It's not all that clear what UserMaster is needed for right now (you set the username and password, but never use it anywhere).

Answer 2

We clearly only see the portions of your code, you want us to comment on. I can understand that, the question would get too long. Your question is very broad though: Whether you 'understand' it. That's rather difficult to say from these parts. It seems like you do, but I did notice some obvious things:

Never store passwords

It is good practice never to store password in a database, not even encrypted. What you store is a hash and a salt, and then you use these to see if access is allowed. It would be a very long answer to explain this all, so take a look here: https://stackoverflow.com/questions/401656/secure-hash-and-salt-for-php-passwords and here: https://crackstation.net/hashing-security.htm which explains it much better than I can.

Open database once, then reuse

I assume you have one, or perhaps a few, databases with a lot of, perhaps quite unrelated information in them. Just one of the many tables present might be your user_master table. You even have a nice INI file which contains your database access information. (Make sure this INI file is not served to the world through your webserver.)

It is a bit strange that you open a connection to the database within the queryByUsernameAndPassword() method. This method shouldn't be doing that, it's got one task: To see if the username and password occur in the database. A better name would be: userExists().

My advice would be to open a connection to the database at the beginning of your application, and then reuse this throughout the application. Opening a database is a lengthy process which you do not want to repeat many times.