DB abstraction, private methods in OOP PHP library

By Posted on

Problem

This library registers a new user.

Questions:

  1. Where should the DB class instantiation happen for user class? I tried instantiation in the constructor but that property doesn’t seem to be available to the rest of the user class. As a work-around, I’m creating a new instance with every function that needs the DB class. It works, but this must be wrong.

  2. Which methods should be public and which private/protected? I’m thinking encryptUser() should be private, but I’m not sure.

  3. Should my database functions to insert and query be combined into one function or is it fine having two? What would a combined function look like.?

  4. Is there a better way to access my config variables or is putting global $config in the constructor?

<?PHP

error_reporting(E_ALL);
require_once 'config.php';
define('BASEPATH',realpath(dirname(__FILE__))  );
define('APPROOT', 'http://blahblah.com/' );


class User{

  public $data;


  public function __construct(){
    $data=new DB;

  }

  public function createNew(){
    //trim post values
    if(!empty($_POST['email'])&& !empty($_POST['password'])){
       $fname=trim($_POST['fname']);
       $lname=trim($_POST['lname']);
       $userEmail=trim($_POST['email']);
       $password=trim($_POST['password']);

         if(!$this->user_exists($userEmail)){

          //salt, hash then insert new user into database
           $crypt=$this->encrypt_user($password);

           $mydata=new DB;

           $sql="INSERT INTO users (fname,lname, password,salt, email) 
                     VALUES (:fname,:lname, :password,:salt,:email)";
           $query_params=array(':fname'=>$fname,':lname'=>$lname,
                            ':password'=>$crypt['password'],':salt'=>$crypt['salt'],
                            ':email'=>$userEmail);

           if($mydata->dbInsert($sql,$query_params)){

             echo '<br>INSERT SUCCESSFUL';

            }else{die('Insert Failed');}

          }
          else{
               echo 'A user with the email'.$userEmail.' already exists. 
                Please user another or click here to login';
                }



    }

  }/*END CREATE USER*/


 public function user_exists($email){
   $mydb=new DB;
   $query='SELECT email from users WHERE email=:email';
   $params=array(':email'=>$email);

   if($mydb->dbQuery($query,$params)){
      return true;
     }else{
           return false;
          }
    }

 public function encrypt_user($pass){
    $salt=dechex( mt_rand(0,2147483678) ).dechex( mt_rand(0,2147483678) );
    $password=hash('sha256',$pass.$salt);

    for($round=0;$round<65336; $round++){
           $pass_hash=hash('sha256',$password.$salt);
           }

  return array('salt'=>$salt,'password'=>$pass_hash);

 }//*end encrypt_user()


}

//----------**END CLASS USER**------------


class DB {

    private $db;
    private $dbHost;
    private $dbUser;
    private $dbPass;


   public function __construct(){ 
    global $config;

    $this->db=$config['dbName'];
    $this->dbHost=$config['dbHost'];
    $this->dbUser=$config['dbUser'];
    $this->dbPass=$config['dbPass']; 
   }


  public function connect(){
   $dbConn=new PDO('mysql:dbname='.$this->db.';
   host='.$this->dbHost.'',$this->dbUser,$this->dbPass);
   return $dbConn;
  }


 public function dbQuery($query,$params){
    if($dbConn=$this->connect()){
    try{
    $stmt=$dbConn->prepare($query);
    $result=$stmt->execute($params);
    var_dump($stmt);    
    $row=$stmt->fetch(PDO::FETCH_ASSOC);

    }catch(PDOException $e){

     die('Error: '.$e->getmessage());
    }


    if(!empty($row)){

     return $row;
    }else{

    return false;
    }

  }
 }//**END dbQUERY

 public function dbInsert($query,$params){

   if($connDB=$this->connect()){
      try{
            $stmt=$connDB->prepare($query);
            $result=$stmt->execute($params);

            return true;
            }catch(PDOException $ex){
            /*THIS COULD ALSO BE SENT TO A LOG FILE*/
            $ex->getmessage();

            } 
   }

 }//**End dbInsert


} 
//**-----------END CLASS DB-----------


class render {

 public function regUser(){

  if(!isset($_POST['submit']))
  { //SHOW FORM

   ?>
    <div id="accform">
      <form method="post" action="#" name="newacc">
        <span>First Name</span><input type="text" name="fname" id="FName">
        <span>Last Name</span><input type="text" name="lname" id="LName">
        <span>Email</span><input type="text" name="email" id="<Mail">
        <span>Password</span><input type="text" name="password" id="PWord">

        <input type="submit" name="submit" value="Sign Up">
      </form>
    </div>
  <?
  } else{
      //process information
      $user= new user;
      $user->createNew();

    }
  }//*END register function


 }
//END OF RENDER CLASS

Solution

Question 1: You’ve partially got the idea. It’s acceptable syntax, but it won’t work, you need to use $this which references the current class. However, you’re not allowing for dependency injection, and keeping you class very tight together. Here’s a way that’s commonly seen:

public function __construct(DB $database) 
{
    $this->data = $database;
}

Question 2: Perfect read from the docs.

Class members declared public can be accessed everywhere. Members
declared protected can be accessed only within the class itself and by
inherited and parent classes. Members declared as private may only be
accessed by the class that defines the member.

Since you’re not using those encryption methods outside of the internal scope, a visibility of private would be acceptable.

Question 3: I advise not making your own database access class because…

  1. Homemade database access objects almost always lack some sort of functionality.
  2. They are very likely to be insecure
  3. They’re not community reviewed (most of the time)

I suggest you use someone else’s maintained and secure code. They’ll often have proper error handling too. Almost every PHP framework has one, and there’s a dozen on GitHub. It’s too hard to specify one, so I suggets you do your research.

Question 4: Yes. Avoid globals. Give yourself room to expand with a config file or a config class. Preferably the file, and then a class to interpret the data the file holds. Again, dependency injection.

Other than that, there are tons of formatting issues. It’s all in the readability of the code. Here ya go:

If you choose to use a framework (i.e. Zend), many come with documented standards and styles that you should follow with that framework.

If you can, get rid of your encryption functions and just use password_hash. It’s a lot safer and easier to use.

Separate your classes into different files, and keep the HTML out of the PHP. Mixing the two makings things a hell-of a lot harder to read!

I’m sorry I don’t really know the answer to number 1, 2, 3. I’m a beginner too, but I think I know a better solution for number 4. As far as I know, global variables are bad. So you should create a new class for storing config data, like this:

class DatabaseConfig
{
    private $configOne = null;
    private $configTwo = null;

    public function __construct()
    {
    }

    public function setConfigOne($value)
    {
        $this->configOne = $value;
    }

    public function setConfigTwo($value)
    {
        $this->configTwo = $value;
    }

    public function getConfigOne()
    {
        return $this->configOne;
    }

    public function getConfigTwo()
    {
        return $this->configTwo;
    }
}

Then modify your database class’s constructor, to accept this config class, and do operations according to the config.

class DB
{
    public function __construct(DatabaseConfig $config)
    {
        if ($config->getConfigOne() == true)
        {
            // do something
        }
        if ($config->getConfigTwo() == 123)
        {
            // do something
        }
    }
}

Then you can pass the config as a parameter to your Database Class, sample usage:

$cfg = new DatabaseConfig();
$cfg->setConfigOne(true);
$cfg->setConfigTwo(123);

$db = new DB($cfg);

Also, it would be better if you organize the classes into different .php files with different namespaces.

Leave a Reply

Your email address will not be published. Required fields are marked *