0

I got this PDO database class

class clsDatabase{
  // db settings
  private $host   = 'localhost';
  private $user   = 'test';
  private $dbname = 'test';
  private $pass   = 'test1';

  private $dbh;
  private $error;

  public function __construct(){
        // Set DSN
        $dsn = 'mysql: host=' . $this->host . ';dbname=' . $this->dbname;
        // Set options
        $options = array(
            PDO::ATTR_PERSISTENT            => true,
            PDO::ATTR_ERRMODE               => PDO::ERRMODE_EXCEPTION,
            PDO::MYSQL_ATTR_INIT_COMMAND    => 'SET NAMES UTF8'
        );
        // Create a new PDO instanace
        try{
            $this->dbh = new PDO($dsn, $this->user, $this->pass, $options); 
        }
        // Catch any errors
        catch(PDOException $e){
            $this->error = $e->getMessage();
            echo $this->error;
            exit;
        }       
    }

    public function query($query){
        $this->stmt = $this->dbh->prepare($query);
    }
}   

I try to seperate my code in different classes, for example i got a clsDBUser which is connected to the clsUserController. I do this so i know what class uses what database code. My clsDBUser class looks like this

class clsDBUser extends clsDatabase {
    // construct
    public function __construct() {
        parent::__construct();
    }

    // get users
    public function getUsers($users_id){
        $query = "
            SELECT 
                email
            FROM 
                users
            WHERE 
               users_id = :users_id
        ";          
        $this->query($query);
        $this->bind(':users_id', $users_id);

        if($row = $this->single()){
            $this->close();
            return $row;
        }
        $this->close();
        return false;       
    }
}

I am wondering if this is the way to go or am i creating a new database connection in every class right now? Because normally in PHP4 (yes i know old) i can't recognize i had to make a new database connection every time.

Do i need to improve this, how do i need to improve this?

poNgz0r
  • 69
  • 1
  • 8
  • Watch singleton implementation and otherwise design pattern like factory , or see other solutions : http://stackoverflow.com/questions/2129162/how-do-you-efficiently-connect-to-mysql-in-php-without-reconnecting-on-every-que – Fky Feb 08 '17 at 16:07
  • PDO already has a perfectly good class, why reinvent a wheel badly and unnecessarily – RiggsFolly Feb 08 '17 at 16:07
  • Why call a method `query()` when all its doing is a `prepare` **all you are adding to PDO is confusion** – RiggsFolly Feb 08 '17 at 16:10
  • 2
    [Your first database wrapper's childhood diseases](https://phpdelusions.net/pdo/common_mistakes) – Your Common Sense Feb 08 '17 at 16:15

5 Answers5

2

You should take the road shown in the mr.void's answer. In short:

  1. get rid of clsDatabase.
  2. Create an instance of PDO.
  3. pass it into clsDBLogin's property like it shown in mr.void's answer.
  4. Then use this pdo instance in the form of $this->db->prepare() and so on

So it should be like

class clsDBLogin
{
    public function __construct($db)
    {
        $this->db = $db;
    }

    public function validateLogin($email)
    {  
        $email = trim($email);

        // Check user in db to start verification
        $query = 'SELECT * FROM users u, users_info ui 
                  WHERE u.users_id = ui.users_id AND u.email = ?';
        $stmt = $this->db->prepare($query);
        $stmt->execute([$email]);
        return $stmt->fetch();
    }
}

$dsn = 'mysql: host=localhost;dbname=test;charset=utf8';
$options = array(
        PDO::ATTR_PERSISTENT            => true,
        PDO::ATTR_ERRMODE               => PDO::ERRMODE_EXCEPTION,
);
// Create a new PDO instanace
$pdo = new PDO($dsn, $this->user, $this->pass, $options); 

$DBLogin = new clsDBLogin($pdo);
$user = $DBLogin->validateLogin($email);
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
1

Hey i would do Something like this

class DB {
   // connectionStuff goes Here
}

class Model {
   private $db

   public function __construct($db) {
       $this->db = $db;
   }
}

Use:

$db = new DB("your connection stuff goes here");


$model = new Model($db);
$userModel = new UserModel($db);
$anotherModel = new AnotherModel($db);
Funk Forty Niner
  • 74,450
  • 15
  • 68
  • 141
enno.void
  • 6,242
  • 4
  • 25
  • 42
  • Wasn't it exactly the question? Because in this snippet you will call `$db = new DB..` every time, before every action in all the classes - isn't it somehow inefficient? – The Godfather Jun 10 '19 at 10:07
  • @TheGodfather no, in the inital question, a connection is made for every instance of an object that inherit the parent class. In my example there is only one instance for creating the connection which is passed to all instances which depending on a sql connection – enno.void Jun 13 '19 at 07:39
1

Rebuild:

clsDB class with connection stuff only

class clsDB{
    // db settings
    private $host   = 'localhost';
    private $user   = 'test';
    private $dbname = 'test';
    private $pass   = 'test';

    private $dbh;
    private $error;

    public function __construct(){
        // Set DSN
        $dsn = 'mysql: host=' . $this->host . ';dbname=' . $this->dbname;
        // Set options
        $options = array(
            PDO::ATTR_PERSISTENT            => true,
            PDO::ATTR_ERRMODE               => PDO::ERRMODE_EXCEPTION,
            PDO::MYSQL_ATTR_INIT_COMMAND    => 'SET NAMES UTF8'
        );
        // Create a new PDO instanace
        try{
            $this->dbh = new PDO($dsn, $this->user, $this->pass, $options); 
        }
        // Catch any errors
        catch(PDOException $e){
            $this->error = $e->getMessage();
            echo $this->error;
            exit;
        }       
    }           
}

clsDBLogin:

class clsDBLogin{   
   private $db;

   public function __construct($db) {
       $this->db = $db;
   }
}

In index.php i do:

$clsDB      = new clsDB();
$clsDBLogin = new clsDBLogin($clsDB);

In clsDBLogin i would do:

public function validateLogin($email){  
    $email = str_replace(' ', '', $email);
    $email = strtolower($email);

    // Check user in db to start verification
    $query = '
        SELECT
            u.*, ui.*
        FROM
            users as u,
            users_info as ui
        WHERE
            u.users_id = ui.users_id
        AND
            u.email = :email                
    ';
    $this->db->prepare($query);
    $this->db->bindValue(':email', $email, PDO::PARAM_STR);

    if($this->db->execute()){
        if($row = $this->db->fetch(PDO::FETCH_ASSOC)){
            return $row;            
        }       
    }   
}
poNgz0r
  • 69
  • 1
  • 8
  • Not quite. You've got a code that is quite messy (clsDatabase::getInstance() ALL over the place), confusing (using query as an alias for prepare I would call a sabotage) and inflexible (this code will stick to single instance and you won't be able to use another one). Besides, upon closing the connection you won't be able to run more than one query in your script which is rather ridiculous – Your Common Sense Feb 09 '17 at 08:46
  • Oke, so what is the way to go then? – poNgz0r Feb 09 '17 at 08:56
  • 1. get rid of clsDatabase. 2. Create an instance of PDO. 3. pass it into clsDBLogin's property like it shown in mr.void's answer. 4. Then use this pdo instance in the form of $this->db->prepare() and so on – Your Common Sense Feb 09 '17 at 09:00
  • This one is essentially better. Though there is still a lot of useless code, in general the approach is ok – Your Common Sense Feb 09 '17 at 09:40
0

There are three layer here:

  • database connector: you can use pure PDO for this or a database abstraction layer library (Doctrine DBAL)
  • repository of entities: in other words, some kind of ORM. Doctrine provides advanced ORM functionality. Of course, you can write your own lightweight solution.
  • entity: can be a simple CRUD, an ActiveRecord or any other object representation of a logical record.

When we do this manually... First, don't extend these from each other. Generally: never extend a different layer from an other. Use Dependency Injection (DI) instead.

It's a very simple case of DI when you pass all the specific information (dependencies) as constructor parameters. My active-object-like example Entity just knows how an entity should be behave in general (at a key in a repository). For simplicity, I use raw SQL.

Repository class:

class Repository {

    private $oPDO;
    private $tableName;
    private $keyFieldName;

    public function __construct($oPDO, $tableName, $keyFieldName) {
        $this->oPDO = $oPDO;
        $this->tableName = $tableName;
        $this->keyFieldName = $keyFieldName;
    }

    public function getPDO() {
        return $this->oPDO;
    }

    public function getTableName() {
        return $this->tableName;
    }

    public function getKeyFieldName() {
        return $this->keyFieldName;
    }

    public function getEntity($id) {
        return new Entity($this, $id);
    }

    public function createEntity() {
        return new Entity($this, null);
    }

}

Entity class:

class Entity implements ArrayAccess {

    private $oRepository;
    private $id;

    private $record = null;

    public function __construct($oRepository, $id) {
        $this->oRepository = $oRepository;
        $this->id = $id;
    }

    public function load($reload = false) {
        if (!$this->record && !$this->id) {
            return false;
        }

        if (!$reload && !is_null($this->record)) {
            return true;
        }

        $quotedTableName = $this->quoteIdentifier($this->oRepository->getTableName());
        $quotedKeyFieldName = $this->quoteIdentifier($this->oRepository->getKeyFieldName());
        $selectSql = "SELECT * FROM {$quotedTableName} WHERE {$quotedKeyFieldName} = ?";
        $oStatement = $this->oRepository->getPDO()->prepare($selectSql);
        $this->bindParam($oStatement, 1, $this->id);
        $oStatement->execute();

        $result = $oStatement->fetch(PDO::FETCH_ASSOC);

        if ($result === false || is_null($result)) {
            return false;
        }

        $this->record = $result;
        return true;
    }

    public function save() {
        $oPDO = $this->oRepository->getPDO();

        $tableName = $this->oRepository->getTableName();
        $keyFieldName = $this->oRepository->getKeyFieldName();
        $quotedTableName = $this->quoteIdentifier($tableName);
        $quotedKeyFieldName = $this->quoteIdentifier($keyFieldName);

        if (is_null($this->id)) {
            $insertSql = "INSERT INTO {$quotedTableName} (";
            $insertSql .= implode(", ", array_map([$this, "quoteIdentifier"], array_keys($this->record)));
            $insertSql .= ") VALUES (";
            $insertSql .= implode(", ", array_fill(0, count($this->record), "?"));
            $insertSql .= ")";
            $oStatement = $oPDO->prepare($insertSql);

            $p = 1;
            foreach ($this->record as $fieldName => $value) {
                $this->bindParam($oStatement, $p, $value);
                $p++;
            }

            if ($oStatement->execute()) {
                $this->id = $oPDO->lastInsertId();
                return true;
            } else {
                return false;
            }
        } else {
            $updateSql = "UPDATE {$quotedTableName} SET ";
            $updateSql .= implode(" = ?, ", array_map([$this, "quoteIdentifier"], array_keys($this->record)));
            $updateSql .= " = ? WHERE {$quotedKeyFieldName} = ?";
            $oStatement = $oPDO->prepare($updateSql);

            $p = 1;
            foreach ($this->record as $fieldName => $value) {
                $this->bindParam($oStatement, $p, $value);
                $p++;
            }
            $this->bindParam($oStatement, $p, $this->id);

            if ($oStatement->execute()) {
                if (isset($this->record[$keyFieldName])) {
                    $this->id = $this->record[$keyFieldName];
                }
                return true;
            } else {
                return false;
            }
        }
    }

    public function isExisting($reload = false) {
        if (!$this->record && !$this->id) {
            return false;
        }

        if (!$reload && !is_null($this->record)) {
            return true;
        }

        $quotedTableName = $this->quoteIdentifier($this->oRepository->getTableName());
        $quotedKeyFieldName = $this->quoteIdentifier($this->oRepository->getKeyFieldName());
        $selectSql = "SELECT 1 FROM {$quotedTableName} WHERE {$quotedKeyFieldName} = ?";
        $oStatement = $this->oRepository->getPDO()->prepare($selectSql);
        $oStatement->bindParam(1, $this->id);
        $oStatement->execute();

        $result = $oStatement->fetch(PDO::FETCH_ASSOC);

        if ($result === false || is_null($result)) {
            return false;
        }

        return true;
    }

    public function getId() {
        return $this->id;
    }

    public function getRecord() {
        $this->load();
        return $this->record;
    }

    public function offsetExists($offset) {
        $this->load();
        return isset($this->record[$offset]);
    }

    public function offsetGet($offset) {
        $this->load();
        return $this->record[$offset];
    }

    public function offsetSet($offset, $value) {
        $this->load();
        $this->record[$offset] = $value;
    }

    public function offsetUnset($offset) {
        $this->load();
        $this->record[$offset] = null;
    }

    private function quoteIdentifier($name) {
        return "`" . str_replace("`", "``", $name) . "`";
    }

    private function bindParam($oStatement, $key, $value) {
        $oStatement->bindParam($key, $value);
    }

}

Usage:

$oRepo = new Repository($oPDO, "user", "user_id");

var_dump($oRepo->getEntity(2345235)->isExisting());

$oSameUser = $oRepo->getEntity(1);
var_dump($oSameUser->isExisting());
var_dump($oSameUser->getRecord());

$oNewUser = $oRepo->createEntity();
$oNewUser["username"] = "smith.john";
$oNewUser["password"] = password_hash("ihatesingletons", PASSWORD_DEFAULT);
$oNewUser["name"] = "John Smith";
$oNewUser->save();

$oNewUser["name"] = "John Jack Smith";
$oNewUser->save();

Of course, you can extend a MoreConcreteRepository from Repository and MoreConcreteEntity from Entity with specific behavior.

Dávid Horváth
  • 4,050
  • 1
  • 20
  • 34
-1

Simply don't extend an entity (clsDBUser) from a connection class (clsDatabase).

Use a singleton (or something more advanced pattern) for clsDatabase.

For example:

class clsDatabase {

    static private $instance = null;

    // some other private fields

    private function __construct(/* parameters*/) {
        // do it
    }

    public static function instance() {
        if (is_null(self::$instance)) {
            self::$instance = new self(/* pass any parameters */);
        }
        return self::$instance;
    }

    public function queryRow($query) {
        $oStatement = $this->dbh->prepare($query);

        // ...

        return $row;
    }
}

class clsDBUser {

    public function getUser($id) {
        $query = "...";
        return $clsDatabase::instance()->queryRow($query);
    }

}
Cœur
  • 37,241
  • 25
  • 195
  • 267
Dávid Horváth
  • 4,050
  • 1
  • 20
  • 34
  • Singleton is an Antipattern, you should avoid it. What u want is DI (https://en.wikipedia.org/wiki/Dependency_injection). You can php-di. Its a DIC (Dependency Injection Container) which automatic wire up your dependencies – enno.void Feb 08 '17 at 16:13
  • 1
    Yes, DI is the correct *solution*, however "don't extend" is the correct *answer*. I have written: "or something more andvanced pattern". – Dávid Horváth Feb 08 '17 at 16:16
  • Oké so can I use your example? Because I am gonna try this tomorrow and might get back to you with more questions, if that is oké – poNgz0r Feb 08 '17 at 20:18
  • @Dávid Horváth can you provide me a full example please? – poNgz0r Feb 09 '17 at 08:25
  • @poNgz0r See my other answer for an advanced example. – Dávid Horváth Feb 10 '17 at 16:50