User registration with Repository pattern

Question

I have been developing a simple login registration page with PHP using the repository pattern. Following are the business rules :

During registration

1. Email must be unique.
2. Email must be a valid email address.

User table columns: id, name, username, password, created_at, updated_at

register.php page

if($_POST)
{
    try{

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

        $user_register_obj = new UserRegister(new UserInfoRepository());
        $user_register_obj->register($input);

        header('Location: login.php');
        exit();
    }
    catch(CustomException $e)
    {
        var_dump($e->getCustomMessage());
    }
}

?>

Following is the userinfo interface: This is for the user table. IUserInfo.php

interface IUserInfo {

    public function getId();
    public function getName();
    public function getEmail();
    public function getUserName();
    public function getCreatedAt();
    public function getUpdatedAt();
    public function getPassword();

}

User.php file :

class User implements IUserInfo{

    private $id;
    private $name;
    private $email;
    private $username;
    private $created_at;
    private $updated_at;
    private $password;

    public function __construct($id,
                                $name,
                                $email,
                                $username,
                                $created_at,
                                $updated_at,
                                $password)
    {
        $this->id = $id;
        $this->name = $name;
        $this->email = $email;
        $this->username = $username;
        $this->created_at = $created_at;
        $this->updated_at = $updated_at;
        $this->password = $password;
    }
    public function getEmail()
    {
       return $this->email;
    }

    public function getId()
    {
        return $this->id;
    }

    public function getName()
    {
        return $this->name;
    }

    public function getUserName()
    {
        return $this->username;
    }

    public function getCreatedAt()
    {
        return $this->created_at;
    }

    public function getUpdatedAt()
    {
        return $this->updated_at;
    }
    public function getPassword()
    {
        return $this->password;
    }
}

UserInfoRepository :

interface IUserInfoRepository {

    public function getById($id);
    public function getByEmail($email);
    public function getAll();
    public function insert($input);
    public function update($id, $input);
    public function delete($id);

}

This is the repository class for userinfo :

class UserInfoRepository implements IUserInfoRepository{

    private $table = 'users';
    private $db_obj;

    public function __construct()
    {
        $this->db_obj = new \DatabaseHandler\Database();
    }

    public function getById($id)
    {
        $query = 'select * from users where id = ' . $id;

        return $this->db_obj->GetOneRow($query);
    }

    public function getAll()
    {
        // TODO: Implement getAll() method.
    }

    public function getByEmail($email)
    {
        $query = "select * from user where email = '$email'";

        $obj = $this->db_obj->GetOneRow($query);

        if(!$obj)
        {
            return null;
        }
        else
        {
            return new User($obj['id'], $obj['name'], $obj['email'], $obj['username'], $obj['created_at'], $obj['updated_at'], $obj['password']);
        }
    }

    public function insert($input)
    {
        $name = $input['name'];
        $email = $input['email'];
        $username = $input['username'];
        $password = md5($input['password']);

        $query = "insert into user (name, email, username, password) values ('$name', '$email', '$username', '$password' )";
        return $this->db_obj->InsertAndGetId($query);
    }

    public function update($id, $input)
    {
        // TODO: Implement update() method.
    }

    public function delete($id)
    {
        // TODO: Implement delete() method.
    }
}

UserRegister class :

class UserRegister {

    private  $userRepo;

    public function __construct(\IUserInfoRepository $userRepo)
    {
        $this->userRepo = $userRepo;
    }

    public function register($input)
    {
        $email = $input['email'];

        $result = filter_var( $email , FILTER_VALIDATE_EMAIL );
        if(!$result)
            throw new \CustomException("Invalid Email");

        $obj = $this->userRepo->getByEmail($email);
        if(!is_null($obj))
            throw new \CustomException("Email already found");

        $this->userRepo->insert($input);

        return true;
    }
}

Please ignore the database handler Database right now. That class seems fine and I am not posting it as it may make it more complex.

Can you please review my code design ? I will accept answer in any languages.

Answer 1

Security

SQL Injection

You are (very likely) open to SQL injection, as you put user input directly into SQL queries. You need to use prepared statements instead. This goes for all statements (select, insert, ...).

Hashing

You also have to use a proper password hashing function. md5 hasn't been acceptable for at least 15 years. Use password_hash instead.

Misc

Structure

• your interfaces are unneeded, as they are unlikely to be used in different situations. Your IUserInfoRepository may work as a generic IRepository though, as likely other objects need to be selected, inserted, etc. Your IUserInfo interface could partly work as an IBaseObject interface which contains getId and possibly getCreatedAtand getUpdatedAt (if these are values that you store for all objects).
• Your getById function returns an array, while your getByEmail function returns a User. This should be handled the same way.
• You should pass on the specific required fields to insert, not just some input array. Arrays are bad for usability, as a user of your class would need to read the documentation, or in your case - as you don't have any - actually look at the code.

Other

• add PHPDoc style comments to your functions to explain what the arguments need to be, what the return values are, etc.
• I would throw an exception if an email doesn't exist instead of returning null, to avoid excessive null checks.
• always use curly brackets
• you don't need one-time variables such as $email = $input['email'];.
• upper-case your SQL keywords to increase readability
• be less generic with your variable names. obj could be userData, query could be userSelectQuery, etc.

Answer 2

I see a little bit weird the use of a in interface on the model of a class (IUserInfo). Because it's unlikely that you're going to swap-out the model, it does not have any implementation details. Also you should not make interfaces with functions that you are not going to implement, like a template interface for all your repositories, and use the same for everyone, and have unimplemented/unused functions.

Also, you could change the names of the variables in this function for something more descriptive, instead of result and obj, you could use userEmailAfterValidation for example.

    public function register($input)
{
    $email = $input['email'];

    $result = filter_var( $email , FILTER_VALIDATE_EMAIL );
    if(!$result)
        throw new \CustomException("Invalid Email");

    $obj = $this->userRepo->getByEmail($email);
    if(!is_null($obj))
        throw new \CustomException("Email already found");

    $this->userRepo->insert($input);

    return true;
}