Product search script Object Oriented PHP

Question

I have made a search script in OO PHP that searches for products based on the inserted criteria, selects them and checks whether a sale is on, if it's in stock and other features.

I am new to OOP so I would like to get your feedback on it and whether I am doing it correctly.

The whole script works but I need to know if I'm doing OK.

Many thanks.

Here is my search class (class.search.php)

<?php

Class Search{

    public $db, $productTitle = array(), $productImage = array(), $productPrice = array(), $productStock = array(),
  $productId = array(), $productSalePrice = array(), $productCount = 0;

    function __construct($db_connection){
        $this->db = $db_connection; //stores provided db connection in variable
    }

    function search($searchContent, $max){

        $searchContent = "%".$searchContent."%";

    if ($max == 0){
      $searchQuery = "SELECT * FROM products WHERE title LIKE ?";
    }else{
      $searchQuery = "SELECT * FROM products WHERE title LIKE ? LIMIT $max";
    }

    try{
       $stmt = $this->db->prepare($searchQuery); //querys db for products that contain the query in the title
       $stmt->bindParam(1, $searchContent);
       $stmt->execute();
    }catch(PDOException $e) {
       echo $e->getMessage();
    }    

    $result = $stmt->fetchAll(PDO::FETCH_ASSOC); //fetches results

    if ($stmt->rowCount() != 0){ ///checks if there were any results

      foreach ($result as $row) { //stores all info/variables in an array

        $this->productCount++; //adds to the number of products

        $this->productTitle[] = $row["title"];
        $this->productImage[] = $row["image"];
        $this->productPrice[] = $row["price"];
        $this->productSalePrice[] = $row["salePrice"];
        $this->productStock[] = $row['quantity'];
        $this->productId[] = $row['id'];

      }

    }


    }

  function getPriceToDisplay($productSalePrice, $regularPrice){

    if ($productSalePrice != 0){

      return "<span class='linePrice'>&pound".$regularPrice."</span> &pound".$productSalePrice;

    }else{

        return "&pound".$regularPrice;

    }

  }

  function getStockClass($stock){ //method to get the class for the css

      if ($stock < 10){

          return "low_stock";

      }else{

          return "high_stock";

      }

  }

  function getStockStatus($stock){ //method to get the stock status for the product - a few left or in stock

      if ($stock < 10 && $stock != 0){

          return "A few left!";

      }else if ($stock == 0){

        return "Out of stock";

      }else{

           return "In stock";

      }

  }

  function searchError($error){ //method which deals with any errors that may happen

    if ($error == "no_results"){

      return "No matches to your query have been found";

    }else if($error == "too_short"){

      return "Your query must be at least two characters long";

    }

  }




}

?>

Here is the search script (searchStore.php)

<?php

include_once "includes/header.php";
require_once "classes/class.db.php";
require_once "classes/class.search.php";

if ($_SERVER["REQUEST_METHOD"] == "GET"){

    if (isset($_GET['searchContent'])){$searchContent = $_GET['searchContent'];}
    if (isset($_GET['order'])){$order = $_GET['order'];}


    $search = new Search($db);

    $search->search($searchContent, 0);

    if (strLen($searchContent) >= 2 && $search->productCount != 0){ 

        echo "<div id='searchProductsContainer'>";

        for ($i = 0; $i < $search->productCount; $i++) { 

            $id = $search->productId[$i];
            $title = $search->productTitle[$i];
            $image = $search->productImage[$i];
            $price = $search->productPrice[$i];
            $salePrice = $search->productSalePrice[$i];
            $stock = $search->productStock[$i];

            echo "<div class='searchProduct'><a href='item.php?item_id=".$id."'><p class='title'>".$title."</p>
                    <img src='".$image."'/></a>
                    <p class='price_stock'>".$search->getPriceToDisplay($salePrice, $price)."<span class='".$search->getStockClass($stock)."'>"$search->getStockStatus($stock)."</span></p>
                 </div>";
        } 

        echo "</div>";

    }else{

        $errorMessage = array();

        //checks what the error is 

        if ($search->productCount == 0){

            $errorMessage[] = $search->searchError("no_results");

        }

        if(strLen($searchContent) < 2){

            $errorMessage[] = $search->searchError("too_short");

        }

        echo "<div id='searchError'>";

        foreach ($errorMessage as $error){
            echo "<p>".$error."</p>";
        }

        echo "</div>";


    }


}



?>

Answer 1

Security: SQl Injection

You use prepared statements, which is great. But you still put variables directly into SQL queries, which makes you vulnerable to SQL injection via $max:

$searchQuery = "SELECT * FROM products WHERE title LIKE ? LIMIT $max";

At a minimum you would need to check if $max is an integer. But really, the only proper solution is to use prepared statements for all variables.

Security: XSS

You are inserting data from the database into HTML without encoding or sanitation, which will lead to XSS by anyone who can add products. This may or may not be acceptable at the moment, but you should also think about future requirements (maybe not everyone who can add products should have full admin rights?).

Structure/OOP

I am new to OOP so I would like to get your feedback on it and whether I am doing it correctly.

Not really. Ideally, a class should be responsible for one thing. But your class does quite a lot. As its name suggest, it searches, but it also builds HTML, transforms error codes to error messages, and checks stock values.

First of all, it would be a good idea to separate all code that builds HTML or other output in one place. This will make it a lot easier to modify it later on. Ideally, you would use a templating engine for this.

The previous point should get rid of your getPriceToDisplay method, which really doesn't belong in this class. But you should also move getPriceToDisplay, getStockStatus, and searchError somewhere else. getStockClass might belong in a Stock class, and getStockStatus and searchError in some component that transfers labels to messages (having this all in one place makes adaptation easier).

You also have quite a lot of fields in your search class, which are also rather confusing (having arrays of the different properties of items instead of an array of items). You could easily get rid of the fields by adding a Product class. You also don't really need productCount.

In general, you might want to look into MVC. You don't have to follow it 100%, but it's a good first approach.

Style

Your indentation is off, and your formatting is not consistent (spacing, vertical whitespace, etc), making your code unnecessarily hard to read.

You can use any IDE to fix most formatting issues automatically.

Naming

Your variable names are generally quite good. One small thing:

• be consistent with your variable names. Either it's camelCase or snake_case, not both.

Your function names could probably use some improvement. Eg:

• searchError: this doesn't search an error, it transforms a label into an error message. It might be called labelToErrorMessage.
• getStockClass: it isn't obvious that this is about CSS at all. This should be expressed via the function name.
• getPriceToDisplay: getPriceToHTML may be more explicit.

But you can see that most of these renames are not necessary or would be obvious if the functions were moved to a better context.

Comments

Your comments are helpful for people learning PHP, but if this isn't code for a tutorial, you should remove them. Inline comments shouldn't duplicate what the code already told us, but explain why the code is written the way it is.

Your function comments would be better as PHPDoc comments. Also, some of them could be a bit more in-depth (eg searchError: how does it "deal with errors"?, etc).

Misc

• don't echo exception messages inside classes, it makes them difficult to reuse. Throw them, and let the calling code handle it.

Answer 2

I think there is sound advice from @tim in his answer. So, I am not going to spend time talking in depth about some of the security vulnerabilities or the general suggestion to separate different concerns into different classes. Instead I will focus on going through the code itself and adding thoughts that hopefully complement his answer.

I have added my comments inside multi-line comments in the code below.

Search Class

<?php
/*
Typical standard would be to either have space after class name and
before bracket or to put opening class bracket on next line.
*/
Class Search{
/*
Put each class property on it's own line. This is hard to read.
*/
    public $db, $productTitle = array(), $productImage = array(), $productPrice = array(), $productStock = array(),
  $productId = array(), $productSalePrice = array(), $productCount = 0;

/*
You should enforce that a valid PDO or DB object is passed as parameter.
Change method signature to something like this using PDO or your own
custom DB class as appropriate:
function __construct(PDO $db_connection){
This will cause InvalidArgumentException to be thrown if you do not
get passed a valid PDO instance. Fail fast!
*/
    function __construct($db_connection){
        $this->db = $db_connection; //stores provided db connection in variable
    }
/*
Since $max seems like optional parameter, should you provide default
value here (perhaps null) to have the ability to better validate
whether something was passed or not?
*/
    function search($searchContent, $max){
/*
Validate your parameters and fail out (throw InvalidArgumentException)
if you are not getting what you expect, which would likely be
string of 2+ characters and either null (no max value passed) or
positive integer. Fail fast!
*/
        $searchContent = "%".$searchContent."%";
/*
Compare $max as null based on comments above. These queries are not going
to be able to use an index on the table since typical MySQL B-Tree index
only support prefix match.  You might need to consider use full text search
with appropriate full text index on title if you want to be able to
make this type of query and leverage an index.
*/
    if ($max == 0){
      $searchQuery = "SELECT * FROM products WHERE title LIKE ?";
    }else{
/*
Parametrize $max. Don't concatenate.
*/
      $searchQuery = "SELECT * FROM products WHERE title LIKE ? LIMIT $max";
    }

    try{
       $stmt = $this->db->prepare($searchQuery); //querys db for products that contain the query in the title
/*
bind $max here to
*/
       $stmt->bindParam(1, $searchContent);
       $stmt->execute();
/*
Add space after closing brackets for things like catch and else blocks.
Typical throughout this code.  This is hard to read as is and I am not
aware of any common style guides which would allow this.
*/
    }catch(PDOException $e) {
/*
Don't just echo message to output here.  Either catch and rethrow (if
you want to wrap the exception) or don't catch at all and let the
exception bubble up to the caller. Right now you are basically
just swallowing the exception.
*/
       echo $e->getMessage();
    }    
/*
This should all be in your try block above. fetchAll() can be problematic
from a memory standpoint. I see now reason that you are doing that here
vs. working with a single row of the result set at a time.  You are
iterating each row anyway below, so why load every row into memory right here?
*/
    $result = $stmt->fetchAll(PDO::FETCH_ASSOC); //fetches results
/*
Use exact comparison. What happens when 0 rows are returned? What
is returned to caller? Right now this method will not return anything
to caller in this case. This line of code should happen BEFORE you try to
fetch results or perhaps not even have it at all if a search which returns
no matches is an expected use case and you don't need to do anything other
than set an empty array.
*/
    if ($stmt->rowCount() != 0){ ///checks if there were any results
/*
      foreach ($result as $row) { //stores all info/variables in an array
/*
Agree with comments from @tim here.  Why and does this class need to store
any of this information? Why not simply fetch each row (prefereably as object
not as assocciative array) and return? If you did that, then a fetchAll
approach makes sense, as this method would then simply return an array
of objects to caller.
*/
        $this->productCount++; //adds to the number of products

        $this->productTitle[] = $row["title"];
        $this->productImage[] = $row["image"];
        $this->productPrice[] = $row["price"];
        $this->productSalePrice[] = $row["salePrice"];
        $this->productStock[] = $row['quantity'];
        $this->productId[] = $row['id'];

      }
/*
Consider a finally block to free statement and result resources
*/
    }


    }
/*
Be specific about public/protected/private visibility on ALL methods.
This method should probably live on a Product class.
*/
  function getPriceToDisplay($productSalePrice, $regularPrice){
/*
Need parameter validation? Fail fast!
*/
    if ($productSalePrice != 0){
/*
Get rid of formatting here.  Just have method return numeric price. Let
caller of some other class worry about formatting.
*/
      return "<span class='linePrice'>&pound".$regularPrice."</span> &pound".$productSalePrice;

    }else{

        return "&pound".$regularPrice;

    }

  }

/*
Method should live on product class. But it seems REALLY odd to pass
the stock value into the method. Just to get some string value out.
I would think stockClass might be a property on a Product class object
and you would not need to pass any parameter to the method at all. You
would just determine this value when Productis instantiated and either
have the property public or provide getter for it. Consider constant
to use to represent value here not a string.
*/
  function getStockClass($stock){ //method to get the class for the css
 /*
 No validation here. Fail fast!
 */
      if ($stock < 10){

          return "low_stock";

      }else{

          return "high_stock";

      }

  }
/*
How is this different from previous method?  Leave display formatting
concerns to caller up the call stack.
*/
  function getStockStatus($stock){ //method to get the stock status for the product - a few left or in stock
/*
No validation here. Fail fast!
*/
      if ($stock < 10 && $stock != 0){

          return "A few left!";

      }else if ($stock == 0){

        return "Out of stock";

      }else{

           return "In stock";

      }

  }

/*
An unnecessary method here. Why would no results be an error? I would think this
could be expected to happen in normal operations. Enforce search
string length in the search method and throw InvalidArgumentException with
appropriate method.
*/  
  function searchError($error){ //method which deals with any errors that may happen
    if ($error == "no_results"){

      return "No matches to your query have been found";

    }else if($error == "too_short"){

      return "Your query must be at least two characters long";

    }

  }

}
/*
Most PHP style guides call for omission of closing PHP tag
*/
?>

Calling Script

<?php
/*
Should this be require?  If this is absolutely necessary for code
execution, it probably should be.
*/
include_once "includes/header.php";
require_once "classes/class.db.php";
require_once "classes/class.search.php";

/*
Use exact comparison. Get rid of nesting. It appears your code only anticipates
that a GET request is being processed.  You are probably better off checking
that the method is NOT GET and stopping code execution.  This would allow rest
of code to not have to be inside IF conditional. Fail fast!
*/
if ($_SERVER["REQUEST_METHOD"] == "GET"){
/*
Break this up across lines. It is hard to read.
You are doing no validation here at all other than seeing is parameter
is set.  Consider !empty() instead of isset.  What happens if these
validation criteria are not met?  Right now your code would continue to
execute. Fail fast!
*/
    if (isset($_GET['searchContent'])){$searchContent = $_GET['searchContent'];}
    if (isset($_GET['order'])){$order = $_GET['order'];}

/*
I see you included DB class, but 
    $search = new Search($db);
/*
Do the search results not need to be set to variable?
Don't pass 2nd parameter here is record limit does not need to be applied
here.
You might need try-catch block here if you want to handle underlying
exceptions thrown by search method. You catch block could be where
you output "Search string to short" messaging to user.
*/
    $search->search($searchContent, 0);

/*
Why do you care about search string length here? Let search method
enforce this requirement. This calling code just needs to handle
validation failure. This check is too late anyway as the search has
already been executed at this point.
If you return the search results as an array as noted above, you
would simply check the size of that array.
Consider checking like this:
if(empty($searchResultArray)) {
    // "No results" messaging to user
} else {
    // display search results
}
*/
    if (strLen($searchContent) >= 2 && $search->productCount != 0){ 

        echo "<div id='searchProductsContainer'>";
/*
If you follow the advice of returning an array of product objects,
You greatly simplify this code. As you don't need to set all these
variables in this loop.  Instead, you would just start operating
against the product objects.
*/
    for ($i = 0; $i < $search->productCount; $i++) { 

            $id = $search->productId[$i];
            $title = $search->productTitle[$i];
            $image = $search->productImage[$i];
            $price = $search->productPrice[$i];
            $salePrice = $search->productSalePrice[$i];
            $stock = $search->productStock[$i];

/*
This is really hard to read. Consider class or function that accepts
a product object and renders the HTML for it. At a minimum consider
usings a heredoc to set this content to a variable.
*/
            echo "<div class='searchProduct'><a href='item.php?item_id=".$id."'><p class='title'>".$title."</p>
                    <img src='".$image."'/></a>
                    <p class='price_stock'>".$search->getPriceToDisplay($salePrice, $price)."<span class='".$search->getStockClass($stock)."'>"$search->getStockStatus($stock)."</span></p>
                 </div>";
        } 

        echo "</div>";

    }else{
/*
Consider moving the "no results" in conditional before the case where there
are results and place the message the user there. No need to have a class
method to generate the message as this calling code has full understanding
of the context.
*/
        $errorMessage = array();

        //checks what the error is 

        if ($search->productCount == 0){

            $errorMessage[] = $search->searchError("no_results");

        }
/*
This should be moved much earlier in code.  When search is attempted and
throws exception because search string is too short, you should handle the
exception and give error messaging there.
*/
        if(strLen($searchContent) < 2){

            $errorMessage[] = $search->searchError("too_short");

        }

        echo "<div id='searchError'>";

        foreach ($errorMessage as $error){
            echo "<p>".$error."</p>";
        }

        echo "</div>";


    }


}
/*
Drop closing PHP tag
*/
?>

In looking through the class code I am wondering whether there really is even a use case to ever instantiate an instance of this class. If you change the search method to return an array of product information, this eliminates the need to store any of this information in a search object. You really don't have a need to store the DB object on the class either as it is only used by the search method. You could end up having a search class with a single static search method with accepts a database object, the search string and an optional limit parameter.