Just starting to learn OOP and first off I have to say I can (finally) see why this is more useful than procedural. Anyway, took this script off of a site, made some very small edits (still more to make). But I was just wondering if anything is wrong (bad practices, code leading to errors, etc)? And how would I make it so the database is checked during/after registration for users with the same username (i.e no two people with the same username/password).
Classes:
PHP Code:
<?php
include "/includes/database.php";
class Users {
private $username;
private $password;
private $email;
private $salt = "Zo4rU5Z1YyKJAASY0PT6EUg7BBYdlEhPaNLuxAwU8lqu1ElzHv0Ri7EM6irpx5w";
//Get user information
public function __construct($data = array()) {
if (isset($data['username'])) {
$this->username = mysqli_real_escape_string($data['username']);
}
if (isset($data['password'])) {
$this->password = mysqli_real_escape_string($data['password']);
}
if (isset($data['email'])) {
$this->email = mysqli_real_escape_string($data['email']);
}
public function storeFormValues($params) {
$this->__construct($params);
}
}
public function userLogin() {
//success variable will be used to return if the login was successful
$sucess = false;
try {
//create our pdo object
$con = new PDO(DB_DSN, DB_USERNAME, DB_PASSWORD);
//set how pdo will handle errors
$con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
//this would be our query
$sql = "SELECT * FROM `users` WHERE `username` = :username AND `password` = :password LIMIT 1";
//prepare the statements
$stmt = $con->prepare($sql);
//give value to named parameter :username
$stmt->bindValue("username", $this->username, PDO::PARAM_STR);
//give value to named parameter :password
$stmt->bindValue("password", hash("sha256", $this->password . $this->salt), PDO PARAM_STR);
$stmt->execute();
$valid = $stmt->fetchColumn(); //Set $_SESSION variables
if ($valid) {
$success = true;
$_SESSION['loggedin'] = 1;
$mem = $stmt->fetchColumn();
$_SESSION['username'] = $mem['username'];
$_SESSION['userid'] = $mem['id'];
$_SESSION['level'] = $mem['level'];
$ip = ($_SERVER['HTTP_X_FORWARDED_FOR']) ? $_SERVER['HTTP_X_FORWARDED_FOR'] : $_SERVER['REMOTE_ADDR'];
}
$con = null;
return $success;
} catch (PDOException $e) {
echo $e->getMessage();
return $success;
}
}
public function register() {
$correct = false;
try {
$con = new PDO(DB_DSN, DB_USERNAME, DB_PASSWORD);
$con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$sql = "INSERT INTO `users`(username, password) VALUES(:username, :password)";
<input type="hidden" name="submitted" value="1" />
<input type="submit" name="submit" value="Register" />
</fieldset>
</form>
<?php
$usr = new Users; //create new instance of the class Users
$usr->storeFormValues($_POST['username'], $_POST['password'], $_POST['email']); //store form values
//if the entered password is match with the confirm password then register him
if ($_POST['password'] == $_POST['cpw']) {
echo $usr->register($_POST);
} else {
//if not then say that the user must enter the same password to the confirm box
echo "<p>Password and Confirm Password fields do not match</p>";
}
?>
__________________ Coding is a challenge, get used to it Always remember to debug Try the guess & check method Break it down into simple steps
if (isset($data['username'])) {
$this->username = mysqli_real_escape_string($data['username']);
}
You shouldn't be storing information in an escaped format. That should be used only during write to a mysql database. You are using PDO though, so you shouldn't be doing anything with MySQLi if that's the intent. Binding doesn't require, and nor should it be given an escaped string as it will corrupt the original value of the string.
This makes no sense:
PHP Code:
public function storeFormValues($params) { $this->__construct($params); }
I don't see a point of calling a constructor on an existing instance of an object. If you need to modify something, do so at the property level; __construct shouldn't be explicitly invoked and should only be used when instantiating a parent constructor or when implicitly called by the new keyword.
if (isset($data['username'])) {
$this->username = mysqli_real_escape_string($data['username']);
}
You shouldn't be storing information in an escaped format. That should be used only during write to a mysql database. You are using PDO though, so you shouldn't be doing anything with MySQLi if that's the intent. Binding doesn't require, and nor should it be given an escaped string as it will corrupt the original value of the string.
This makes no sense:
PHP Code:
public function storeFormValues($params) {
$this->__construct($params);
}
I don't see a point of calling a constructor on an existing instance of an object. If you need to modify something, do so at the property level; __construct shouldn't be explicitly invoked and should only be used when instantiating a parent constructor or when implicitly called by the new keyword.
Ah, was wondering about using mysqli AND PDO together.
So, just:
PHP Code:
if (isset($data['username'])) {
$this->username = $data['username'];
}
then?
As for the constructor part, I was actually wondering about that as well when I was looking at the script. I assumed it was sending the values to the storeFormValue(), which then sent them to the constructor, which as you said would be smarter/better/whatever you wanna say to send them straight to the constructor. Methods are class functions and properties are class variables, correct? Or can everything within a class be considered a method?
__________________ Coding is a challenge, get used to it Always remember to debug Try the guess & check method Break it down into simple steps
Only functions are methods.
The constructor chain appears to me to be a (lazy at that) short cut. On a side note, doing it the other way around completely would be acceptable solution that doesn't require internally constructing new objects. The constructor can certainly call any method it wants, so if you have an overwriting method, chain the constructor to that instead of the other way around.
Yeah, PDO and MySQLi are not compatible. Since you need to have MySQLi established to make use of a mysql_real_escape_string anyway, you should choose which to use. But more importantly properties should never be stored in an escaped format, regardless of if you intend to use a prepared statement bound variable or if you intend to use a simple string with escaped input data. Escaping is specific for the db, not for the data in question. It is completely legal for me to have a string with \' in the string, but by escaping it you have now corrupted it onto \\\' instead.
<input type="hidden" name="submitted" value="1" />
<input type="submit" name="submit" value="Register" />
</fieldset>
</form>
<?php
$usr = new Users($_POST['username'], $_POST['password'], $_POST['email']); //create new instance of the class Users
//if the entered password is match with the confirm password then register him
if ($_POST['password'] == $_POST['cpw']) {
echo $usr->register($_POST['username'], $_POST['password'], $_POST['email']);
} else {
//if not then say that the user must enter the same password to the confirm box
echo "<p>Password and Confirm Password fields do not match</p>";
}
?>
</div>
</div>
</body>
</html>
classes:
PHP Code:
<?php
include "/includes/database.php";
class Users {
private $username;
private $password;
private $email;
private $salt = "Zo4rU5Z1YyKJAASY0PT6EUg7BBYdlEhPaNLuxAwU8lqu1ElzHv0Ri7EM6irpx5w";
//Get user information
public function __construct($data = array()) {
if (isset($data['username'])) {
$this->username = $data['username'];
}
if (isset($data['password'])) {
$this->password = $data['password'];
}
if (isset($data['email'])) {
$this->email = $data['email'];
}
}
public function userLogin() {
//success variable will be used to return if the login was successful
$sucess = false;
try {
//create our pdo object
$con = new PDO(DB_DSN, DB_USERNAME, DB_PASSWORD);
//set how pdo will handle errors
$con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
//this would be our query
$sql = "SELECT * FROM `users` WHERE `username` = :username AND `password` = :password LIMIT 1";
//prepare the statements
$stmt = $con->prepare($sql);
//give value to named parameter :username
$stmt->bindValue("username", $this->username, PDO::PARAM_STR);
//give value to named parameter :password
$stmt->bindValue("password", hash("sha256", $this->password . $this->salt), PDO PARAM_STR);
$stmt->execute();
$valid = $stmt->fetchColumn(); //Set $_SESSION variables
if ($valid) {
$success = true;
$_SESSION['loggedin'] = 1;
$mem = $stmt->fetchColumn();
$_SESSION['username'] = $mem['username'];
$_SESSION['userid'] = $mem['id'];
$_SESSION['level'] = $mem['level'];
$ip = ($_SERVER['HTTP_X_FORWARDED_FOR']) ? $_SERVER['HTTP_X_FORWARDED_FOR'] : $_SERVER['REMOTE_ADDR'];
}
$con = null;
return $success;
} catch (PDOException $e) {
echo $e->getMessage();
return $success;
}
}
public function register() {
$correct = false;
try {
$con = new PDO(DB_DSN, DB_USERNAME, DB_PASSWORD);
$con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$sql = "INSERT INTO `users`(username, password, email) VALUES(:username, :password, :email)";
your if() condition in the userLogin() method won’t work out. your result set contains only one row that is processed by the first call to fetchColumn(). hence the second call will return false (no more rows to fetch from) and all the array accesses will cause a warning.
Quote:
string PDOStatement::fetchColumn ([ int $column_number = 0 ] )
Returns a single column from the next row of a result set or FALSE if there are no more rows.
__________________
please post your code wrapped in [CODE] [/CODE] tags
your if() condition in the userLogin() method won’t work out. your result set contains only one row that is processed by the first call to fetchColumn(). hence the second call will return false (no more rows to fetch from) and all the array accesses will cause a warning.
so, something like this? or would it just be better to use $valid (even though it is oddly named in this case)
or would it just be better to use $valid (even though it is oddly named in this case)
$valid only gives you the content of the first field. you would need a complete ->fetch() to get all data.
in that original code, all that $valid is used for is to check, whether there are data returned. that these data are superfluous in the concept is a mistake often made. (the well-known problem of the SQL wildcard character *).
__________________
please post your code wrapped in [CODE] [/CODE] tags
$valid only gives you the content of the first field. you would need a complete ->fetch() to get all data.
in that original code, all that $valid is used for is to check, whether there are data returned. that these data are superfluous in the concept is a mistake often made. (the well-known problem of the SQL wildcard character *).
Ah, should have known that based on ->fetchColumn(). Well, while I was researching fetch(), I stumbled upon fetchAll(). So, my question is what is the difference? Would you say they are interchangeable?
__________________ Coding is a challenge, get used to it Always remember to debug Try the guess & check method Break it down into simple steps
Having problems with the $username and $email variables. In the database, only the first letter of each variable's value is displayed. Also, when attempting to log in, I always receive my "Invalid username/password" error message even when the values are correct.
classes:
PHP Code:
<?php
include "config.php";
class Users {
private $username;
private $password;
private $email;
private $salt = "Zo4rU5Z1YyKJAASY0PT6EUg7BBYdlEhPaNLuxAwU8lqu1ElzHv0Ri7EM6irpx5w";
private $ip;
//Get user information
public function __construct($data = array()) {
if (isset($data['username'])) {
$this->username = $data['username'];
}
if (isset($data['password'])) {
$this->password = $data['password'];
}
if (isset($data['email'])) {
$this->email = $data['email'];
}
}
public function userLogin() {
//success variable will be used to return if the login was successful
$sucess = false;
try {
//create our pdo object
$con = new PDO(DB_DSN, DB_USERNAME, DB_PASSWORD);
//set how pdo will handle errors
$con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
//this would be our query
$sql = "SELECT * FROM `users` WHERE `username` = :username AND `password` = :password LIMIT 1";
//prepare the statements
$stmt = $con->prepare($sql);
//give value to named parameter :username
$stmt->bindValue("username", $this->username, PDO::PARAM_STR);
//give value to named parameter :password
$stmt->bindValue("password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR);
$stmt->execute();
$valid = $stmt->fetchColumn(); //Check data returned & set $_SESSION variables
if ($valid) {
$success = true;
$_SESSION['loggedin'] = 1;
$mem = $stmt->fetch();
$_SESSION['username'] = $mem['username'];
$_SESSION['userid'] = $mem['id'];
$_SESSION['level'] = $mem['level'];
$ip = ($_SERVER['HTTP_X_FORWARDED_FOR']) ? $_SERVER['HTTP_X_FORWARDED_FOR'] : $_SERVER['REMOTE_ADDR'];
}
$con = null;
return $success;
} catch (PDOException $e) {
echo $e->getMessage();
return $success;
}
}
public function register() {
$correct = false;
try {
$con = new PDO(DB_DSN, DB_USERNAME, DB_PASSWORD);
$con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
$checkUsrs = "SELECT * FROM `users` WHERE `username`= :username";
$usrsChecked = $con->prepare($checkUsrs);
$usrsChecked->bindValue("username", $this->username, PDO::PARAM_STR);
$usrsChecked->execute();
$valid = $usrsChecked->fetchColumn();
if (!$valid) {
$sql = "INSERT INTO `users`(ip, username, password, email) VALUES(:ip, :username, :password, :email)";
$stmt = $con->prepare($sql);
$stmt->bindValue("ip", $this->ip, PDO::PARAM_STR);
$stmt->bindValue("username", $this->username, PDO::PARAM_STR);
$stmt->bindValue("password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR);
$stmt->bindValue("email", $this->email, PDO::PARAM_STR);
$stmt->execute();
return "<p>Registration was successful - </p> <a href='#login'>You may now login</a>";
}
else echo "<p>Username already taken</p>";
} catch (PDOException $e) {
return $e->getMessage();
}
<?php
$usr = new Users($_POST['username'], $_POST['password']);
if ($_POST['submitted'] == 1 or isset($_POST['submit']))
{
if ($usr->userLogin()) {
echo "Welcome ," . $_SESSION['username'];
} else {
echo "<p style='color:red;'>Invalid Username/Password</p>";
}
}
?>
register page script:
PHP Code:
<?php
$usr = new Users($_POST['username'], $_POST['password'], $_POST['email']); //create new instance of the class Users
if (isset($_POST['submit']) or $_POST['submitted'] == 1)
{
$usrIP = $usr->getIp();
echo $usr->register($_POST['username'], $_POST['password'], $_POST['email'], $usrIP);
}
?>
__________________ Coding is a challenge, get used to it Always remember to debug Try the guess & check method Break it down into simple steps
second code block: Users constructor expects an array ...
first code block:
- $mem = $stmt->fetch(); no more data to fetch, returning false.
- username already exists in the class
- SQL, no need of a LIMIT 1 if you have proper DB constraints (e.g. unique username)
third code block: Users->register() does not accept parameters
__________________
please post your code wrapped in [CODE] [/CODE] tags
Thanks Dormilich, fixed everything but now ran into another (presumably) smaller problem. It isn't echoing the $username property. I'd assume this is because, to paraphrase, the $mem property is returning false due to lack of data. But how exactly would I fix that? I've tried fetchAll(), and fetch(1), but to no avail.
classes:
PHP Code:
<?php include "config.php"; class Users { private $username; private $password; private $email; private $salt = "Zo4rU5Z1YyKJAASY0PT6EUg7BBYdlEhPaNLuxAwU8lqu1ElzHv0Ri7EM6irpx5w"; private $ip; //Get user information public function __construct($data = array()) { if (isset($data['username'])) { $this->username = $data['username']; } if (isset($data['password'])) { $this->password = $data['password']; } if (isset($data['email'])) { $this->email = $data['email']; } $this->ip = $_SERVER['REMOTE_ADDR']; if (!empty($_SERVER['HTTP_CLIENT_IP'])) { $this->ip = $_SERVER['HTTP_CLIENT_IP']; } elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) { $this->ip = $_SERVER['HTTP_X_FORWARDED_FOR']; } } public function userLogin() { //success variable will be used to return if the login was successful $sucess = false; try { //create our pdo object $con = new PDO(DB_DSN, DB_USERNAME, DB_PASSWORD); //set how pdo will handle errors $con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); //this would be our query $sql = "SELECT * FROM `users` WHERE `username` = :username AND `password` = :password"; //prepare the statements $stmt = $con->prepare($sql); //give value to named parameter :username $stmt->bindValue("username", $this->username, PDO::PARAM_STR); //give value to named parameter :password $stmt->bindValue("password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR); $stmt->execute(); $valid = $stmt->fetchColumn(); //Check data returned & set $_SESSION variables if ($valid) { $success = true; $_SESSION['loggedin'] = 1; $mem = $stmt->fetch(1); $_SESSION['username'] = $mem['username']; $_SESSION['userid'] = $mem['id']; $_SESSION['level'] = $mem['level']; $userIp = ($_SERVER['HTTP_X_FORWARDED_FOR']) ? $_SERVER['HTTP_X_FORWARDED_FOR'] : $_SERVER['REMOTE_ADDR']; } $con = null; return $success; } catch (PDOException $e) { echo $e->getMessage(); return $success; } } public function register() { $correct = false; try { $con = new PDO(DB_DSN, DB_USERNAME, DB_PASSWORD); $con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); $checkUsrs = "SELECT * FROM `users` WHERE `username`= :username"; $usrsChecked = $con->prepare($checkUsrs); $usrsChecked->bindValue("username", $this->username, PDO::PARAM_STR); $usrsChecked->execute(); $valid = $usrsChecked->fetchColumn(); if (!$valid) { $sql = "INSERT INTO `users`(ip, username, password, email) VALUES(:ip, :username, :password, :email)"; $stmt = $con->prepare($sql); $stmt->bindValue("ip", $this->ip, PDO::PARAM_STR); $stmt->bindValue("username", $this->username, PDO::PARAM_STR); $stmt->bindValue("password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR); $stmt->bindValue("email", $this->email, PDO::PARAM_STR); $stmt->execute(); return "<p>Registration was successful - </p> <a href='#login'>You may now login</a>"; } else echo "<p>Username already taken</p>"; } catch (PDOException $e) { return $e->getMessage(); }
Side Note: It is allowing me to log in with no trouble, so I'd assume $_SESSION['loggedin'] is set and is set to 1, although whenever I exit that tab and return I must log in again which makes me assume something (by which I mean what you previously stated) is going on with the $_SESSION variables in that if statement
__________________ Coding is a challenge, get used to it Always remember to debug Try the guess & check method Break it down into simple steps
the problem is that by calling $valid = $stmt->fetchColumn(); you get the first field’s value and then move to the next result row (that doesn’t exist). hence if you want to get the data out of the original row, you need to change that line. bear in mind that the original code was never interested in DB data in the first place.
__________________
please post your code wrapped in [CODE] [/CODE] tags
the problem is that by calling $valid = $stmt->fetchColumn(); you get the first field’s value and then move to the next result row (that doesn’t exist). hence if you want to get the data out of the original row, you need to change that line. bear in mind that the original code was never interested in DB data in the first place.
ugh, almost there...It is displaying the username now, but when the tab is exited and then reopened you are required to log in again. I'd assume it was this line: if (!isset($_POST['submit'])) {
updated login method:
PHP Code:
public function userLogin() {
//success variable will be used to return if the login was successful
$sucess = false;
try {
//create our pdo object
$con = new PDO(DB_DSN, DB_USERNAME, DB_PASSWORD);
//set how pdo will handle errors
$con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
//this would be our query
$sql = "SELECT * FROM `users` WHERE `username` = :username AND `password` = :password";
//prepare the statements
$stmt = $con->prepare($sql);
//give value to named parameter :username
$stmt->bindValue("username", $this->username, PDO::PARAM_STR);
//give value to named parameter :password
$stmt->bindValue("password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR);
$stmt->execute();
$vMem = $stmt->fetch(); //Check data returned & set $_SESSION variables
if ($vMem) {
$success = true;
$_SESSION['loggedin'] = 1;
$_SESSION['username'] = $vMem['username'];
$_SESSION['userid'] = $vMem['id'];
$_SESSION['level'] = $vMem['level'];
$userIp = ($_SERVER['HTTP_X_FORWARDED_FOR']) ? $_SERVER['HTTP_X_FORWARDED_FOR'] : $_SERVER['REMOTE_ADDR'];
}
$con = null;
return $success;
} catch (PDOException $e) {
echo $e->getMessage();
return $success;
}
}
Before you post, read our: 
