Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.In the flag module there's a check to identify whether cookies are enabled using session_api_available().
//From flag_flag.inc
$sid = flag_get_sid($uid, TRUE);
// Anonymous users must always have a session id.
if ($sid == 0 && $account->uid == 0) {
$this->errors['session'] = t('Internal error: You are anonymous but you have no session ID.');
return FALSE;
}
///////////////////////////////////////////////////////////////
function flag_set_sid($uid = NULL, $create = TRUE) {
$sids = &drupal_static(__FUNCTION__, array());
if (!isset($uid)) {
$uid = $GLOBALS['user']->uid;
}
// Set the sid if none has been set yet. If the caller specified to create an
// sid and we have an invalid one (-1), create it.
if (!isset($sids[$uid]) || ($sids[$uid] == -1 && $create)) {
if (module_exists('session_api') && session_api_available() && $uid == 0) {
// This returns one of the following:
// - -1. This indicates that no session exists and none was created.
// - A positive integer with the Session ID when it does exist.
$sids[$uid] = session_api_get_sid($create);
}
else {
$sids[$uid] = 0;
}
}
// Keep the -1 case internal and let the outside world only distinguish two
// cases: (1) there is an SID; (2) there is no SID (-> 0).
return $sids[$uid] == -1 ? 0 : $sids[$uid];
}
This check is preventing flag from working properly (throwing "Internal error: You are anonymous but you have no session ID") while using a varnish layer which strips out all cookies during the first load of the page.
Even if varnish vcl have excluded SESS* and session_api_session cookies from stripping, session_api_get_sid($create) will never be executed as session_api_available() always returns false.
/**
* Determine if cookies are enabled.
* From session_api.module
*/
function session_api_available() {
return !empty($_COOKIE);
}
</code>
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | is_this_the_correct_way-2416313-8.patch | 1.67 KB | marabak |
| #6 | is_this_the_correct_way-2416313-6.patch | 1.73 KB | Eli-T |
Comments
Comment #1
dropchew CreditAttribution: dropchew commentedComment #2
dropchew CreditAttribution: dropchew commentedComment #3
dropchew CreditAttribution: dropchew commentedComment #4
leewillis77 CreditAttribution: leewillis77 commentedWe've just hit this on the site. I'd concur with the original reporter in that the function session_api_available seems to be faulty. I'd vote for either removing the function entirely (What is it supposed to be achieving?) - or if you're worried about back-compatibility with other modules, just replace it with:
return TRUE;Comment #5
leewillis77 CreditAttribution: leewillis77 commentedComment #6
Eli-TWe hit this issue using Fastly. After modifying the vcl to let the session_api_session cookie through, it still wouldn't be set for anonymous users, as the anonymous cookies would be stripped by Fastly.
This patch gives a configuration option to bypass the check in session_api_available() and an explanation in the config form as to why you might want to do that.
(it also deletes all the session_api variables on uninstall as per good practice).
Comment #7
a.ross CreditAttribution: a.ross commentedI patched that piece of code from Flag. It was the result of some research into how session_api works.
The bit that I never quite understood was the session_api_available function (it felt weird to me too). But since I never worked with pressflow, I didn't trust myself enough to change it. I guess since the Pressflow site hasn't been updated since 2012, it's safe to ignore it by now?
However, I already did some patching of session API, and what will happen now is that the module will attempt to start the session anyway (when get_sid is called). Actually, calling code shouldn't have to use
session_api_available()becausesession_api_get_sid()already does that. I guess I didn't realize that at the time I patched Flag. What will happen is that session API "doesn't work" at the first request, but then tries to set a cookie via $_SESSION. If the client accepts that cookie and sends it with the next request, then Session API works as expected.Perhaps the module just shouldn't try to do anything clever with browsers that don't support cookies or have cookies disabled. The only problem is that the sessions table would grow unwieldy. I'd like some more input on this before deciding what to do.
Comment #8
marabak CreditAttribution: marabak at Lumini commentedsame patch against 1.0-rc1
Comment #9
indigoxela CreditAttribution: indigoxela commentedI suspect, we'll see a little more activity here soon, The most recent D7 update removes the "has_js" cookie by default, so it's more likely that the odd check for a non-empty $_COOKIE will fail in many browsers.
What seems clear so far:
That check in session_api_available() is unreliable. For several reasons already posted here.
That's exactly what came to my mind. Or a little shorter "heck, why?".
Are there assumptions about percentage of growth? Will removing that check lead to sessions stored for each and every bot? Because that could be significant and I'm not sure if we should do that.
Comment #10
a.ross CreditAttribution: a.ross commentedWell the ideal solution would be some javascript snippet to start the session. However, that has the disadvantage that session api won't be available at the first page load. A full page reload could "solve" that, although it's not very elegant.
Just accepting defeat and starting a session for every client (including bots) would have none of these issues, but would cause a lot of noise in the sessions table for sites that are crawled a lot.
Thinking about it, it could be a setting to enable either the JS solution or the "blindly accept" solution. But let's start with a "blindly accept", and if anyone starts complaining about DB deadlocks or whatever then we can implement the JS alternative.
Comment #11
indigoxela CreditAttribution: indigoxela commentedThe question I can't answer is: do sessions get created for crawlers, anyway?
I tried to play a bit with wget, with function session_api_available() only returning TRUE, and was able to create records in both, table sessions and session_api, but realized that there's some additional protection. Like the token for javascript toggle links.
Agreed.
Is the current patch approach still OK? It's marked as "needs work", but I'm not sure, why. It applies to 7.x-1.0-rc1, but I didn't test dev yet.
Comment #12
a.ross CreditAttribution: a.ross commentedThere's a couple of problems:
- it shouldn't be an option, func should just return TRUE
- it also adds an uninstall function which is not a part of the issue (even if valid)
Anyway, talking about this costs more time than fixing it, so release incoming.
Comment #14
a.ross CreditAttribution: a.ross commented