I incorporated the feedback I received from my previous post into a very simple Login class.
Main question:
- How can I design this class and it's implementation so that it looks like it was developed by a seasoned senior level PHP developer that follows all of the best practices? I'm trying to design this class so that it can be reused in other applications. I also have some other questions that are located inside of the code. Thank you in advance for helping!
Please overlook commenting on the following 2 things:
I know I have to learn these!!! but just not yet!! :-)
- Docblock commenting
- Unit testing
Login Class
<?php
class Login
{
private $username;
private $password;
//PDO mysql connection object
private $pdo;
/**
* Question 1.
* Is this the correct way to use an error object? I am passing it in to
* the constructor for DI (Dependency Injection) just like the $pdo
* object? I will have multiple classes that need error management.
*
* Question 2.
* Would each class that uses the error object need to have a method
* called getErrors() as shown below and should I implement an interface
* called 'errors' that has a getErrors() method or should I use an abstract
* class that implements it once so all classes can use it?
*/
private $error;
public function __construct(PDO $pdo, Error $error, $loginCredentials)
{
/**
* Throw exception if missing Login Credentials
*/
if (!isset($loginCredentials['username']) || !isset($loginCredentials['password'])) {
throw new InvalidArgumentException(
"Login credentials must be passed into the ". __CLASS__ . " class in the form of an
associative array with the keys 'username' and 'password'."
);
}
$this->pdo = $pdo;
$this->error = $error;
$this->username = $loginCredentials['username'];
$this->password = $loginCredentials['password'];
}
public function authenticate()
{
/**
* Question 3.
* In each class that uses a PDO connection object,
* is it proper to do all these database interaction details
* in each method that requires database interaction?
*/
$query = "SELECT * FROM admin WHERE user = :user AND password = :pass";
//Prepare statement
$stmt = $this->pdo->prepare($query);
//Set fetch mode
$stmt->setFetchMode(PDO::FETCH_ASSOC);
//Set params to bind
$params = array(
':user'=>$this->username,
':pass'=>sha1($this->password)
);
$stmt->execute($params);
//If the user is found, log them in.
if ($stmt->rowCount()) {
$this->loginUser();
return true;
} else {
/**
* Is this the best way to set errors?
*/
$this->error->set('Invalid Username and or Password combination');
}
}
/**
* Question 4.
* Is this the proper way to display errors back to the user?
* Once a use submits the login form, the client code calls
* authenticate(). If it returns false, it runs this method.
*/
public function getErrors() {
return $this->error->getErrors();
}
private function loginUser()
{
$_SESSION['loggedIn'] = 1;
$this->isLoggedIn = true;
}
}
Error Class
class Error
{
private $errors = array();
public function set($msg)
{
$this->errors[] = $msg;
}
public function getErrors()
{
return $this->format();
}
private function format()
{
if (count($this->errors) > 0) {
$errors = '';
foreach ($this->errors as $error) {
$errors .= '<p>'.$error.'</p>';
}
return $errors;
}
return false;
}
}
Implementation
if (isset($_POST['submit'])) {
/**
* Question 5.
* Looking at the below code, is there a better way so that I can comply
* with the DRY (Don't Repeat Yourself) principle in order to not repeat
* the try{} catch() {} block very time I pass a $pdo object into a class?
*
* Is the proper way accomplished by first creating a new class i.e. PDOConfig
* that extends PDO, and inside the constructor put the try catch block?
*
* If so, is it always better to pass the details into PDOConfig like this:
* $Login = new Login(new PDOConfig(DB_DSN, DB_USER, DB_PASS), new Error(), $_POST)
* or is it better to hard code the constants into the PDOConfig
* class's constructor, and then call it like this:
* $Login = new Login(new PDOConfig(), new Error(), $_POST)
*/
try{
$pdo = new PDO(DB_DSN, DB_USER, DB_PASS);
$pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch(PDOException $e) {
echo "There was an error connecting: " . $e-getMessage();
}
$Login = new Login($pdo, new Error(), $_POST);
if ($Login->authenticate()) {
header("Location: index.php");
exit();
} else {
echo $Login->getErrors();
}
}
