Custom session handler class - is it robust enough?

I've been working on this class for a while now and would appreciate some feedback, even if it's just nitpicking. I've added basic instructions on how to use the class below along with some questions.

```

/
class SessionHandler
{
/**
* @description PDO database handle
* @access private
* @var PDO database resource
/
private $_db = NULL;

/**
* @description Database table name where sessions are stored.
* @access private
* @var string
/
private $_table_name = 'sessions';

/**
* @description Cookie name where the session ID is stored.
* @access private
* @var string
/
private $_cookie_name = 'session_cookie';

/**
* @description Number of seconds before the session expires. Default is 2 hours.
* @access private
* @var integer
/
private $_seconds_till_expiration = 7200; // 2 hours

/**
* @description Number of seconds before the session ID is regenerated. Default is 5 minutes.
* @access private
* @var integer
/
private $_renewal_time = 300; // 5 minutes

/**
* @description Closes the session when the browser is closed.
* @access private
* @var boolean
/
private $_expire_on_close = FALSE;

/**
* @description IP address that will be checked against the database if enabled. Must be a valid IP address.
* @access private
* @var string
/
private $_ip_address = FALSE;

/**
* @description User agent that will be checked against the database if enabled.
* @access private
* @var string
/
private $_user_agent = FALSE;

/**
* @description Will only set the session cookie if a secure HTTPS connection is being used.
* @access private
* @var boolean
/
private $_secure_cookie = FALSE;

Your code looks very good. I like the way you are using Dependency Injection. This code is a good example for other people to follow.

Here are just a few points, seeing as no-one else has answered.

1. How can I improve the existing code?

Move the work that you do while constructing the object out of the constructor. Miško Hevery says it better than i could here. Basically it makes it harder for you to test or extend the object. What I would do is replace your constructor with your setConfig method code as it is checking the configuration of the object you are creating (which is a good thing to do while constructing your object).

I would rewrite it with all of the checks and exceptions thrown first and then an assignment of all the fields done at the end of the constructor.

I would move the following from the constructor:

// Runs the session mechanism
if ($this->_read()) {
    $this->_update();
} else {
    $this->_create();
}

and make it a public method which would be called after the object was constructed. I'm not sure what you would call it. To me it looks like the whole thing could be named update because it updates or creates the session.

I would also move the cleanExpired and setCookie out of the constructor. I'm not sure that cleanExpired really belongs with this class. At least with it as a separate method you would have more control over the time at which it was called.

Very minor: Consider arranging the methods alphabetically within the class.

Very minor: I think 1 if statement is better than 2, consider changing the following:

if (! $this->_expire_on_close)
    {
        if (($result['time_updated'] + $this->_seconds_till_expiration) < time())

to:

if (! $this->_expire_on_close &&
       (($result['time_updated'] + $this->_seconds_till_expiration) < time()))

the logic is grouped together and is very readable.

1. Any obvious security or logic flaws?

No obvious ones that I can see, but I didn't look very hard and am not a security expert. I'd recommend checking out the password salting answers on stack overflow for specific advice on that.

1. Documented/commented well enough or too extensively?

Looks good, it is nice to see the docblock comments.

1. For variables that are strings that will get set: use FALSE or NULL as default value? (e.g. $_user_agent = FALSE vs NULL;)

My preference is for NULL. It makes the check seem more natural:

if (isset($_user_agent) &&
   // vs the more magical / less self explanatory.
   if (($_user_agent) &&

1. A better way to check for valid integers?

I have heard that the PHP filter functions are good for this sort of thing.

1. The _setConfig() method. Nasty or not? Is splitting it into smaller methods pointless?

See 1. I wouldn't split it up.

1. Is there any benefit of commenting class variables?

Up to you. Is @access needed? I thought that was generated automatically?

1. @param void and @return void unnecessary?

Yes, I think they are unnecessary.

1. Is it safe to omit single quotes for values in a database query with prepared statements? ? vs '?'.

Yes, the ? is a placeholder, trying to use '?' would make it a value and the query would no longer work (I think).

StackExchange Code Review Q#8261, answer score: 7