Database, User and Validator in PHP

Question

Please review my classes and give me some advice to improve them. I have also put some additional questions in the code and I hope you can give me an answer for some of them.

Database.php

<?php
class Database 
{
    private static $dsn = 'mysql:host=localhost;dbname=knight;charset=UTF-8';
    private static $user = 'root';
    private static $pass = '';
    private static $instance = null;

    private function __construct()
    {}

    //is PDO+Singleton a good combination?
    public static function getInstance()
    {
        if(!isset(self::$instance))
        {
            try 
            {
                self::$instance = new PDO(self::$dsn, self::$user, self::$pass);
                self::$instance->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            }
            catch (PDOException $e)
            {
                echo 'Error found: '.$e->getMessage();
            }
        }
        return self::$instance;
    }

    //Should i make methots prepare(), execute(), bindValue() etc. and why?
}
?>

Validator.php

<?php
class Validator
{
    //All the methods are static, is this a good practice for a class like this?

    public static function isValidMail($var)
    {
        return preg_match("/([\w\-]{3,40}+\@[\w\-]{3,120}+\.[\w\-]{2,6}+)/", $var);
    }

    public static function isLong($var, $max)
    {
        return(mb_strlen($var, 'UTF-8') > $max);
    }

    public static function isShort($var, $min)
    {
        return (mb_strlen($var, 'UTF-8') < $min);
    }

    public static function isValidID($var)
    {
        $var = (int)$var;
        return ($var > 0);
    }
}

User.php

<?php
class User 
{
    //Should I have properties such as username, password, email etc. and why?

    private $db = null;
    private $errors = array();

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

    public function register($username, $pass, $mail)
    {
        if($this->validate($username, $pass, $mail))
        {
            $username = $this->filter($username);
            $mail = $this->filter($mail);
            $exists = $this->exists($username, $mail);
            if(!$exists)
            {
                $pass = $this->hash($pass);
                $time = time();

                //Should I use exceptions for the SQL queries in this class?

                $rs = $this->db->prepare("INSERT INTO users(username, mail, pass, date_reg) VALUES(:name, :mail, :pass, :time)");
                $rs->execute(array(':name' => $username, ':mail' => $mail, ':pass' => $pass, ':time' => $time));
                header('Location: index.php');
                die();
            }
            else
            {
                $this->errors[] = 'Username or email already exists!';
            }
        }

        if(count($this->errors > 0))
        {
            $this->showErrors();
        }
    }

    public function login($username, $pass)
    {
        if($this->validate($username, $pass))
        {
            $username = $this->filter($username);
            $exists = $this->exists2($username, $pass);
            if($exists)
            {
                $_SESSION['logged'] = 1;
                $_SESSION['data'] = $exists->fetch(PDO::FETCH_ASSOC);
                header('Location: index.php');
                die();
            }
            else
            {
                $this->errors[] = 'Wrong username or password!';
            }
        }

        if(count($this->errors > 0))
        {
            $this->showErrors();
        }
    }

    public function exists($username, $mail)
    {
       $exists = $this->db->prepare("SELECT user_id FROM users WHERE username = :name OR mail = :mail");
       $exists->execute(array(':name' => $username, ':mail' => $mail));
       return ($exists->rowCount() > 0);
    }

    public function exists2($username, $pass)
    {
       $pass = $this->hash($pass);
       $exists = $this->db->prepare("SELECT * FROM users WHERE username = :name AND pass = :pass");
       $exists->execute(array(':name' => $username, ':pass' => $pass));
       if($exists->rowCount() > 0) return $exists;
       return false;
    }

    public function validate($username, $pass, $mail = null)
    {
        if(Validator::isShort($username, 4)) $this->errors[] = 'Short username!';
        elseif(Validator::isLong($username, 25)) $this->errors[] = 'Long username!!';
        if(Validator::isShort($pass, 5)) $this->errors[] = 'Short password!';
        if($mail !== null)
        {
            if(!Validator::isValidMail($mail))
            {  
               $this->errors[] = 'Invalid email!';
            }
        }
        return (count($this->errors) == 0);
    }

    public function getUserData($id)
    {
        if(Validator::isValidID($id))
        {
            $rs = $this->db->prepare("SELECT * FROM users WHERE user_id = ?");
            $rs->execute(array($id));
            if($rs->rowCount() == 1) return $rs->fetch(PDO::FETCH_ASSOC);
            return false;
        }
        return false;
    }

    private function hash($var)
    {
        return hash('sha512', $var);
    }


    public function filter($var)
    {
        //Is htmlentities enough if I use PDO?
        return htmlentities($var, ENT_QUOTES, 'UTF-8');
    }


    private function showErrors()
    {
        foreach($this->errors as $e)
        {
            //I am outputting text in the class, is that bad? 
            echo '<strong>'.$e.'</strong><br />';
        }
    }
}

Answer 1

Let's first create some test code to show how this might all work to say register a new user. In your controller code to register a user which would accept POST :

$name = $_POST['name'];
$password = $_POST['password'];
$email = $_POST['email'];

$db = Database::getInstance();
$user = new User($db);
$user->register($name, $password, $email);

Right off the bat, I see that I have to pass name, pw, email to the register method. Some improvement:

$name = $_POST['name'];
$password = $_POST['password'];
$email = $_POST['email'];

$db = Database::getInstance();
$user = new User($db);
$user->setName($name);
$user->setPassword($password);
$user->setEmail($email);

if ($user->register() !== true) {
    //show some sort of error page, utilize the $user object in your view
    //i.e. foreach ($user->getErrors() as $error)...
} else {
    //show the success result page, utilize the $user object in your view
}

The register method can perform validations as necessary and insert the appropriate db record.

Onto the Database class. Hard coding the connection parameters is not very flexible. There's no real behavior behind your Database class other than a factory method to create an instance of itself. I think you'd be better off with a PDOFactory class which can accept a configuration (your db connection params) and return a single instance of a PDO object. Then, use the PDO object where needed.

The Validation class could be extended to provide what you now have for static methods:

abstract class Validator
{
    protected $value;

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

    abstract public function isValid();
}

class EmailValidator extends Validator
{
    public function isValid()
    {
        //implement your validation code and return true or false
    }
}

Create subclasses for the rest.

All this said, you should know that there are several frameworks which implement all of this already. I strongly recommend you look at either Zend Framework or Symfony. They've already thought of everything you are asking about here and more.