2
\$\begingroup\$

I've been developing a forum as my first OOP project, I've successfully built a login/registration and other functionality typical in forums.

I'm looking for some guidance in terms of where to take the build and other common functions that will need integrating.

users.php

<?php
class USER
{
  private $db;

  function __construct($DB_con)
  {
    $this->db = $DB_con;
  }

  public function register($fname,$lname,$uname,$umail,$upass)
  {
   try
   {
     $new_password = password_hash($upass, PASSWORD_DEFAULT);

     $stmt = $this->db->prepare("INSERT INTO users(user_name,user_email,user_pass) 
       VALUES(:uname, :umail, :upass)");

     $stmt->bindparam(":uname", $uname);
     $stmt->bindparam(":umail", $umail);
     $stmt->bindparam(":upass", $new_password);            
     $stmt->execute(); 

     return $stmt; 
   }
   catch(PDOException $e)
   {
     echo $e->getMessage();
   }    
 }

 public function login($uname,$umail,$upass)
 {
   try
   {
    $stmt = $this->db->prepare("SELECT * FROM users WHERE user_name=:uname OR user_email=:umail LIMIT 1");
    $stmt->execute(array(':uname'=>$uname, ':umail'=>$umail));
    $userRow=$stmt->fetch(PDO::FETCH_ASSOC);
    if($stmt->rowCount() > 0)
    {
     if(password_verify($upass, $userRow['user_pass']))
     {
      $_SESSION['user_session'] = $userRow['user_id'];
      return true;
    }
    else
    {
      return false;
    }
  }
}
catch(PDOException $e)
{
 echo $e->getMessage();
}
}

public function is_loggedin()
{
  if(isset($_SESSION['user_session']))
  {
   return true;
 }
}

public function redirect($url)
{
 header("Location: $url");
}

public function logout()
{
  session_destroy();
  unset($_SESSION['user_session']);
  return true;
}

public function back() {
  $previous = $_SERVER['HTTP_REFERER']; 
  if (isset($previous)) {
    echo $previous;
  } else {
    return FALSE;
  }


}

}
?>

forum.php

<?php 

include_once 'database.php';

class FORUM {

    function __construct($DB_con)
    {
        $this->db = $DB_con;
    }

    public function getColumn() {
        try {
            $query = $this->db->prepare("DESCRIBE posts");
            $query->execute();
        //retrieve the columns inside the table posts
            $forumrows = $query->fetchAll(PDO::FETCH_COLUMN); 
        //Output each column  Value
            foreach($forumrows as $forumrow) {
                echo $forumrow . "</br>";
            }
        } catch(PDOException $e) {
            echo $e->getMessage();
        }
    }

public function getColumnUrl() {

}

} //End forum class

$forum = new FORUM($DB_con);
$forum->getColumn();

?>

database.php

<?php

session_start();

$DB_host = "localhost";
$DB_user = "root";
$DB_pass = "";
$DB_name = "dblogin";

try
{
    $DB_con = new PDO("mysql:host={$DB_host};dbname={$DB_name}",$DB_user,$DB_pass);
    $DB_con->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
}
catch(PDOException $e)
{
    echo $e->getMessage();
}


include_once 'users.php';
$user = new USER($DB_con);

?>

The rest of the pages just optimise the the functions and a general bootstrap layout for the posts and login pages.

\$\endgroup\$
2
\$\begingroup\$
  • back: I'm not sure what this function is used for (and the name doesn't seem that great), but it is open to XSS. An attacker could create a page at http://evil.com/?<img src=onerror=alert(1)>, which would redirect the user to your back page, which would then echo the XSS payload.
  • you should always die after a header redirect (clients do not have to follow it, so any code after the redirect may be executed, which may have security implications).
  • your indentation is inconsistent, and 1 or 2 spaces aren't really enough. I would use 4 at a minimum to ensure readability.
  • your spacing is also off, and your positioning of curly brackets isn't consistent. You can easily solve all these formatting issues with any IDE.
  • try to always write out your variable names. Right now, fname may be clear to you (in context), but firstName is much easier to read.
  • a nested if-else in an if is often not so nice to read (especially with your formatting). It's difficult to see what happens when rowCount is not greater than 0. You could just combine the two ifs.
  • getColumn doesn't actually get anything, it echoes something. You could rename it to showColumn, but really, it should return an array of columns, which is then echoed in a view class.
  • the same goes for all the exceptions. Just catching them and echoing the message isn't a great way to handle it. If you can't recover in the function, just throw them upwards, and let the calling code decide what to do.
\$\endgroup\$
  • \$\begingroup\$ Thanks for the comments Tim, I will do my best to adapt the code to these conventions. regarding the back function its meant to take the user back to the previous page but I was having trouble finding much information regarding security and what the best practise is. What do you mean by your last point, with the exceptions? My idea was to retrieve the columns values as different topics within the forum then have them link through to individual posts if its possible. Thanks for your feedback I will mark your awnser. \$\endgroup\$ – Dannyw24 Apr 28 '16 at 12:27
  • \$\begingroup\$ @Dannyw24 It's a good idea to separate different tasks into different functions and classes; ideally, a function should only do one thing. But your function does two things: It gets stuff, and it displays stuff. Separating the retrival of data and the display of data is something that is done quite often, as it makes it easier to test your application. It would also make it easier to change it, and it would reduce the length of your functions. For more information, you might want to look into design patterns such as MVC. \$\endgroup\$ – tim Apr 28 '16 at 13:17
  • \$\begingroup\$ Thanks Tim some great feedback again, I've dabbled in laravel so not what you mean with the get and set conventions \$\endgroup\$ – Dannyw24 Apr 28 '16 at 13:18

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.