I've been a PHP procedural programmer for several years but I'm trying to learn OOP and I'm getting a bit confused with some patterns and principles. I would appreciate it if you could give me some tips and advice.
interface LoginAuthenticator
{
public function authenticate(UserMapper $userMapper);
}
class UserAuthenticator implements LoginAuthenticator
{
private $user;
private $session;
public function __construct(User $user, Session $session)
{
$this->user = $user;
$this->session = $session;
}
public function authenticate(UserMapper $userMapper)
{
if (!$user = $userMapper->findByUsernameAndPassword($this->user->getUsername(), $this->user->getPassword()))
{
throw new InvalidCredentialsException('Invalid username or password!');
}
$this->logUserIn($user);
}
private function logUserIn(User $user)
{
$this->session->setValue('user', $user);
}
public function logUserOut()
{
$this->session->unsetValue('user');
$this->session->destroy();
}
}
try
{
$user = new User();
$user->setUsername($_POST['username']);
$user->setPassword($_POST['password'], new MD5());
$pdo = new PDO('mysql:host=localhost;dbname=database', 'root', '');
$userAuth = new UserAuthenticator($user, new Session());
$userAuth->authenticate(new PdoUserMapper($pdo));
header('Location: index.php');
}
catch (InvalidArgumentException $e)
{
echo $e->getMessage();
}
catch (PDOException $e)
{
echo $e->getMessage();
}
catch (InvalidCredentialsException $e)
{
echo $e->getMessage();
}
Well, here is my first concern, the SRP: I don't really know if I should inject a Mapper into my UserAuthenticator::authenticate method or if I should create a UserFinder class and inject it instead. I don't know if it's a Mapper responsibility to find. What do you think ?
Furthermore, I'm also confused about the $user property: my findByUsernameAndPassword method returns a User object, so I have two Users instances in the same class: one injected and another returned by the Mapper. Should I inject just $username and $password instead of a User object in order to authenticate ?
I have also some wrappers classes like Session and MD5 but they are not needed to understand how my classes works.
Edit
My classes after changes and user related classes:
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;
}
}
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();
}
One thing that is bothering 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 still making something wrong.
