SessionHandlerInterface Class

Question

The following class is used for handling sessions:

class DBHandler implements SessionHandlerInterface {

    protected $exists;

    public function open($save_path, $name) {
        try {
            DBCxn::get();
            return true;
        } catch(PDOException $e) {
            return false;   
        }
    }

    public function close() {
        return true;    
    }

    public function destroy($session_id) {
        $sth = DBCxn::get()->prepare("DELETE FROM sessions WHERE session_id = ?");
        $sth->execute(array($session_id));
        return true;
    }

    public function gc($maxlifetime) {
        $sth = DBCxn::get()->prepare("DELETE FROM sessions WHERE last_update < ?");
        $sth->execute(array(time() - $maxlifetime));    
        return true;    
    }

    public function read($session_id) {
        $sth = DBCxn::get()->prepare("SELECT session_data FROM sessions WHERE session_id = ?");
        $sth->execute(array($session_id));
        $rows = $sth->fetchALL(PDO::FETCH_NUM);
        if (count($rows) == 0) {
            $this->exists = "n";
            return '';
        }
        else {
            $this->exists = "y";
            return $rows[0][0];
        }   

    }


    public function write($session_id, $session_data) {

        if ($this->exists == "y") {
            $sth = DBCxn::get()->prepare("UPDATE sessions SET session_data = ?, last_update = NOW() WHERE session_id = ?");
            $sth->execute(array($session_data, $session_id));
        }
        if ($this->exists == "n") {

            $sth = DBCxn::get()->prepare("INSERT INTO sessions (session_id, session_data, last_update) VALUES (?, ?, NOW())");
            $sth->execute(array($session_id, $session_data));           
        }


    }
}

session_set_save_handler(new DBHandler);

How could this class be improved?

Answer 1

So there is a consideration that needs to be made when implementing your own session handler that is not really clear in the PHP documentation. That is that the SessionHandlerInterface does not support the create_sid() method introduced in PHP 5.5.1 & 7. For this reason, you might strongly consider extending the PHP SessionHandler class as opposed to implementing the interface if you want best compatibility with more current PHP releases.

Again, this is actually contrary to what the PHP documentation states, but it might help give context to this decision if you look through the the feature development conversation from the core PHP developers. In a nutshell, they didn't add create_sid() to the interface in order not to break backward compatibility. I would particularly point you to the comment from It from Dec 14, 2012 for the most compelling discussion around interface vs. object inheritance.

By having access to create_sid() method call, you could do something like:

function create_sid()
{
    $sessionId = parent::create_sid();
    // create record in database with this new session id
}

---

The class name seems odd. It is not just a "Db Handler" right? It is Db-based session handler. So perhaps DbSessionHandler, PdoSessionHandler (if PDO is really the dependency here) or similar might be more appropriate.

---

Why have all the calls to DBCxn::get() throughout the class? Since this is a critical dependency for this class, you should probably pass this dependency (i.e. a valid PDO object) to the constructor. There should be no reason for this class to keep having to try to instantiate DB connections (even if perhaps you are using a singleton here). How about usage like:

try {
    $pdo = new DbCxn::get();
    $dbSessionHandler = new DbHandler($pdo);
    session_set_save_handler($dbSessionHandler);
} catch (PdoException $e) {
    /* bail on execution or maybe don't even catch */
}

With constructor like:

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

This gives you the benefit of knowing that you have a valid DB connection via PDO before you even try to set the session save handler. You can bail out in your bootstrapping process rather than having exceptions/error taking place when you are actually trying to interact with session functionality.

---

Your implementation is a little naive in terms of handling session destruction and garbage collection. To implement sessions in a secure and resilient way, one should strongly consider thinking of session destruction as nothing more than setting as setting session data as invalid. That session data might need to remain in order to handle things such as race conditions from asynchronous requests, such that you can maintain good user experience in your application. Most mature implementations require setting of timestamp-based controls around data validity and a separate process for data deletion.

So here your destroy() function might simply update a field on the record that the data is no longer valid and setting a TTL (maybe a few seconds, maybe longer depending on your application needs) on when the data should be eligible for final deletion via garbage collection.

Your gc() method should then look to delete either active sessions where last update is past maximum lifetime or inactive records that have passed the data validity TTL.

---

I don't understand the exists property at all. It seems unnecessary. You can use REPLACE_INTO to design away your two code branches in write() method (or change operational order of update/insert if you wanted to perhaps remain more database agnostic as suggested in another answer).

I also think that perhaps you are thinking of these method calls being made in more of a linear fashion than what might actually be presented.

You may want to take a look at this great article on how session handlers are called relative to session management code in PHP. By looking at this, you can see that things like session ID change can happen between calls to the other methods.

I especially like this summary comment from the article:

The simplest way to think about custom session handlers, is to avoid thinking of them as objects with instance methods, although that happens to be how it's implemented - instead, think of the methods as being static; or in other words, avoid state in your implementation, with the exception of accidental state (for optimization/caching purposes), and bear in mind, the original API for custom session handlers was a set of individual functions, essentially the analog to a set of static methods, not to an object.

Right now, you are adding state to your class.

---

How do you handle session forwarding? When your code calls for session ID to be regenerated, how will you forward any calls from old session to new session? This could obviously be handled outside the session handler itself, but it certainly ties into the same data expiry consideration I noted before.

---

Consider namespacing your session library.

---

    $rows = $sth->fetchALL(PDO::FETCH_NUM);
    if (count($rows) == 0) {
        $this->exists = "n";
        return '';
    }
    else {
        $this->exists = "y";
        return $rows[0][0];
    }

This seems like it could be simplified. You are only fetching a single value after all.

How about just:

return ($data = $sth->fetchColumn(0)) ? $data : '';

Or non-ternary equivalent if you like.

---

Your DB interactions seem to consider "happy path" only. What if statement preparation or execution fails?

---

I am adding a link to a session management library I created, not so much for you to use (though you are free to do so, though it has no built-in database handler and would need to be extended for that), but rather because I think that within the main readme and the individual class documentation, I have tried to assemble some the better known resources (to me) around session security and session management in PHP. Between this and the code itself, I hope it gives you better footing for dealing with sessions in PHP (which PHP certainly does not make easy "out of the box").

https://github.com/mikecbrant/php-ultimate-sessions

Answer 2

One thing that I would change is to replace your $this->exists property with a boolean (which is faster to process than a string), and perhaps clean things up a bit, thus making your code a bit more expressive:

public function write($session_id, $session_data) 
{
    if (!$this->exists) {
        $sth = DBCxn::get()->prepare("INSERT INTO sessions (session_id, session_data, last_update) VALUES (?, ?, NOW())");
        $sth->execute(array($session_id, $session_data));           
    }
    $sth = DBCxn::get()->prepare("UPDATE sessions SET session_data = ?, last_update = NOW() WHERE session_id = ?");
    $sth->execute(array($session_data, $session_id));
}

I might also have the write function return the number of records updated, to allow for error checking.

Answer 3

Native sessions can handle arrays and objects. Yours will apparently convert everything to a string. I would json_encode before inserting and json_decode after retrieving.

Use a boolean (or a zero/one) for the exists property.

In your write method use if...else rather than 2 ifs.

the query parameters are coming directly from PHP, so prepared statements probably aren't necessary, you may be able to improve efficiency by swapping them for query()s. (speaking for the queries that only require a session id, inserting session data obviously needs to be prepared.)