MySQL PDO query to search by name and role

Question

I'm new to this PDO PHP and I have coded this spaghetti style. I hope you can give me idea how to improve using prepared statements. I'm not a lazy person, but I really need your help to improve my coding style.

            try {
        $select = "SELECT users.id, users.users_id, username, firstname, lastname, password, position";
        $from = " FROM users
                  INNER JOIN users_role
                  ON users.id=users_role.users_id
                  INNER JOIN role
                  ON users_role.users_role=role.id";
        $where = " WHERE";
        $placeholders = array();
        if ($category !=''){
            if ($category == 'admin'){
                $where .= " position='admin'";
                if ($character_match !=''){
                    $where .= " AND ";
                }

            }
            else if($category == 'instructor'){
                $where .= " position='Instructor'";
                if ($character_match !=''){
                    $where .= " AND ";
                }
            }
            else if ($category=='student'){
                $where .= " position='student'";
                if ($character_match !=''){
                    $where .= " AND ";
                }
            }
            elseif ($category == 'Unverified'){
                $where .= " position='Unverified'";
            }
            else if ($category == 'view_all') {
                $where = "";
                if ($character_match != ''){
                    $where = " WHERE ";
                }
            }
            if($character_match !=''){
                $where .= "(username LIKE '" . $character_match . "%'" 
                        . " OR firstname LIKE '"  . $character_match . "%'"
                        . " OR lastname LIKE '"  . $character_match . "%')";
                $placeholders[] = $character_match;
            }
            $where .= " ORDER BY position ASC";

            $sql = $select . $from . $where; 
            $q = $this->db->prepare($sql);
            $q->execute($placeholders);         
        }
    }
    catch(PDOException $e){
            $e->getMessage();
        }
       $rows = array();

       while($row = $q->fetch(PDO:: FETCH_ASSOC)){
           $rows[] = $row;
       }

Answer 1

There's different things about your code, the most important one is to start using PDO tools to facilitate our work. Use named parameters, and dissociate your query construct from setting correct values:

1. if the only thing you're going to catch is echo the error message, there's absolutely no reason to do it. setting $this->db->setAttribute(PDO::ATTR_ERRMODE,PDO::ERRMODE_EXCEPTION); will react in the exact same way.

2. for the LIKE portion of the code: named parameters must contain a full litteral string (or integer, or whatever) but you have to put wildcards % IN the string for the LIKE to work. exemple: $values[':character_match'] = '%value%'

---

$sql = 'SELECT users.id, users.users_id, username, firstname, lastname, password, position
            FROM users
            INNER JOIN users_role ON users.id=users_role.users_id
            INNER JOIN role ON users_role.users_role=role.id ';
if ($category != 'view_all') {
    $sql .= 'WHERE position=:position ';
}
if ($character_match != '') {
    $sql .= ($category == 'view_all' ? ' WHERE ' : ' AND ');
    $sql .= '(username LIKE :character_match OR firstname LIKE :character_match OR lastname LIKE :character_match)';
    $values[':character_match'] = $character_match;
}
$sql .= ' ORDER BY position';

switch ($category) {
    case 'admin':
        $values[':position'] = 'admin';
        break;
    case 'instructor':
        $values[':position'] = 'Instructor';
        break;
    case 'student':
        $values[':position'] = 'student';
        break;
    case 'Unverified':
        $values[':position'] = 'Unverified';
        break;
}

$q = $this->db->prepare($sql);
$q->execute($placeholders);

$rows = array();
while ($row = $q->fetch(PDO:: FETCH_ASSOC)) {
    $rows[] = $row;
}

Answer 2

First, a few minor remarks:

• Your spacing and indentation is inconsistent, e.g. $category=='student' vs. $category == 'instructor', else if vs. elseif. If you have decided on a coding style, follow it through. It doesn't matter that much what you do as long as you do it consistently.

• Your code snippet does not seem to be wrapped inside a function. Do this even when the code is only used in one place, as it lends a self-documenting quality to the code, and makes the scope of some variables clearer.

Now on to your code. Assuming that $character_match is external data and has not been sanitized, you are suffering from an SQL injection vulnerability. Consider what happens with %'; DROP TABLE students; -- or similar fun. Use the quote method on your PDO object to avoid this. Your current use of placeholders makes exactly zero sense, as far as I can see. (I.e. I am not seeing any placeholders, but an argument to execute)

if ($category == 'Unverified' && $character_match != ''), then you will generate invalid SQL.

You do not handle the case that $category is none of the given strings.

Your cases for admin, instructor, and student are exactly the same, except that the capitalization of instructor changes. If possible clean up your DB to be consistent in this respect. In the meanwhile, you could use an array:

# careful: the values have to be safe for SQL without further escaping
$position = array(
    'admin'      => 'admin',
    'instructor' => 'Instructor',
    'student'    => 'student');

if (array_key_exists($category, $position)) {
    $where .= " position='" . $position[$category] . "'";
    ...
}

This means you only have to handle the two special cases Unverified and view_all differently.