This is a follow-up of #3018713: Add optional deletion of anonymous flaggings whose session has expired
The MR adds a cron job that deletes anonymous flaggings. Now it deletes a flagging when the flagging is older than the session timeout (session.gc_maxlifetime).
But the age of the flagging is not the same as the state of the session. A visitor can use one session for a long time. Such a visitor comes back every day, the session is always alive, but their old flaggings become older than the timeout. Then cron deletes them, and the visitor loses their favorites. The session was never expired.
So the correct check is not "how old is the flagging" but "does the session still exist". Sessions are stored in the {sessions} table. The original cleanup script from the issue summary did this check.
I see some problems with this approach:
1. Flag module saves a random string in the session_id field of the flagging, see FlagService::getAnonymousSessionId(). It is not the real PHP session id. The {sessions} table stores the real session id, hashed with Crypt::hashBase64(). So today we can not compare the flagging session_id with the sessions table. To make this possible we need to store the real session id in the flagging. This changes the data model, and we need an update path for existing flaggings.
2. The {sessions} table exists only with the default database session backend. If the site uses Redis or Memcached for sessions, the table is empty. Then every session looks deleted, and cron will delete all anonymous flaggings. We need to detect the session backend, or skip the check in this case.
3. Race condition: a session can appear between the moment when we read the sessions list and the moment when we delete the flaggings. To be safe, we should delete only flaggings that are older than some margin, for example one cron run.
Maybe we can combine both checks: delete only flaggings that are older than the timeout AND whose session does not exist anymore. The age check works as a pre-filter, the session check gives the correct result.
Also note: core does not have a hook or event when a session expires, SessionHandler::gc() just deletes the rows. So cron is still the only way. This was already discussed in #3018713: Add optional deletion of anonymous flaggings whose session has expired.
Comments
Comment #2
ivnish