I'm currently working on implementing Session API support for Flag module, allowing anonymous users to flag content without creating an account. See #271582: Allow anonymous users to flag content.

I've been looking over the cleanup functionality for Session API, and I can't quite see the rational around a lot of the it. Session API provides a hook_session_api_cleanup(), but the two modules that are featured on the project page (Session Favorites and Domain User Default) don't implement this hook at all. They also don't seem to implement a hook_cron(), so do both of these modules just accumulate stale records from anonymous users whose sessions have expired?

It seems to me on any large scale site, allowing such an accumulation of old data would be crippling. Especially if search bots go through the site and start generating all kinds of bogus anonymous sessions.

Some other things that seems suspicious, session cleanup is actually disabled by default. Considering this could have devastating impacts on sites that have this disabled (potentially accumulating millions of sessions records), why not turn it on by default? Or even better, why make it an option at all? It seems like something all sites would need and I can't see any reason to clutter up the administrative interface with an option that would only have negative impact. Any modules that absolutely need further configuration could provide their own settings page.

So here's a patch with some changes that might make this module less likely to cause problems. It passes a list of outdated session IDs to modules in hook_session_api_cleanup() so that they can remove any matching records from their database tables. It also removes the administrative section and options for disabling cleanup. I haven't tested these changes yet.

CommentFileSizeAuthor
session_api_cleanup.patch5.02 KBquicksketch

Comments

quicksketch’s picture

Status: Active » Needs review
jhedstrom’s picture

Status: Needs review » Fixed

Thanks quicksketch. I've applied this to 6.x dev.

I agree that the system was needlessly complicated. That's a result of providing a hook and never actually implementing it :(

BTW, I'm stoked that flags is going to provide anonymous user support.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.