PHP: Proper way of using a PDO database connection in a class

Question

Trying to organize all my code into classes, and I can't get the database queries to work inside a class. I tested it without the class wrapper, and it worked fine. Inside the class = no dice. What about my classes is messing this up?

EDIT: The problem is that it won't query the database and return a result. Any result.

class ac
  {
  public function dbConnect() 
    {
    global $dbcon;

    $dbInfo['server'] = "localhost";
    $dbInfo['database'] = "sn";
    $dbInfo['username'] = "sn";
    $dbInfo['password'] = "password"; 

    $con = "mysql:host=" . $dbInfo['server'] . "; dbname=" . $dbInfo['database'];
    $dbcon = new PDO($con, $dbInfo['username'], $dbInfo['password']);
    $dbcon->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $error = $dbcon->errorInfo();

    if($error[0] != "") 
      {
      print "<p>DATABASE CONNECTION ERROR:</p>";
      print_r($error);
      }
    }


  public function authentication()
    {
    global $dbcon;

    $plain_username = $_POST['username'];
    $md5_password = md5($_POST['password']);

    $ac = new ac();
    if (is_int($ac->check_credentials($plain_username, $md5_password)))
      {
      ?>
      <p>Welcome!</p> <!--go to account manager here-->
      <?php
      }
    else
      {
      ?>
      <p>Not a valid username and/or password. Please try again.</p>
      <?php
      unset($_POST['username']);
      unset($_POST['password']);
      $ui = new ui();
      $ui->start();
      }
    }



  private function check_credentials($plain_username, $md5_password)
    {
    global $dbcon;

    $userid = $dbcon->prepare('SELECT id FROM users WHERE username = :username AND password = :password LIMIT 1');
    $userid->bindParam(':username', $plain_username);
    $userid->bindParam(':password', $md5_password);
    $userid->execute();

    print_r($dbcon->errorInfo());

    $id = $userid->fetch();
    Return $id;
    }
  }

And if it's any help, here's the class that's calling it:

require_once("ac/acclass.php");
$ac = new ac();
$ac->dbconnect();
class ui
  {
  public function start()
    {
    if ((!isset($_POST['username'])) && (!isset($_POST['password'])))
      {
      $ui = new ui();
      $ui->loginform();
      }
    else
      {
      $ac = new ac();
      $ac->authentication();
      }
    }

  private function loginform()
    {
    ?>
    <form id="userlogin" action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post">
     User:<input type="text" name="username"/><br/>
     Password:<input type="password" name="password"/><br/>
     <input type="submit" value="submit"/>
    </form>
    <?php
    }
  }

Answer 1

Won't solve your problem, but if you are using the classes just as namespaces to group similar functions together, consider using static methods to avoid the overhead of creating an instance.

class Foo {

    static function bar() {

    }

}

Foo::bar()

Answer 2

The default for PDOStatement::fetch is to return a row as an array of fields, indexed by column name and number (mode PDO::FETCH_BOTH). This means ac::check_credentials returns an array, but ac::authentication checks for an integer. Also, field values are strings, so is_int will fail unless you explicitly convert the result field to an integer. Try PDOStatement::fetchColumn() and is_numeric.

    public function authentication() {
        ...
        if (is_numeric($this->check_credentials($plain_username, $md5_password))) {
        ...
    }

    private function check_credentials($plain_username, $md5_password) {
        return $userid->fetchColumn();
    }

As an alternative to is_numeric, check that the result of the credential check isn't identical to False.

    public function authentication() {
        ...
        if (False !== $this->check_credentials($plain_username, $md5_password)) {
        ...
    }

Some Stylistic Points

As Yaggo points out, ui::start and ac::dbConnect should be static methods. ac::authentication doesn't need to create a new ac; since it's not a static method, it can access the current object via the $this special variable (as is done above). $dbcon should be made a static property of ac so you don't pollute the global namespace. Class names should use UpperCamelCase, and should be more descriptive.

Classes should have a single, well-defined purpose and not stray from that. ac has many purposes: manage a DB connection, handle authentication and display results of authentication. Consider designing a set of classes for a Data Access Layer, hiding the database from everything else. Also consider separating the domain logic (authentication &c) from display. This is part of the pattern known as the Model View Controller architecture. This doesn't need to happen all at once; you can slowly refactor the code.