PHP DataMapper and OOP Design

Question

I'm creating a small app to learn OOP PHP but I'm getting confused with some patterns and with my design. I'm not sure if I'm doing it right. I would really appreciate any advice on my DataMapper and general OOP design.

interface Authenticator
{
    public function authenticate(UserCredentials $userCredentials);
}

class LoginAuthenticator implements Authenticator
{
    private $userMapper;

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

    public function authenticate(UserCredentials $userCredentials)
    {
        if (!$user = $this->userMapper->findByUsernameAndPassword($userCredentials->getUsername(), $userCredentials->getPassword()))
        {
            throw new InvalidCredentialsException('Invalid username or password!');
        }

        return $user;
    }
}

class UserCredentials
{
    private $username;
    private $password;

    public function getUsername()
    {
        return $this->username;
    }
    public function setUsername($username)
    {
        if (!is_string($username) || strlen($username) < 3)
        {
            throw new InvalidArgumentException('Invalid username.');
        }

        $this->username = $username;
    }

    public function getPassword()
    {
        return $this->password;
    }
    public function setPassword($password, Encryptor $encryptor)
    {
        if (!is_string($password) || strlen($password) < 8)
        {
            throw new InvalidArgumentException('Invalid password.');
        }

        $this->password = $encryptor->encrypt($password);
    }
}

class User
{
    private $id;
    private $firstName;
    private $lastName;
    private $email;
    private $username;
    private $password;

    public function getPassword()
    {
        return $this->password;
    }
    public function setPassword($password, Encryptor $encryptor)
    {
        $this->password = $encryptor->encrypt($password);
    }

    //more getters and setters
}

interface UserMapper
{
    public function insert(User $user);
    public function update(User $user);
    public function delete($id);
    public function findByUsernameAndPassword($username, $password);
    public function findAll();
}

class PdoUserMapper implements UserMapper
{
    private $pdo;
    private $table = 'users';

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

    public function insert(User $user)
    {
        $statement = $this->pdo->prepare("INSERT INTO {$this->table} VALUES(null, ?, ?, ?, ?)");

        $userValues = array(
            $user->getFirstName(),
            $user->getLastName(),
            $user->getEmail(),
            $user->getUsername(),
            $user->getPassword()
        );

        $statement->execute($userValues);

        return $this->pdo->lastInsertId();
    }

    public function update(User $user)
    {
        $statement = $this->pdo->prepare("UPDATE {$this->table} SET name = ?, last_name = ?, email = ?, password = ? WHERE id = ?");

        $userValues = array(
            $user->getFirstName(),
            $user->getLastName(),
            $user->getEmail(),
            $user->getPassword(),
            $user->getId()
        );

        $statement->execute($userValues);
    }

    public function delete($id)
    {
        $statement = $this->pdo->prepare("DELETE FROM {$this->table} WHERE id = ?");
        $statement->bindValue(1, $id);
        $statement->execute();
    }

    public function findById($id)
    {
        $statement = $this->pdo->prepare("SELECT * FROM {$this->table} WHERE id = ?");
        $statement->bindValue(1, $id);

        if (!$result = $statement->execute())
        {
            return null;
        }

        $user = new User();
        $user->setId($result['id']);
        $user->setFirstName($result['name']);
        $user->setLastName($result['last_name']);
        $user->setUsername($result['username']);
        $user->setEmail($result['email']);

        return $user;
    }

    public function findByUsernameAndPassword($username, $password)
    {
        $statement = $this->pdo->prepare("SELECT * FROM {$this->table} WHERE username = ? AND password = ?");
        $statement->bindValue(1, $username);
        $statement->bindValue(2, $password);
        $statement->execute();

        if (!$result = $statement->fetch())
        {
            return null;
        }

        $user = new User();
        $user->setId($result['id']);
        $user->setFirstName($result['name']);
        $user->setLastName($result['last_name']);
        $user->setEmail($result['email']);
        $user->setUsername($result['username']);
        $user->setPassword($result['password'], new MD5());

        return $user;
    }

    public function findAll()
    {
        $statement = $this->pdo->query("SELECT * FROM {$this->table}");

        while ($result = $statement->fetch(PDO::FETCH_ASSOC))
        {
            $user = new User();
            $user->setId($result['id']);
            $user->setFirstName($result['name']);
            $user->setLastName($result['last_name']);
            $user->setUsername($result['username']);
            $user->setEmail($result['email']);

            $userCollection[] = $user;
        }

        return $userCollection;
    }
}

//login
try
{
    $userCredentials = new UserCredentials();
    $userCredentials->setUsername($_POST['username']);
    $userCredentials->setPassword($_POST['password'], new MD5());

    $pdo = new PDO('mysql:host=localhost;dbname=database', 'root', '');

    $loginAuth = new LoginAuthenticator(new PdoUserMapper($pdo));
    $user = $loginAuth->authenticate($userCredentials);

    $session = new Session();
    $session->setValue('user', $user);

    header('Location: index.php');
}
catch (InvalidArgumentException $e)
{
    echo $e->getMessage();
}
catch (PDOException $e)
{
    echo $e->getMessage();
}
catch (InvalidCredentialsException $e)
{
    echo $e->getMessage();
}

//inserting new user
$pdo = new PDO('mysql:host=localhost;dbname=database', 'root', '');

$user = new User;
$user->setFirstName($_POST['name']);
$user->setLastName($_POST['last_name']);
$user->setEmail($_POST['email']);
$user->setPassword($_POST['password'], new MD5());

$userMapper = new PdoUserMapper($pdo);
$userMapper->insert($user);

What do you think about my DataMapper design ? Is it correct ? What should I change ?

Another thing that concerns me is the need to pass an Encryptor two times: to the User::setPassword method and to UserCredentials::setPassword method. If I need to change my encryption algorithm, I'll have to change it in more than one place, what leads me to think that I'm making something wrong.

Answer 1

When invoking setPassword() you're actually going to set a password hash, not a password itself. And also this one does more than one thing - it knows how to assign a password hash and also knows how to encrypt it.

So, with a current setPassword() method you have two problems:

1) setPassword() name's implies that you're going to set a password, not its hash. Therefore it breaks the POLS

2) Since the method also knows how to encrypt a password, the encryption algorithm becomes tightly coupled to it.

So, I'd recommend going this way

// Only once!
$passwordHash = md5($_POST['password'])

$userCredentials->setUsername($_POST['username']);
$userCredentials->setPasswordHash($passwordHash);

....

$user->setPasswordHash($passwordHash);

This makes you hashing algorithm completely decoupled from authentication, thus when changing it, it wouldn't hurt so much.

In general I'd say - your design is OK.