I'm working on the registration process for new users of a small web app, and as this is largely a learning project I'm doing it from scratch.
Here is my user class: Class_user.php
<?php
class User {
// The class variables are the same and have the same name as the db fields.
private $userID; // Must be incremental + unique. Also used as salt.
private $userName; // Must be unique.
private $hashedPassword;
private $userEmail;
function __construct($userID, $userName, $hashedPassword, $userEmail){
$this->userID=$userID;
$this->userName=$userName;
$this->hashedPassword=$hashedPassword;
$this->userEmail=$userEmail;
}
function getUserID(){
return $this->userID;
}
function getUserName() {
return $this->userName;
}
function getHashedPassword() {
return $this->hashedPassword;
}
function getUserEmail() {
return $this->userEmail;
}
}
?>
Here is my model for the user - Model_User.php:
<?php
require_once('Class_DB.php');
require_once('Class_User.php');
require_once('DAO_User.php');
class Model_user{
// Object that represents a connection to the user DB
private $dbInstance;
function __construct($dbInstance){ // Using Dependency Injection passing a Class_DB object rather than global variable.
$this->dbInstance=$dbInstance;
}
function insertNewUser($user, $userPassword){ // INCOMPLETE
$userDAO=new DAO_User($this->dbInstance);
$insertedUser=$userDAO->createNewUser($user, $userPassword);
$userPassword=""; // We clear the user's password from memory.
if ($insertedUser){ // User was correctly inserted in db
return true; // Should return the $user object.
} else {
return false;
}
}
}
?>
Here is the key snippet from my user registration controller, Controller_Register.php
<?php
require_once("Controller.php");
require_once("Model_User.php");
require_once("General.php");
class Controller_Register extends Controller {
protected $page='UI_Register.php';
function execute($view){
// Several lines to make sure the form is filled adequately.
While (!$userInserted){ // As long as the user isn't correctly inserted...
$userName=$_POST["userName"];
$userEmail=$_POST["userEmail"];
$userPassword=$_POST["userPassword"];
$_POST["userPassword"]=""; // We don't keep the pw in memory.
$user=new user("", $userName, "", $userEmail); // User ID will be generated by the db, and the hashed pw has not been generated at this point.
$userInserted=$userDBConnection->insertNewUser($user,$userPassword); // We insert the user name not knowing what the autoincremented user ID is.
$userPassword=""; // We don't keep the pw in memory.
}
$_POST["password"]=""; // We clear the user's password from memory.
if ($userInserted){// The value is true if the registration was succesful.
$msg=new Message("Congratulations ".$_POST['userName']."! You are now registered.");
}
}
return $view;
}
?>
Finally, here is my user DAO code, with the key item I would like guidance on, the registration - DAO_User.php
<?php
require_once('Class_User.php');
require_once('Class_DB.php');
require_once('General.php');
class DAO_User {
private $dbInstance; // This is an instance of Class_DB to be injected in the functions.
function __construct($dbInstance){
$this->dbInstance=$dbInstance; // Using Dependency Injection passing a Class_DB object rather than global variable.
}
function createNewUser($user, $userPassword){ // The $user object only has a userName and a userEmail at this point.
$dbConnection=$this->dbInstance->createConnexion(); // This connection is local, so automatically closed after the function is completed.
$inserted=false;
while (!$inserted){ // This insert a new user, without any value for pw, and generates an autoincrement user ID on the db side.
$query=$dbConnection->prepare("INSERT INTO users (userName, userEmail) VALUES (?,?)"); // userID is generated via autoincrement - therefore not known at time of insertion.
$query->bindValue(1, $user->userName);
$query->bindValue(2, $user->userEmail);
$inserted=$query->execute(); //True if succesful, False if not.
}
$query=$dbConnection->prepare("SELECT LAST_INSERT_ID()"); // This allows us to retrieve the user ID as generated by the db.
$userIDquery=$query->execute();
$result=$userIDquery->fetch(PDO::FETCH_ASSOC); // returns an array indexed by column name as returned in result set - here column name is "userID" in the DB
$userID=$result["userID"];
$user->userID=$userID; // We modify the user ID of the $user object to be the autoincremented number generated by the db.
$hashedPassword=stringHashing($userPassword,$user->userID);
$userPassword="";
$user->hashedPassword=hashedPassword;
$hashedPWinserted=false;
while (!$hashedPWinserted){ // This modifies the user table in db to add hashed PW.
$query=$dbConnection->prepare("UPDATE users SET hashedPassword=? WHERE userID=?");
$query->bindValue(1, $user->hashedPassword);
$query->bindValue(2, $user->userID);
}
return $user;
}
}
?>
The General.php code contains the stringHashing function that receives a string and a salt as parameter and and returns the salted string. I'm not sure where it should live in an MVC framework.
My users table is a MySQL table with 4 fields:
- userID (INT(10), not null, autoincrement, PK) - also used as a salt for PW hashing
- userName (varchar(50), not null)
- hashedPassword (char(128), can be null)
- userEmail (varchar(255), can be null)
Some specific questions I have, mostly on the createNewUser function:
- Are the db transactions correctly and efficiently done?
- Should I split some functionalities outside of this function?
- Should I limit the use of intermediate variables?
- Does the function accomplish the goals I want it to do, i.e., insert in the db a new user with an autoincremented user ID generated by the db, and then a hashed password?
Also interested in any other piece of feedback people may have, especially with regards to readability (e.g., are my commentaries too verbose and obvious) and understandability of my code, as well as best practice object programming (e.g., I suppose I should modify my $user object with a setter rather than $user->userID=$userID;).
UPDATE When I run this code, I get no error, but I also don't get any record in the db.
