1
\$\begingroup\$

Following yesterday's advice on my Customer class, I have now created a new class which is for looking up descriptions from a MySQL database. I have a Lookup table which contains type, name, value, and descriptions.

Here's a basic example of type = customer and name = state

  • Customer States
    1. New Customer
    2. Pending Approval
    3. Approved / Active
    4. Deleted

In this example if I pass in the parameters "customer", "state", and 2, the method will return "Pending Approval".

pdo.inc.php

<?php

// Version 0.1
// Last updated 08 Jun 2016

define('DB_CONFIG_HOST', 'localhost');
define('DB_CONFIG_DB', 'dev');
define('DB_CONFIG_USER', 'dev');
define('DB_CONFIG_PW', 'dev');
$dsn = 'mysql:host=' . DB_CONFIG_HOST . ';dbname=' . DB_CONFIG_DB . ';';
define('DB_CONFIG_DSN', $dsn);

try
{
    $pdo = new PDO(DB_CONFIG_DSN, DB_CONFIG_USER, DB_CONFIG_PW);
}
catch (PDOException $ex)
{
    error_log('Connection failed: ' . $ex->getMessage());
    die();
}

?>

lookup.class.php

<?php

// Version 0.1
// Last updated 09 Jun 2016

class Lookup
{

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

    function getLookup($lookup_type, $lookup_name, $lookup_value)
    {

        $valid_types = array("customer");
        $valid_names = array("state");

        if (!in_array($lookup_type, $valid_types))
        {
            throw new InvalidArgumentException('Lookup type is not a valid');
        }

        if (!in_array($lookup_name, $valid_names))
        {
            throw new InvalidArgumentException('Lookup name is not a valid');
        }

        if (empty($lookup_value) || !is_int($lookup_value) || $lookup_value < 0)
        {
            throw new InvalidArgumentException('Lookup value is not a valid integer');
        }

        $query = "SELECT lookup_description FROM Lookup WHERE lookup_type = :lookup_type AND lookup_name = :lookup_name AND lookup_value = :lookup_value LIMIT 1";
        try
        {
            $stmt = $this->pdo->prepare($query);
            $stmt->bindParam(':lookup_type', $lookup_type);
            $stmt->bindParam(':lookup_name', $lookup_name);
            $stmt->bindParam(':lookup_value', $lookup_value);
            $stmt->execute();
            if ($stmt->rowCount() === 0)
            {
                return false;
            }
            else
            {
                return $stmt->fetchColumn();
            }
        }
        catch (PDOException $ex)
        {
            error_log('Something went wrong in getLookup ' . $ex->getMessage());
            return null;
        }
    }

}

?>

test.php

<?php

// Version 0.1
// Last updated 09 Jun 2016

include_once('pdo.inc.php');
include_once('lookup.class.php');

try
{
    $l = new Lookup($pdo);
}
catch (Exception $ex)
{
    echo $ex->getMessage();
    die();
}


try
{
    echo $l->getLookup("customer", "state", 1); // Returns "New Customer"
    echo $l->getLookup("customer", "state", 2); // Returns "Pending Approval"
}
catch (Exception $ex)
{
    echo $ex->getMessage();
    error_log('Customer name lookup failed with: ' . $ex->getMessage());
}

?>

I'm not sure I like the idea of having fixed arrays for looking up valid parameter values. I was thinking of getting a distinct list from MySQL, but as to do that I might as well just pass the query without validation and let it return null. Any thoughts?

\$\endgroup\$
  • \$\begingroup\$ You should link back to yesterday's code review from this message, so folks can get context of the evolution of your code. You can do that simply by adding hyperlink to it where you note "yesterday's advice" \$\endgroup\$ – Mike Brant Jun 9 '16 at 18:12
  • \$\begingroup\$ Not sure of your intent for Lookup class. Are you only ever going to query for a single customer at a time (in which case you might attempt to instantiate two different customer objects, each which expose all properties of a customer)? Or are you looking to be able to do something like retrieve a list of customers based on some query? You have totally changed your approach from yesterday in terms of having a class called Customer, to now not having a customer class at all. Why is that? \$\endgroup\$ – Mike Brant Jun 9 '16 at 18:14
2
\$\begingroup\$

Having provided an answer to your previous review, I have a mixed bag of reactions towards this latest version.

The good:

  • You seem to have embraced the concept of data validation (though conspicuously missing in your constructor to Lookup class).
    • Your usage of try-catch seems appropriate (with one possible exception I will note below).
    • I think you are handling the PDO dependency in the class in a much better manner now, by storing the connection on the object.

Possible points of concern:

  • First and foremost it now more unclear to me what sort of approach you are trying to take to this class definition. Before you had a customer class, now you have a lookup class. What are you really trying to do? Are you trying to establish a factory pattern whereby you can query a set of records and return appropriate object representations? If so, why no object class now. Since I don't know your full use case here, I will not get into what the interface design may look like for a factory and will instead still with a single class implementation, as I see nothing in your question that would suggest you are actually trying to work with collections of customers (but rather single customer instances).

To this end, I will begin to talk in terms of creating an object relational mapping class that allows you to instantiate a single customer object based on a provided ID and have access to whatever properties on the customer object that may be appropriate. You may want that customer class to look like this:

<?php
// Version 0.1
// Last updated 09 Jun 2016

class Customer
{

    protected $customerId;
    protected $firstName;
    protected $lastName;
    protected $state;
    // any other properties from DB table you want to capture on this object.

    public abstract function getCustomerById(PDO $pdo, $customerId) {
        if(!self::validateCustomerIdFormat($customerId) {
            throw new InvalidArgumentException(
                '$customer_id is not a valid integer' . 
                ' Value provided: ' . var_export($customerId, true)
            );
        }
        $query = "SELECT
                    customer_id AS customerId,
                    first_name AS firstName,
                    last_name AS lastName,
                    state Lookup
                    /* any other properties along with alias to property name */
                  FROM customer
                  WHERE customer_id = :customer_id
                  LIMIT 1"; // limit not needed if customer_id is unique

        try {
            $stmt = $pdo->prepare($query);
            $stmt->bindParam(':customer_id', $customerId);
            $stmt->execute();
            // return saturated instance of this class
            // no need for row count check here
            // as this method returns false if there are no records
            // remaining in result set     
            return $stmt->fetchObject(__CLASS__);
        } catch (PDOException $ex) {
            // note here that I have decided just to rethrow the exception
            // rather then returning null as in previous example
            // this is because if there is a problem with underlying PDO object
            // there is nothing really this class can do (a terminal exception)
            error_log('Something went wrong in getCustomerById' . $ex->getMessage());
            throw $ex;
        }
    }

    // constructor has been made private
    // to force use of abstract method to instantiate class
    private function __construct($customer_id) {

    }

    // have added a public abstract validation function around customer ID
    // the allows single place to configure validation rules
    // and can be used outside object context for validating customer ID 
    // format anywhere in the application
    public abstract function validateCustomerIdFormat($customerId) {
        if (empty($customerId) || !is_int($customerId) || $customerId < 0) {
            return false;
        }
        return true;
    }
}
?>

In this case, your calling code might look like:

include_once('pdo.inc.php');
include_once('customer.class.php');

try
{
    $customer1 = Customer::getCustomerById($pdo, 1);
    $customer2 = Customer::getCustomerById($pdo, 2);
}
catch (Exception $ex)
{
    echo $ex->getMessage();
    error_log('Customer ID lookup failed with: ' . $ex->getMessage());
}

// conditional needed here are Customer::getCustomerById can return false
// if no match found
// here we simply echo out the customer's "state" information
if($customer1) {
    echo $customer1->state;
}
if($customer2) {
    echo $customer2->state;
}
  • The one minor quibble I have about your try-catch usage is that you might consider splitting the instantiation of each customer object into separate try-catch block depending on your need to granularly perform different catch block activities on each. I didn't show that in my example, because I know your test.php is just a proof of concept. But in the real world, if you needed to instantiate two customer objects, you might want to handle cases where either one fail independently.
\$\endgroup\$
2
\$\begingroup\$

Getting a distinct list from MySQL would slow things down quite a bit, I wouldn't do that.

Perhaps you're overdoing it a bit? Yes, it is always good to filter user input, but I would do that at the point where this input arrives in your application, not in each class inside the application. You are already binding the parameters, which secures the database against injection. Anything apart from what's in your database will result in false, which might be better behaviour than throwing an exception.

So the protection is in the binding of the parameters, and not really in the validation.

Talking about validation. There might be a perfectly good reason to insert all that code. Regretably your validation is incomplete. If I give your method a $lookup_value of 10 it will just return false. That might be a real value, or a sign that the value doesn't exist. You would normally use null in these cases, which is what you do when the query fails.

So my suggestions are:

  • Filter user input, perhaps with static arrays, directly at the point of entry. So at the point where you get user input out of $_POST, $_GET, or anything else. Try to be as strict as possible. When a value doesn't meet all the criteria, give feedback and allow the user to enter a correct value. Never let an user freely enter values into your application in the (false) assumption that the application will deal with any abnormal values all by itself: It won't. You will eventually make mistakes.
  • Keep the validation inside your application very simple and flexible, this will make it easier to read, modify, and understand the code. The validation is there to support you to debug your application, it should not be relied upon as a security feature.
  • Always bind parameters, of course.

Something like this:

function getLookup($lookup_type,$lookup_name,$lookup_value)
{
  // checking type of arguments
  if (!(is_string($lookup_type) &&
        is_string($lookup_name) &&   
        is_int($lookup_value))) {
    throw new InvalidArgumentException('Invalid arguments in Lookup->getLookup().');
  }
  // select a description of ???
  $query = 'SELECT lookup_description 
            FROM Lookup 
            WHERE lookup_type  = :lookup_type AND 
                  lookup_name  = :lookup_name AND 
                  lookup_value = :lookup_value 
            LIMIT 1';
  try {
    $statement = $this->pdo->prepare($query);
    $statement->bindParam(':lookup_type', $lookup_type);
    $statement->bindParam(':lookup_name', $lookup_name);
    $statement->bindParam(':lookup_value',$lookup_value);
    $statement->execute();
    // return any results
    if ($statement->rowCount() > 0) return $statement->fetchColumn();
  }
  catch (PDOException $exception) {
    error_log('Database error in Lookup->getLookup(): '.$exception->getMessage());
  }
  // failure: no results or database error
  return null;
}

While I was writing this code I noticed that you're using strange names in your database: A Lookup table, containing columns like lookup_name. From these names I get no idea what the content of that table is. The names describe the functions in PHP and not their meaning. A database should be comprehensible on its own, without any code operating on it.

PS: Perhaps someone told you to validate the arguments of your method thouroughly. I cannot help that, this happens when many people can give advise.

\$\endgroup\$
  • \$\begingroup\$ This question mentions no concept of user-provided data, so while what you say about validating data from end user and bind parameters being a good SQL injection mitigation is true, it might be beyond the scope of this review. The part about validating input to the methods makes absolute sense. You need to validate for example that the customer Id provided is within acceptable range such that method can proceed. If you ever get in the habit of writing unit tests around these methods, and are properly writing them, you will need to have such logic to address edge cases. \$\endgroup\$ – Mike Brant Jun 9 '16 at 19:52

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.