0

I am new to PHP and I am trying to develop a login system for my website. I have managed to successfully login to my website but now I am trying to tidy my code up and place the database connection code in one class and have other php classes access the one database connection class. At the moment, I don't seem to be able to make a connection to my database. I have tried to find the errors by adding in the relevant error reporting code but nothing is output.

I would appreciate it if someone could look over my code and see where I am going wrong!

Here is my php class which checks the username and password - checklogin.php

<?php

require_once(__DIR__. "/DBConnection.php");

class checklogin {

    public $count=0;

    public function checklogin() {

        ob_start();

        $db = DBConnection()->get_connection();


        $myusername=$_POST['myusername'];
        $mypassword=$_POST['mypassword'];

        $myusername = stripslashes($myusername);
        $mypassword = stripslashes($mypassword);
        $myusername = mysql_real_escape_string($myusername);
        $mypassword = mysql_real_escape_string($mypassword);

        $encrypt_password=md5($mypassword);

        $sql="SELECT id FROM members WHERE username='$myusername' and password='$encrypt_password'";
        $result=mysql_query($sql);
        $row=mysql_fetch_row($result);

        $this->count=mysql_num_rows($result);

        if($this->count==1){
        session_start();
        $_SESSION['ID']=$row[0];
        header("location:login_success.php");
        } else {
            $this->error(); 
        }
        ob_end_flush();

    }
}

?>

Class which I am trying to implement the database connection and set up an instance - DBConnection.php

<?php

class DBConnection{

    private static $instance;

    private function __construct(){
        $host=".....";
        $username="....";
        $password=".....";
        $db_name=".....";

        mysql_connect("$host", "$username", "$password")or die("cannot connect");
        mysql_select_db("$db_name")or die("cannot select DB");
    }

    public static function get_connection(){
        if(empty (self::$instance)) self::$instance = new DBConnection;
        return self::$instance;
    }
}

?>
Dan
  • 138
  • 1
  • 1
  • 12

2 Answers2

1

You would call the static method, like so:

$db = DBConnection::get_connection();

This will give you the instance of the database connection.

gpmcadam
  • 5,406
  • 2
  • 28
  • 35
  • I used: $db = DBConnection()->get_connection(); Is that incorrect, do I need :: instead of -> – Dan Jan 27 '14 at 23:01
  • @Dan The problem there, as far as I can tell, is that the call to `DBConnection()` is returning an instance, and you're calling a static method on an instance of an object. – gpmcadam Jan 27 '14 at 23:03
0

The correct way is to create the database connection object at a higher level, and then pass it as a parameter to your class. This is called dependency injection - your class depends on another one (database), and it gets that object injected.

There are basically two ways to inject that object: For anything that absolutely must be present, pass it as a parameter to the constructor. For anything optional, or which might create a default value internally, use a setter method.

As example:

class checkLogin {
    private $db;
    public function __construct(DBConnection $db) {
        $this->db = $db;
    }

    public function checklogin() {
    ... do anything with $this->db ...
    }
}

Your current code has several major drawbacks:

  1. The DBConnection class is implementing a singleton. Singletons are a bad design pattern and should be avoided if at all possible.
  2. Your DBConnection class does not offer any convenience. It creates a global connection, but it does not offer any method to execute a database query.
  3. You are using the mysql extension. This extension is deprecated and will be removed soon from the default PHP. If you want your code to be able to still run in a few years, use mysqli or PDO.
  4. $db = DBConnection()->get_connection(); looks like a syntax error to me. There probably is no function "DBConnection()" defined. The currently correct call must be static: DBConnection::get_connection(); - but static calls are just another form of global state. It works, but you will sooner or later come to the conclusion that it will hurt you.
Sven
  • 62,889
  • 9
  • 98
  • 100
  • I originally created a database connection for each php class. I thought that creating the db connection once would be more efficient. What would the best design pattern be? – Dan Jan 27 '14 at 23:15
  • Ok, on the lowest level multiple calls to `mysql_connect` with the same connect data will only create one connection and reuse it. If you'd need a new connection explicitly, there is a parameter `$new_link` set to true. So not doing anything would have solved your problem from the start - you probably are creating the connection only once. But this isn't very object oriented. To get there, you'd create one database object at the start, might store it in a global variable (putting it in a static variable is equal, even if it is private), and pass it to every object that needs a database. – Sven Jan 28 '14 at 01:08
  • -1 For "The DBConnection class is implementing a singleton. Singletons are a bad design pattern and should be avoided if at all possible.". This is simply your opinion and can't be backed up by evidence. – gpmcadam Jan 29 '14 at 10:55
  • You probably have evidence to back your comment... but we probably should not discuss this again. All arguments are collected here: http://stackoverflow.com/questions/137975/what-is-so-bad-about-singletons – Sven Jan 29 '14 at 18:02