Problem
I’d like you to review my attempt at creating an authorization class and let me know if there is re-factoring to be done and/or how I can make the code cleaner and abide by the MVC structure.
UsersController.php
<?php
class UsersController extends BaseController {
protected $user;
public function __construct(User $user)
{
$this->beforeFilter('csrf', array('on' => 'post'));
$this->user = $user;
}
public function getRegister()
{
return View::make('admin.register');
}
public function postRegister()
{
try
{
$this->user->insertUserIntoDatabase(Input::all());
}
catch (ValidationError $e)
{
return Redirect::to('admin/register')
->with('message', 'Something went wrong')
->withInput()
->withErrors($e->getErrors());
}
return Redirect::to('admin/login')
->with('message', 'Thank you for creating a new account');
}
public function getLogin()
{
return View::make('admin.login');
}
public function postLogin()
{
if (Auth::attempt(array('email' => Input::get('email'), 'password' => Input::get('password') ))){
return Redirect::to('admin/dashboard')->with('message', 'Thanks for logging in');
}
return Redirect::to('admin/login')->with('message', 'Your email/password combo failed.');
}
public function getLogout()
{
Auth::logout();
return Redirect::to('admin/login')->with('message', 'You have been logged out.');
}
public function getDashboard()
{
return View::make('admin/dashboard');
}
}
Users.php (Model)
<?php
use IlluminateAuthUserInterface;
use IlluminateAuthRemindersRemindableInterface;
class User extends Eloquent implements UserInterface, RemindableInterface {
/**
* The database table used by the model.
*
* @var string
*/
protected $table = 'users';
/**
* The attributes excluded from the model's JSON form.
*
* @var array
*/
protected $hidden = array('password');
protected $fillable = array('firstname', 'lastname', 'email');
/**
* Get the unique identifier for the user.
*
* @return mixed
*/
public function getAuthIdentifier()
{
return $this->getKey();
}
/**
* Get the password for the user.
*
* @return string
*/
public function getAuthPassword()
{
return $this->password;
}
/**
* Get the e-mail address where password reminders are sent.
*
* @return string
*/
public function getReminderEmail()
{
return $this->email;
}
public function insertUserIntoDatabase($input)
{
$validation = new ServicesValidatorsUsers;
if ($validation->passes()) {
$user = new User;
$user->firstname = Input::get('firstname');
$user->lastname = Input::get('lastname');
$user->email = Input::get('email');
$user->password = Hash::make(Input::get('password'));
$user->save();
}
else {
$this->errors = $validation->errors;
throw new ValidationError($validation->errors);
}
}
}
Solution
I didn’t used PHP in the last few years, so just some generic notes:
-
The
$input
parameter ofinsertUserIntoDatabase($input)
is unused. I guess you wanted to use the parameter instead of directInput::
calls inside the function. -
You could use a guard clause in
insertUserIntoDatabase
to make the code flatten:public function insertUserIntoDatabase($input) { $validation = new ServicesValidatorsUsers; if (!$validation->passes()) { $this->errors = $validation->errors; throw new ValidationError($validation->errors); } $user = new User; $user->firstname = Input::get('firstname'); $user->lastname = Input::get('lastname'); $user->email = Input::get('email'); $user->password = Hash::make(Input::get('password')); $user->save(); }
-
The fields in the User class are
protected
. You might want to useprivate
instead. See: Should I always use the private access modifier for class fields?; Item 13 of Effective Java 2nd Edition: Minimize the accessibility of classes and members. -
In
postRegister()
you could move the return inside the try block. I guessRedirect::to
doesn’t throw anyValidationError
exceptions, so the following is the same:public function postRegister() { try { $this->user->insertUserIntoDatabase(Input::all()); return Redirect::to('admin/login') ->with('message', 'Thank you for creating a new account'); } catch (ValidationError $e) { return Redirect::to('admin/register') ->with('message', 'Something went wrong') ->withInput() ->withErrors($e->getErrors()); } }
-
if (Auth::attempt(array('email' => Input::get('email'), 'password' => Input::get('password') ))){ return Redirect::to('admin/dashboard')->with('message', 'Thanks for logging in'); }
I would create an explanatory local variable for the array for better readability:
$credentials = array( 'email' => Input::get('email'), 'password' => Input::get('password') ); if (Auth::attempt($credentials)) { return Redirect::to('admin/dashboard')->with('message', 'Thanks for logging in'); }
(Clean Code by Robert C. Martin, G19: Use Explanatory Variables; Refactoring: Improving the Design of Existing Code by Martin Fowler, Introduce Explaining Variable)