PHP user class (login/logout/signup)

Question

Started experimenting with building classes, and I've began by converting my user registration/login into a single class. Wanted to stop and ask for feedback before getting too far.

class UserService {     private $_email;     private $_password;      public function login($email, $password)     {         $this->_email = mysql_real_escape_string($email);         $this->_password = mysql_real_escape_string($password);          $user_id = $this->_checkCredentials();         if($user_id){             $_SESSION['user_id'] = $user_id;             return $user_id;         }         return false;     }      protected function _checkCredentials()     {         $query = "SELECT *                     FROM users                     WHERE email = '$this->_email'";         $result = mysql_query($query);         if(!empty($result)){             $user = mysql_fetch_assoc($result);             $submitted_pass = sha1($user['salt'] . $this->_password);             if($submitted_pass == $user['password']){                 return $user['id'];             }         }         return false;     }    } 

One of the questions I do have related to my class is: should I be building it as this:

$User = new UserService(); $User->login($_POST['email'], $_POST['password']); 

Where the login method calls the _checkCredentials method automatically. Or should it be built like:

$User = new UserService(); $UserId = $User->checkCredentials($_POST['email'], $_POST['password']); $User->login($UserId); 

Other than that I've love some tips on how to restructure this and please point out anything I'm doing wrong!

thanks guys

Answer 1

I think your main idea was to separate the user handling (session) from the database query, which is a good thing in my opinion.

However, this is not the case with your actual implementation, because login escapes the data to be sent to the database, even if the rest of the method does not having anything to do with databases. Not to say that your database query depends on a global resource to work. While I'm at it, I will also suggest you use PDO.

Also, your properties $_email and $_password are in the private scope, but are to be accessed by a protected method. This may cause problems. The properties and the method should have equivalent visibility.

Now, I can see that your UserService requires three things: a database handler, an email and a password. It would make sense to put it in a constructor.

Here's how I would do it:

class UserService {     protected $_email;    // using protected so they can be accessed     protected $_password; // and overidden if necessary      protected $_db;       // stores the database handler     protected $_user;     // stores the user data      public function __construct(PDO $db, $email, $password)      {        $this->_db = $db;        $this->_email = $email;        $this->_password = $password;     }      public function login()     {         $user = $this->_checkCredentials();         if ($user) {             $this->_user = $user; // store it so it can be accessed later             $_SESSION['user_id'] = $user['id'];             return $user['id'];         }         return false;     }      protected function _checkCredentials()     {         $stmt = $this->_db->prepare('SELECT * FROM users WHERE email=?');         $stmt->execute(array($this->email));         if ($stmt->rowCount() > 0) {             $user = $stmt->fetch(PDO::FETCH_ASSOC);             $submitted_pass = sha1($user['salt'] . $this->_password);             if ($submitted_pass == $user['password']) {                 return $user;             }         }         return false;     }      public function getUser()     {         return $this->_user;     } } 

Then use it as such:

$pdo = new PDO('mysql:dbname=mydb', 'myuser', 'mypass');  $userService = new UserService($pdo, $_POST['email'], $_POST['password']); if ($user_id = $userService->login()) {     echo 'Logged it as user id: '.$user_id;     $userData = $userService->getUser();     // do stuff } else {     echo 'Invalid login'; }