Using session API to allow anonymous users to use Flag, the following error is thrown when flagging after cleaning cookies. I'm not sure if this is a problem with flag or with Session API.

PDOException: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'sid' at row 1: INSERT INTO {flag_content} (fid, content_type, content_id, uid, sid, timestamp) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => 2 [:db_insert_placeholder_1] => node [:db_insert_placeholder_2] => 600 [:db_insert_placeholder_3] => 0 [:db_insert_placeholder_4] => -1 [:db_insert_placeholder_5] => 1338975474 ) in flag_flag->_flag() (line 724 of sites/all/modules/flag/flag.inc).

Files: 
CommentFileSizeAuthor
#93 1619114.93.flag_.invalid-session-id.patch2.7 KBjoachim
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1619114.93.flag_.invalid-session-id.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#90 invalid-session-1619114-89.patch2.7 KBa.ross
PASSED: [[SimpleTest]]: [MySQL] 266 pass(es).
[ View ]
#88 invalid-session-1619114-88.patch1.12 KBa.ross
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch invalid-session-1619114-88.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#79 flag-fix_pdoexception_bug-1619114-79.patch913 bytesShabana Blackborder
PASSED: [[SimpleTest]]: [MySQL] 254 pass(es).
[ View ]
#75 flag-fix_pdoexception_bug-1619114-75.patch913 bytescspurk
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
#67 flag-fix_pdoexception_bug-1619114-67.patch913 bytescspurk
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]
#67 flag-fix_get_user_count_session-1619114-67.patch451 bytescspurk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch flag-fix_get_user_count_session-1619114-67.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#48 invalid-session-1619114-48.patch614 bytesa.ross
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]
#44 invalid-session.patch476 bytesa.ross
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]
#34 flag_forceset_oncreate.patch467 bytessmartango
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]
#26 exception-on-rule-flag.patch2.07 KBsmartango
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]
#15 krumo-backtrace.zip12.13 KBa.ross
#7 flag-anonymous-pdo-7.patch529 bytesa.ross
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]

Comments

Component:Flag core» Rules integration

I should note that it happens only when the flag is automatically created by Rules. It doesn't happen when manually flagged by the user. So this should probably be targeted at the rules integration.

More information:
We have a rule setup to flag content when viewed to be able to display a browsing history. Content is first unflagged and then flagged, to preserve the correct order. Anyway, when content is viewed once, the node is flagged for the first time. Then, when the user clears his cache and views the same content again, this error displays.
I've been able to reproduce this locally and it seems that manually unflagging the previously flagged content fixes the problem.

[copied from #1088672: Duplicate fid-node key error when flagging content ]

maybe i found something ..
adding
$create = TRUE;

at line 2041 of flag.module, make it works again

<?php
function flag_set_sid($uid = NULL, $create = TRUE) {
  static
$sids = array();
  if (!isset(
$uid)) {
   
$uid = $GLOBALS['user']->uid;
  }
  if (!isset(
$sids[$uid])) {
    if (
module_exists('session_api') && session_api_available() && $uid == 0) {
     
$create = TRUE; // this fix the problem
     
$sids[$uid] = session_api_get_sid($create);
    }
    else {
     
$sids[$uid] = 0;
    }
  }
  return
$sids[$uid];
}
?>

also I think there is no reason to be FALSE there

Should a new session be requested at that point? I don't know enough about how Session API works to be sure.

looking at the code, in fact, session_api_get_sid with create setted to TRUE call

http://api.drupal.org/api/drupal/includes!session.inc/function/drupal_se...

that is safe, and should be called this way all time.

Actually this is the signature:

function session_api_get_sid($create = TRUE)

it would work:

<?php
function flag_set_sid($uid = NULL, $create = TRUE) {
  static
$sids = array();
  if (!isset(
$uid)) {
   
$uid = $GLOBALS['user']->uid;
  }
  if (!isset(
$sids[$uid])) {
    if (
module_exists('session_api') && session_api_available() && $uid == 0) {
     
$sids[$uid] = session_api_get_sid();
    }
    else {
     
$sids[$uid] = 0;
    }
  }
  return
$sids[$uid];
}
?>

however I still get -1 from that function sometime (I can't say when), and this should never happen from definition of session_api_get_sid with params TRUE ..

Anyone experiencing this?

I do tested

if ($sids[$uid] == -1) $sids[$uid] =0;

as workaround and this "works"

StatusFileSize
new529 bytes
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]

My findings suggest that passing TRUE works. I only got the PDO error after cleaning cookies, but the error is now gone. However, I'm a bit confused about the code:

<?php
function flag_get_sid($uid = NULL, $create = FALSE) {
  return
flag_set_sid($uid, $create);
}
?>

This function is only ever called with a single argument in flag.module, so why does it have two arguments? Also, it's just a wrapper around flag_set_sid(). Is this for legacy reasons? This is the function declaration of flag_set_sid().
<?php
function flag_set_sid($uid = NULL, $create = TRUE) {
?>

This function only gets called by flag_get_sid(), its wrapper, so the default value for $create is never even used.

Anyway, I've attached a patch that does not touch all this, but it seems to me it could be much simpler.

A get/set pair where one has a static and one is a wrapper is a common Drupal pattern. That's ok.

But I'm rather confused that the two have different parameters:

function flag_set_sid($uid = NULL, $create = TRUE) {
function flag_get_sid($uid = NULL, $create = FALSE) {

And I'm really unsure about changing the behaviour with session ids... what are the consequences?

Yeah, I've seen this pattern a number of times, but in this case it just seems redundant.

As far as consequences, I don't seem to be getting a new session upon every time I flag content, which is the only thing I can think of that could go wrong. However, when I clean cookies, it correctly recognizes a new session upon flagging.

joachim that make sense, so why do call flag_get_sid and not flag_set_sid? or why don't pass TRUE? (in _flag() method)

and having a sid equal -1 code should just leave and not try to set (so testing if sid is less than 0 would make error disappear, but things flag + session api do not work as expected: no flag)

Use of set variant is the better choice for me.

joachim I read the comment in #1088672-19: Duplicate fid-node key error when flagging content about the normal behaviour, this persuades me that it have to be called without parameters.

In http://drupal.org/node/319656 is not documented the availability of parameter.

also http://drupalcode.org/project/session_api.git/commit/81b1a9b2fb998984fb5... this commit says it is the port of the patch for #785292: Incompatible with Pressflow (create parameter), but I can't find when and if flag should use this flag setted to false, or do not care at all.
(finding this commit could explain more)

The -1 behavior is wanted, but, at least, it is not compatible with flag db table definition

The same bag, i don't know when it happens ....

PDOException: SQLSTATE[22003]: Numeric value out of range: 1264 Out of range value for column 'sid' at row 1: INSERT INTO {flag_content} (fid, content_type, content_id, uid, sid, timestamp) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => 3 [:db_insert_placeholder_1] => node [:db_insert_placeholder_2] => 17 [:db_insert_placeholder_3] => 0 [:db_insert_placeholder_4] => -1 [:db_insert_placeholder_5] => 1343879411 ) в функции flag_flag->_flag()

This was changed in commit 87783505a245e5bc5e2f02bc5c0a5ad337d6d9b2:

@@ -1803,11 +1803,8 @@ function flag_check_token($token, $content_id) {
/**
  * Set the Session ID for a user. Utilizes the Session API module.
- *
- * This function is only called in flag_init(), to set the current user's
- * SID in case the user logs in during this request.
  */
-function flag_set_sid($uid = NULL) {
+function flag_set_sid($uid = NULL, $create = TRUE) {
   static $sids = array();
   if (!isset($uid)) {
@@ -1816,7 +1813,7 @@ function flag_set_sid($uid = NULL) {
   if (!isset($sids[$uid])) {
     if (module_exists('session_api') && session_api_available() && $uid == 0) {
-      $sids[$uid] = session_api_get_sid();
+      $sids[$uid] = session_api_get_sid($create);
     }
     else {
       $sids[$uid] = 0;
@@ -1828,7 +1825,14 @@ function flag_set_sid($uid = NULL) {
/**
  * Get the Session ID for a user. Utilizes the Session API module.
+ *
+ * @param $uid
+ *   The user ID. If the UID is 0 (anonymous users), then a SID will be
+ *   returned. SID will always be 0 for any authenticated user.
+ * @param $create
+ *   If the user doesn't yet have a session, should one be created? Defaults
+ *   to FALSE.
  */
-function flag_get_sid($uid = NULL) {
-  return flag_set_sid($uid);
+function flag_get_sid($uid = NULL, $create = FALSE) {
+  return flag_set_sid($uid, $create);
}

The issue is: #1258764: Flag starts a session on anonymous user loads which is bad for pressflow/varnish.

Given that d.org runs Pressflow, this isn't something we can change without understanding it.

Status:Active» Postponed (maintainer needs more info)

> the following error is thrown when flagging after cleaning cookies. I'm not sure if this is a problem with flag or with Session API.

This must be because somewhere in the chain a session ID should be getting requested but isn't.

Could someone with this problem do a backtrace please?

StatusFileSize
new12.13 KB

I've done a ddebug_backtrace() from the function _flag(). What's the generally accepted way to post backtraces on d.o?

Here's something at least, in the attachment is the HTML (without krumo js though):

26: flag_flag->_flag() (Array, 2 elements)
25: flag_flag->flag() (Array, 2 elements)
24: flag_node->flag() (Array, 2 elements)
23: flag_rules_action_flag() (Array, 2 elements)
22: call_user_func_array() (Array, 1 element)
21: FacesExtendable->__call() (Array, 2 elements)
20: RulesExtendable->__call() (Array, 2 elements)
19: RulesAction->executeCallback() (Array, 2 elements)
18: RulesAbstractPlugin->evaluate() (Array, 2 elements)
17: RulesActionContainer->evaluate() (Array, 2 elements)
16: Rule->evaluate() (Array, 2 elements)
15: RulesReactionRule->evaluate() (Array, 2 elements)
14: RulesActionContainer->evaluate() (Array, 2 elements)
13: RulesEventSet->executeByArgs() (Array, 2 elements)
12: rules_invoke_event() (Array, 2 elements)
11: rules_entity_view() (Array, 2 elements)
10: call_user_func_array() (Array, 1 element)
9: module_invoke_all() (Array, 2 elements)
8: node_build_content() (Array, 2 elements)
7: node_view() (Array, 2 elements)
6: node_view_multiple() (Array, 2 elements)
5: node_show() (Array, 2 elements)
4: node_page_view() (Array, 2 elements)
3: page_manager_node_view_page() (Array, 2 elements)
2: call_user_func_array() (Array, 1 element)
1: menu_execute_active_handler() (Array, 2 elements)
0: main() (Array, 2 elements)

joachim backtrace is 7 megs ... I had some difficulties understanding it ...

Anyway (thank you for finding that), summary says:

Currently, if flags are enabled for anonymous users there is a check on every page to see if the user has created a flag. This forces session_api to create and save session data for this anonymous user. It would be better for caching systems to only create that session data once the user actually flags something.

ok, fine. In flag.inc flag_get_sid is in 3 points, I just check this one:

flag_flag::access() method: return TRUE if user can flag the content, called as flag_get_sid($uid) (do not create). How do you check that user can or cannot flag a content, if you do not start session for anonymous user?

the code is:

<?php
 
function access($content_id, $action = NULL, $account = NULL) {
    if (!isset(
$account)) {
     
$account = $GLOBALS['user'];
    }
    if (isset(
$content_id) && !$this->applies_to_content_id($content_id)) {
     
// Flag does not apply to this content.
     
return FALSE;
    }
    if (!isset(
$action)) {
     
$uid = $account->uid;
     
$sid = flag_get_sid($uid);
     
$action = $this->is_flagged($content_id, $uid, $sid) ? 'unflag' : 'flag';
    }
   
// Base initial access on the user's basic permission to use this flag.
   
$access = $this->user_access($action, $account);
   
// Check for additional access rules provided by sub-classes.
   
$child_access = $this->type_access($content_id, $action, $account);
    if (isset(
$child_access)) {
     
$access = $child_access;
    }
   
// Allow modules to disallow (or allow) access to flagging.
   
$access_array = module_invoke_all('flag_access', $this, $content_id, $action, $account);
    foreach (
$access_array as $set_access) {
      if (isset(
$set_access)) {
       
$access = $set_access;
      }
    }
    return
$access;
  }
?>

why do not just pass TRUE there? or use flag_set_sid($uid) ?

if there's a need to check the user can flag or unflag a content before starting session, it should be done before (for both action), then if there is a chance (one of two), create the session and check what action is needed for that content.

Say add a method _actions_access($content_id,$account) return an array of access, if one is true, start sid and check if action needed is the one returned as true.

Is this acceptable? or just start session there?

> What's the generally accepted way to post backtraces on d.o?

That looks pretty good :)

But could you put the backtrace at the point where flag_get_sid() returns a -1 please?

Ok, what about this one:

... (Array, 17 elements)
    16: session_api_get_sid() (Array, 2 elements)
        file (String, 51 characters ) sites/all/modules/session_api/session_api.modul...
        args (Array, 1 element)
            0 (Boolean) FALSE
    15: flag_set_sid() (Array, 2 elements)
        file (String, 39 characters ) sites/all/modules/flag/flag.module:2053
        args (Array, 2 elements)
            0 (Integer) 0
            1 (Boolean) FALSE
    14: flag_get_sid() (Array, 2 elements)
        file (String, 39 characters ) sites/all/modules/flag/flag.module:2074
        args (Array, 1 element)
            0 (Integer) 0
    13: flag_flag->access() (Array, 2 elements)
        file (String, 35 characters ) sites/all/modules/flag/flag.inc:471
        args (Array, 1 element)
            0 (String, 2 characters ) 70
    12: flag_link() (Array, 2 elements)
        file (String, 38 characters ) sites/all/modules/flag/flag.module:254
        args (Array, 3 elements)
            0 (String, 4 characters ) node
            1 (Object) stdClass
            2 (Boolean) FALSE
    11: flag_entity_view() (Array, 2 elements)
    10: call_user_func_array() (Array, 1 element)
     9: module_invoke_all() (Array, 2 elements)
     8: node_build_content() (Array, 2 elements)
     7: node_view() (Array, 2 elements)
     6: node_view_multiple() (Array, 2 elements)
     5: node_show() (Array, 2 elements)
     4: node_page_view() (Array, 2 elements)
     3: page_manager_node_view_page() (Array, 2 elements)
     2: call_user_func_array() (Array, 1 element)
     1: menu_execute_active_handler() (Array, 2 elements)
     0: main() (Array, 2 elements)

Status:Postponed (maintainer needs more info)» Active

I've just tried to reproduce this and I can't.

Steps I took:

1. Open site in Safari
2. Flag some nodes without logging in
3. Cleared cookies.
4. Reloaded the page
5. Tried to flag a node.

Thanks for the backtrace.

Looks like flag_flag->access() calls flag_get_sid() without the parameter to get a new session ID if needed. But the thing is, I'm not sure flag_flag->access() can really know at this point whether it should or shouldn't ask for one. Or at least, I don't how how that should be determined.

Well, the error I'm facing may be slightly different from the error smartango is having. I only get the error when flagging with Rules, as you can see in the former backtrace :)

a.ross it is exactly the same : error on rule: access content -> flag content

Also, backtrace says when error happen, not when session should be started in order to not raise error.

<?php
13
: flag_flag->access() (Array, 2 elements)
       
file (String, 35 characters ) sites/all/modules/flag/flag.inc:471
        args
(Array, 1 element)
           
0 (String, 2 characters ) 70
    12
: flag_link() (Array, 2 elements)
       
file (String, 38 characters ) sites/all/modules/flag/flag.module:254
        args
(Array, 3 elements)
           
0 (String, 4 characters ) node
            1
(Object) stdClass
            2
(Boolean) FALSE
?>

flag_link should had already started the session for this .. why do you don't read my comment at #16 ?
using flag_set_sid in access method fix my problem, and I think yours too. If also has to be checked if anonymous user have a chance to set/unset a content before start the session to check what action has to be taken on that content, then it is needed a wider change (not so wide)

> flag_link should had already started the session for this

I'm not sure it should.
The change in the other issue I linked to is about not creating a session for an anon user *until* a flagging action takes place.
In other words, don't create one when outputting links.

> The change in the other issue I linked to is about not creating a session for an anon user *until* a flagging action takes place.
> In other words, don't create one when outputting links.

"using flag_set_sid in access method fix my problem .." is related to access method (called from flag_link)

the flag_link code comment says:

<?php
...
if (!
$flag->access($content_id) && (!$flag->is_flagged($content_id) || !$flag->access($content_id, 'flag'))) {
     
// User has no permission to use this flag or flag does not apply to this
      // content. The link is not skipped if the user has "flag" access but
      // not "unflag" access (this way the unflag denied message is shown).
     
continue;
    }
..
?>

and in access method code there is:

<?php
   
if (!isset($action)) {
     
$uid = $account->uid;
     
$sid = flag_get_sid($uid);
     
$action = $this->is_flagged($content_id, $uid, $sid) ? 'unflag' : 'flag';
    }
    ..
// code to check action on content_id ($sid is not used below)
?>

if instead reverse the check: which action is permitted on content_id by user account (anonymous), if at least one, check state of content for this user.

To test the content_id flag status of an anonymous user YOU HAVE TO START SESSION there's no other way.

the original question (1095910):

Is there a way you could only start the session when something has actually happened that needs flag to record data?

has the answer: flag not only record data, also show data status to user, so session has to be start when there is data recorded, but you can not know if data is recorded if you do not start session ... or shortly: No.

The only option is to check if content shown is not flag-able and not unflag-able by anonymous, if not to both do not start session, otherwise .. yes.

or just state flag is incompatible with caching of pressflow.

.. I just want to make clean:

<?php
function access($content_id, $action = NULL, $account = NULL) {
    if (!isset(
$account)) {
     
$account = $GLOBALS['user'];
    }
    if (isset(
$content_id) && !$this->applies_to_content_id($content_id)) {
     
// Flag does not apply to this content.
     
return FALSE;
    }
   
$access_actions = $this->_access_actions($content_id,$account);
    if(!
$access_actions['flag'] && !$access_actions['unflag']) return FALSE;
    if (!isset(
$action)) {
     
$uid = $account->uid;
     
$sid = flag_set_sid($uid);
     
$action = $this->is_flagged($content_id, $uid, $sid) ? 'unflag' : 'flag';
    }
    return
$access_actions[$action];
  }
  private function
_access_actions($content_id,$account) {
   
$actions = array('flag','unflag');
    foreach(
$actions as $action) {
     
// Base initial access on the user's basic permission to use this flag.
     
$access = $this->user_access($action, $account);
     
// Check for additional access rules provided by sub-classes.
     
$child_access = $this->type_access($content_id, $action, $account);
      if (isset(
$child_access)) {
   
$access = $child_access;
      }
     
// Allow modules to disallow (or allow) access to flagging.
     
$access_array = module_invoke_all('flag_access', $this, $content_id, $action, $account);
      foreach (
$access_array as $set_access) {
    if (isset(
$set_access)) {
     
$access = $set_access;
    }
      }
     
$access_actions[$action] = $access;
    }
    return
$access_actions;
  }
?>

realistically access is called before any action taken. This change is tested and work. It could be a viable compromise

@smartango, I forgot for a moment that you'd mentioned Rules, but do you also get it after cleaning cookies? Because that's the only time I'm getting it (at least reliably).

> or just state flag is incompatible with caching of pressflow.

That's not going to fly -- both are in use on d.org.

> To test the content_id flag status of an anonymous user YOU HAVE TO START SESSION there's no other way.

Right, but when links are being output, if there's no session in existence, presumably the desired behaviour intended by #1258764: Flag starts a session on anonymous user loads which is bad for pressflow/varnish is to say 'If there's no session, there's no existing flagging data, therefore everything is unflagged'. And then only if the user tries to flag something should the session get created.

Could you post changes as patch files btw -- it does make it much easier to understand what changes you are suggesting.

StatusFileSize
new2.07 KB
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]

It happen when I start private navigation, then open a page for which it apply a rule, rule is:

<?php
{ "rules_prodotto_visitato" : {
   
"LABEL" : "Prodotto Visitato",
   
"PLUGIN" : "reaction rule",
   
"REQUIRES" : [ "uc_product", "rules", "flag" ],
   
"ON" : [ "node_view" ],
   
"IF" : [
      {
"node_is_product" : { "node" : [ "node" ] } },
      {
"data_is" : { "data" : [ "view-mode" ], "value" : "full" } }
    ],
   
"DO" : [
      {
"flag_unflagnode" : {
         
"flag" : "visitato",
         
"node" : [ "node" ],
         
"flagging_user" : [ "site:current-user" ],
         
"permission_check" : 0
       
}
      },
      {
"flag_flagnode" : {
         
"flag" : "visitato",
         
"node" : [ "node" ],
         
"flagging_user" : [ "site:current-user" ],
         
"permission_check" : 0
       
}
      }
    ]
  }
}
?>

there is no other flagable content. When I visit that content type, I got PDOException. If I log in, logout, and visit a product (to which apply rule), I got PDOExcetion.

I tried to skip permission check on flag in the rule, but nothing changed.

Smartango, could you try manually flagging and unflagging a product (with the standard flag buttons) and then see if the PDO exception still happens upon viewing the content? In my case, after manually flagging/unflagging, the exception is gone. Until I clear cookies again.

If it were possible to reproduce this bug without Rules involved, that would be a big help.

ok, the test is
- define another flag for other content type (article)
- show link on that content type
- visit an article
- visit product (error)
- flag article
- visit product: worked

So without rule involved it seems not possible to reproduce .. maybe something in action rule are different from flag via

http://rafaels.u84/flag/flag/flagarticle/13?destination=node/13&token=ec...

I also tested this, after restarting anonymous:

http://rafaels.u84/flag/unflag/flagarticle/13?destination=node/13&token=...

(copying link and changing flag to unflag). It worked and made rule flag work. (just to be sure, the rule first unflag then flag content, to update time)

After some testing I can say that the problem is no flag_set_sid code:

(in pseudo diff syntax)

<?php
function flag_set_sid($uid = NULL, $create = TRUE) {
  static
$sids = array();
  if (!isset(
$uid)) {
   
$uid = $GLOBALS['user']->uid;
  }
-  if (!isset(
$sids[$uid])) {
+  if (!isset(
$sids[$uid])|| $sids[$uid]==-1) {
    if (
module_exists('session_api') && session_api_available() && $uid == 0) {
     
$sids[$uid] = session_api_get_sid($create);
    }
    else {
     
$sids[$uid] = 0;
    }
  }
  return
$sids[$uid];
}
?>

because it is good to have static for performance, but if sid is setteted for anonymous as -1 (there was a previous call to flag_get_sid()) it never get the chance to start the sessione even in real flag action, when it should.

just this change and thing should start to work.

session is not started in other case, such as visiting a content, thus it would be ok for the other bug

My rule originally had the unflag action also, but reproducing it on my test environment I omitted it.

I think it would be better to not set a -1 into the static $sids array: so change this line:

      $sids[$uid] = session_api_get_sid($create);

actually I cant immagine a scenario when this array is usefull at all, but maybe I have not a so wide vision ..

I prefer this:

<?php
 
if (!isset($sids[$uid]) || ($sids[$uid]==-1 && $create)) {
    if (
module_exists('session_api') && session_api_available() && $uid == 0) {
     
$sids[$uid] = session_api_get_sid($create);
    }
    else {
     
$sids[$uid] = 0;
    }
  }
 
//return ($sids[$uid]<0)?0:$sids[$uid]; // this?
 
return $sids[$uid];
?>

if that check (this?) in return was there before, we never get notice of problem, and it would have been harder to find why flag are not setted for anonymous by a rule. Also (the rest of) code seems to works, and $sids array has a reason for uid 0: skip get sid call.

in fact returned sid is supposed to be 0 for no session or greater for session available, and test are in form of $sid != 0, $sid==0, and not $sid>0, $sid<=0 ... I suppose it could be a problem, but i do not know.

StatusFileSize
new467 bytes
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]

this one line change work, please test this patch in real world.

A test could be:

1. define a rule (with flag action)
2. visit a content that cause the action of the rule in 1. as anonymous user

with session_id enabled.

Like I said, I'd rather we didn't store a -1 sid in the first place. It doesn't strike me as useful or meaningful in any way.

Also, complex expressions like that need a code comment in my book. Parsing that makes my head hurt :)

which expression? this:

<?php
if (!isset($sids[$uid]) || ($create && $sids[$uid]==-1)) {
..
?>

$sids not setted for that uid, or the function is called with create option TRUE AND $sids[$uid] was setted by a previous call of session_api_get_sid(); (the only code that set it to -1).

If not set on create we need:

<?php
 
if (!isset($sids[$uid]) || ($create && $uid==0 && $sids[$uid]==0) {
    if (
module_exists('session_api') && session_api_available() && $uid == 0) {
     
$r = session_api_get_sid($create);
     
$sids[$uid] = ($r>=0)?$r:0;
    }
    else {
     
$sids[$uid] = 0;
    }
  }
...
?>

not this way .. cleanup

<?php
      
// anon       AND  (not setted)  OR (setted to 0  && create)
  
if ( $uid == 0 && ( !isset($sids[$uid]) || ($sids[$uid]==0 && $create) )
     if(
module_exists('session_api') && session_api_available()) {
      
$r = session_api_get_sid($create);
      
$sids[$uid] = ($r>=0)?$r:0;
     }
  }
   if (!isset(
$sids[$uid])) $sids[$uid] = 0;
   return
$sids[$uid];
?>

But it look to me more complex than the patch, and not so efficient.

instead something like this:

<?php
if (isset($sids[$uid]) && $sids[$uid]==-1 && $create) {
 
// I know module exists: who other set it to -1? also know that is $uid=0: which other $uid could have sids = -1?
 
$sids[$uid] = session_api_get_sid($create);
}
if (!isset(
$sids[$uid])) {
 
// just move simple test at begin
 
if ($uid == 0 && module_exists('session_api') && session_api_available()) {
   
$sids[$uid] = session_api_get_sid($create);
  }
  else {
   
$sids[$uid] = 0;
  }
}
return
$sids[$uid]>=0?$sids[$uid]:0;
?>

ok, a lot of idea, the problem is how to take advance of static $sids and reduce function call, also having something that is stable and do not break other module. If $sids array exists it should (or could) be used elsewhere.

just in case, can I propose a greater change that make it efficent and clean? like:

<?php
function flag_set_sid($uid = NULL, $create = TRUE) {
  static
$sids = array();
  static
$has_session_api = FALSE;
  if (!isset(
$uid)) {
   
$uid = $GLOBALS['user']->uid;
  }
  if(
$has_session_api && $create && $uid ==0) {
      
// $has_session_api => $sids[$uid]==0
       // session_api_available() below says user agent has cookie enabled, so session_api_get_sid does not return -1
      
$sids[$uid] = session_api_get_sid($create);
      
$has_session_api = FALSE;
  }
  if (!isset(
$sids[$uid])) {
    if (
$uid == 0 && module_exists('session_api') && session_api_available()) {
     
$sids[$uid] = 0;
     
$r = session_api_get_sid($create);
      if(
$r>0) {
       
$sids[$uid] = $r;
      } else {
       
$has_session_api = TRUE;
      }
    }
    else {
     
$sids[$uid] = 0;
    }
  }
  return
$sids[$uid];
}
?>

session_api_get_sid is called at most 2 time, module_exists and session_api_available, at most 1 time, $sids array is maintained in a correct state (check syntax, I wrote down here in the html textarea, no check)

Yup, this:

> if (!isset($sids[$uid]) || ($create && $sids[$uid]==-1)) {

I'd like something that complex to come with a comment that explains what it's doing.

But like I said, I don't think this is the right way to go. Not that I'm sure my suggestion is right either .... We need to properly understand what flag_set_sid() is trying to do:

- it keeps static cache of sids set so far
- if the given $uid does not have a cached sid, it tries to get one.
-- if SessionAPI is installed and the user is anonymous, it gets one from session_api_get_sid()
-- otherwise, the session id is just 0.

So I would say this:

    if (module_exists('session_api') && session_api_available() && $uid == 0) {
      $sid = session_api_get_sid($create);
      sids[$uid] = $sid > 0 ? $sid : 0;
    }
    else {
      $sids[$uid] = 0;
    }

That's one extra line than the patch, yes, but that's really not a big deal.

EDIT: two extra lines, because that should have a comment on it too!


- it keeps static cache of sids set so far
- if the given $uid does not have a cached sid, it tries to get one.
-- if SessionAPI is installed and the user is anonymous, it gets one from session_api_get_sid()
-- otherwise, the session id is just 0.

there is something missing here:
- call flag_get_uid(0) [ which call flag_set_sid($uid,FALSE) ] returns 0 and set static $sids[0] to 0
- call flag_set_uid(0) [ which has $create = TRUE as default ] return 0 never start session [WRONG]

wrong because it cause the session to not start even if is intended by the call semantic: we should not care about what is setted in array, now it is called the set flavour and session should be started.

It is not the problem on how many line has a patch, but if it is the right code to do it. One can choose to reduce call number, or to be more elegant, (the better would be to achieve both), but at least it has to work in the right way.

If comment is missing this is the code:

<?php
function flag_set_sid($uid = NULL, $create = TRUE) {
  static
$sids = array();
  if (!isset(
$uid)) {
   
$uid = $GLOBALS['user']->uid;
  }
 
// not setted or previously setted by session_api_get_sid(FALSE): this func called with $create==FALSE
 
if (!isset($sids[$uid]) || ($sids[$uid]==-1 && $create)) {
    if (
module_exists('session_api') && session_api_available() && $uid == 0) {
     
$sids[$uid] = session_api_get_sid($create);
    }
    else {
     
$sids[$uid] = 0;
    }
  }
  return
$sids[$uid];
}
?>

but actually the last snippet in #36 has comment and it seems clean, given the scenario i described here (that leads to wrong behaviour).

I like the last snippet in #36 because it is efficient, as explained

Ah yes I think I see what you mean.

If session API returns a -1, we want to later on be able to come back here and this time create a sid.

I confirm that #39 works perfectly fine for both authentication and non authenticated users

Needs making into a patch.

I was thinking, if it's just about handling the -1 special case, why not just do this?

<?php
function flag_set_sid($uid = NULL, $create = TRUE) {
 
$sids = &drupal_static(__FUNCTION__, array());
  if (!isset(
$uid)) {
   
$uid = $GLOBALS['user']->uid;
  }
  if (!isset(
$sids[$uid])) {
    if (
module_exists('session_api') && session_api_available() && $uid == 0) {
     
$sids[$uid] = session_api_get_sid($create);
      if (
$sids[$uid] == -1) {
       
$sids[$uid] = session_api_get_sid(TRUE);
      }
    }
    else {
     
$sids[$uid] = 0;
    }
  }
  return
$sids[$uid];
}
?>

StatusFileSize
new476 bytes
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]

The patch for the above code.

I also don't think it's a good idea to add another static. (and shouldn't statics use drupal_static?)

      if ($sids[$uid] == -1) {
        $sids[$uid] = session_api_get_sid(TRUE);
      }

The problem with that is that -- as far as I understand -- for performance reasons we shouldn't be asking Session API for a sid unless we really need it.

Yes but aren't the results cached in the $sids static? So you would only have to set it once?

Otherwise I can create a patch of #39 and clarify the doc line a bit.

StatusFileSize
new614 bytes
PASSED: [[SimpleTest]]: [MySQL] 61 pass(es).
[ View ]

Patch in the style of #39

Status:Active» Needs review

Ehi boy, why do you reset your brain every time you post a commemt???
That is called with create true or false....

Please read what it's said before

First of all, no need for offensive comments like that. Second, name the person you are addressing.
If you were referring to me, I created a patch of your code in #39, so I'm not sure what your point is.

ok I calm me down, and I apologize for my comment, but please do not close or commit patch before some test.

@a.ross I refer to #43 and #44, I was reading on cellphone, while a bit late for a visit.

But offensive comment maybe would hide the fear of wrong patch uploaded, and this path is not correct: to have a patch without at least two test cases (where it should set session, and where it shouldn't) is not a good practice.

Also with session_api dependencies, and without.

I stop blaming, I have to write a lot of tests in this moment, and I maybe I'll try to write some SimpleTest soon, I could spare time to write those for flag, with patch against git

Status:Needs review» Reviewed & tested by the community

Applied patch #48 and problem resolved.
Also it did not cause any problem for authenticated users

tx @a.ross

Version:7.x-2.x-dev» 7.x-3.x-dev
Status:Reviewed & tested by the community» Needs work

Needs to go on 3.x first.

Also, I'm still not happy that I don't understand what a -1 means from session_api_get_sid(). Ideally I'd like that function to get some clearer documentation before this goes in. I don't want code I don't understand and can't maintain.

@Joachim, have you seen my comment in #38? Unfortunately, the session API issue queue seems a bit deserted.

Heh, I even less want to be in charge of code that depends on code that's not maintained... :/

Well, according to what i see in code flag already depends on session_api, lets not forget that 5,000 site are using session_api. So someone else will take over or fork it if it's really deserted. It's not that difficult to findout about it's status. All you gotta do is contacting the maintainer :)

Well, someone who wants this patch to get in needs to contact the maintainer :D

Rules integration here is currently on minimal maintenance -- see #1736524: Rules maintainer / decouple Rules integration to a new project.

It don't think this patch is only related to rules.
It happens when an anonymous user flag a content
I also contacted session api maintainer, lets see how he's doing :)

patch #48 works for me.

Patch #48 is also working for me on 7.x-2.x.

That's great, and thanks for testing everyone, but I am still not happy with this code.

+++ b/flag.module
@@ -2057,7 +2057,9 @@ function flag_set_sid($uid = NULL, $create = TRUE) {
+  // Sets the sid if none was set. If the caller specified to create an sid and
+  // we have an invalid one, create it instead of returning the invalid sid.
+  if (!isset($sids[$uid]) || ($sids[$uid] == -1 && $create)) {
     if (module_exists('session_api') && session_api_available() && $uid == 0) {
       $sids[$uid] = session_api_get_sid($create);
     }

That means that the LAST time we came here, we returned an invalid ID. We're here now and finding an invalid ID in $sids because we previously came here and session_api_get_sid() gave us a -1, which we RETURNED, even though at the time it presumably was invalid too.

Which bit of Flag got the bad sid? What does it cause?

Has anyone contacted the maintainers of Session API to get clarification?

I thought Smartango at least planned to write some tests, but he hasn't showed up in this issue anymore. As for Session API, I linked to this issue before: #1233650: Make sure Session API is available on the first page load, haven't looked into that myself since that, but it may very well be related. In that case the "fix" we're working on here could be a workaround.

At the very least I'd like the sort of logic in the patch at #44.

Clearly this issue affects a lot of people, so it would be good to get some sort of fix in. However, if SessionAPI has problems, and they affect us, it would be great to see that sorted. Don't suppose anyone wants to volunteer to co-maintain it? :)

right a.ross, I like to be alerted, I planned tests for Sunday 9th.

EDIT 2012-12-09.
It is too much sunday here. I will reschedule this during first half of this week.

Test plan is:

• depends on session_api, rules, views (setUp() ok)
• create content type
• define flag
• define rule on content visited (definition in this issue)
• define the view (all flagged contents, order by flagged reverse)
• check no PDO exception
• check view for the content container in the view (".content .node-nid" as first child in view - check DOM tree)

For the content type, flag, rule, and view you can create a hidden test module that defines all these in standard hooks. Core does this for a few things.

StatusFileSize
new451 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch flag-fix_get_user_count_session-1619114-67.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new913 bytes
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]

Test Case

Here is a test case for when I get the PDOException; I hope you can use that to reproduce the problem:

  1. Implement hook_flag_access() like this:
    <?php
    function MY_MODULE_flag_access($flag, $content_id, $action, $account) {
     
    // only allow flagging with the "test_flag" up to 3 times
     
    if ($flag->name == 'test_flag' && $action == 'flag' && $flag->get_user_count($account->uid) >= 3) {
        return
    FALSE;
      }
      return
    TRUE;
    }
    ?>
  2. As the anonymous user, open a page with a test_flag flag link.
  3. Clicking the flag link produces the PDOException.

My explanation for the exception in this test case: when clicking the flag link, MY_MODULE_flag_access() (and also the access() method of the flag_flag) class) is called and will set the static $sids[$uid] in flag_set_sid() to -1 (via get_user_count()). Then the actual flagging action has to use this -1 SID which leads to the error.

Solution Proposal

In reply to #62:

That means that the LAST time we came here, we returned an invalid ID. We're here now and finding an invalid ID in $sids because we previously came here and session_api_get_sid() gave us a -1, which we RETURNED, even though at the time it presumably was invalid too.

Which bit of Flag got the bad sid? What does it cause?

Has anyone contacted the maintainers of Session API to get clarification?

I haven’t contacted the Session API maintainer. Still I’d like to add my two cents on why I believe -1 is returned from session_api_get_sid(). Basically session_api_get_sid() can return the following three values:

<SOME_SESSION_ID>
in case cookies are enabled and a session ID has either previously been set or $create == TRUE
FALSE
in case cookies are not enabled (→ the user’s problem)
-1
in case cookies are enabled, a session ID has not previously been set and $create == FALSE (→ the Flag module’s problem)

That’s the situation with which the Flag module has to cope: there are three cases/states to handle. If I’m not mistaken, then there is currently not a dedicated handling of the -1 case. As noted above, the -1 case is the Flag module’s responsibility, though, so it needs to be handled internally.

I would propose to entirely handle the -1 case in flag_set_sid() and to never show this state outside of this function. For the outside world, it could just look like the FALSE case for now (which appears to be handled throughout the Flag module already). We still have to differentiate the two cases internally so that we can update the internally stored session ID upon request ($create == TRUE). See the first attached patch to this comment which does exactly that and therefore eliminates the PDOException. The second patch attached to this comment is required for the test case above to work as expected.

Component:Rules integration» Flag core
Status:Needs work» Needs review

I think the problem is a core problem, not just related to the Rules integration component (see #67). I have also changed the status of this issue in order to notify the maintainers of my patches in #67.

Status:Needs review» Needs work

The last submitted patch, flag-fix_get_user_count_session-1619114-67.patch, failed testing.

@cspurk please try patch in #48 that already fix the problem, it does not?
Does you tested it separately?
I do not understand what is the test case, and on what it differs from the one I planned in #65, I mean depending on Rules module (and rule) or defining an hook, are these options really different?

The fact is that we can test with an available module (Rules) instead creating a new one (even if trivial), and test a real scenario, also the plan include a views and order on flag time.

Anyway we have still missing the test case, I wrote some code, first 3 step on plan (the simplest). Unfortunately, without a reproducible test case patches are not very appealing

> I would propose to entirely handle the -1 case in flag_set_sid() and to never show this state outside of this function. For the outside world, it could just look like the FALSE case for now (which appears to be handled throughout the Flag module already).

I like the sound of that.

Are both the patches in #67 part of the same fix? If so, they should be a single patch file rather than two.

+++ b/flag.module
@@ -2067,7 +2069,9 @@ function flag_set_sid($uid = NULL, $create = TRUE) {
+  return $sids[$uid] != -1 ? $sids[$uid] : 0;
}

Having a negative statement as a condition makes it harder to read -- could this be reversed?

I think it is a kind of .. déjà vu. Does really have a -1 as return value cause the problem? from the code I worked on this is not really a problem.

What is called a "flag module problem", in #67 was discussed in this issue and it is due to the optimization, say: do not set session on testing flag.

Scenario:

- user visit page yoursite.domain/apage

- apage content test if a flag is setted by the anonymous user

- she/he is anonymous, it is impossible, then return -1 and all goes right

- now user click on bookmark (or the flag defined)

- being anonymous but with session_api module, the behavior should be to set flag any way

- code executed on flagging content is:
1. test flag status
2. set flag to that content

The patch attached in #48 has the effect of force creation of session id just in this scenario.

In other cases the session is not setted, and this has good implication on performance for anonymous user who do not really need the session id (such as the one just looking around the site).

This would lead to 2 test cases I think:

Case A. no session id setted for anonymous when flag is not requested to be setted
Case B. no exception when it is requested

I write it better:

- code executed on flagging content is, during the single request (executiuon):
1. test flag status

2. set flag to that content

without the code in patch, module throws exception in the part of code that store in db session data, because session_id is negative.

stressing it again and again ...
code in #26 #1619114-26: After cleaning cookies, flagging content as an anonymous user causes a PDO exception. does set the flag instead of asking for the availability, but I revise this solution because of the following discussion (comments that follow)

The code in http://drupal.org/files/flag-fix_get_user_count_session-1619114-67.patch (second patch):

<?php
  
function get_user_count($uid, $sid = NULL) {
     if (!isset(
$sid)) {
-     
$sid = flag_get_sid($uid);
+     
$sid = flag_get_sid($uid, TRUE);
     }
     return
db_select('flag_content', 'fc')->fields('fc', array('fcid'))
       ->
condition('fid', $this->fid)->condition('uid', $uid)->condition('sid', $sid)
?>

has the mean of have a $sid not negative in a where clause in which code could not throw exception, because there is no violation of db constrain on checking a condition that could never be satisfied. So that second part is of no use. Right? or it has? maybe it make fail the test Case A in #1619114-72: After cleaning cookies, flagging content as an anonymous user causes a PDO exception.

Status:Needs work» Needs review
StatusFileSize
new913 bytes
PASSED: [[SimpleTest]]: [MySQL] 186 pass(es).
[ View ]

Okay, thanks all for your comments. I have further thought about the issue and done some more testing:

  • @smartango (#70): yes, the patch in #48 fixes the problem, too. I have just extended it in #67 because @joachim appeared to feel uncomfortable with the -1 SID to leak into the whole module (cf. #62).
  • My second patch from #67 is indeed not required. Apart from that I had created my patches for the wrong Flag version (7.x-2.x-dev), that’s why the automatic test failed in #69. Sorry for the noise.
  • Attached you can find a revised version of my (first and only useful) patch from #67 according to the suggestion from @joachim’s comment in #71 and now against Flag version 7.x-3.x-dev.
  • In reply to @smartango (#70):

    I do not understand what is the test case, and on what it differs from the one I planned in #65, I mean depending on Rules module (and rule) or defining an hook, are these options really different?

    The fact is that we can test with an available module (Rules) instead creating a new one (even if trivial), and test a real scenario, also the plan include a views and order on flag time.

    Well, that may be a question of taste. I find the relevant workflow of my test case easier to grasp than a workflow in which automatically firing rules and yet another (third-party) module are involved. The test case I have given in #67 also stems from a “real scenario” here.

  • In reply to @smartango (#72):

    What is called a "flag module problem", in #67 was discussed in this issue and it is due to the optimization, say: do not set session on testing flag.

    Yes, I fully agree. My new patch doesn’t change the overall situation; it just doesn’t introduce any new SID case (-1) in the Flag module. There is still no session created – a 0 SID means there is no session.

@cspurk
Fine. Thanks. I got your point, and maybe is more simple to include a test module in the tree than define rule and views module in the test framework, thus reducing dependencies.

That said, I think your suggested test case (#67) should be split just in the last step (3.):

3a: re-load the same content and check no cookie are setted
3b: click on set flag and check no exception

and add some visible state in case of seted flag: thus causing the check-the-flag-code to be executed.

Status:Needs review» Needs work

Re-reading this issue again months later, I think the -1 return code should really be a constant (like SESSION_API_INVALID_SID) which has the -1 value, or -769042 for that matter. That constant should be referenced in documentation/API usage examples etc. That will make life a lot easier for the Flag module & friends.

Maybe it's a better idea to file a patch against session API first, which makes some headway in this direction.

It works. but why it's not at release?

Status:Needs work» Needs review
StatusFileSize
new913 bytes
PASSED: [[SimpleTest]]: [MySQL] 254 pass(es).
[ View ]

I can confirm the patch in #75 works just fine. I think it is a good resolution for now. I have re-rolled the patch for the 7.x-3.x-dev version. Hope Joachim can commit this.

Status:Needs review» Reviewed & tested by the community

Working perfectly !

I also hope that this (#79) gets in asap !

Status:Reviewed & tested by the community» Needs work

Looking good so far! Thanks for the patch, and for the review.

Let's do a bit more clean-up, to make this more maintainable in the future:

+++ b/flag.module
+++ b/flag.module
@@ -2158,7 +2158,9 @@ function flag_set_sid($uid = NULL, $create = TRUE) {

Can we take this opportunity to document the params and return of this function?

+++ b/flag.module
@@ -2158,7 +2158,9 @@ function flag_set_sid($uid = NULL, $create = TRUE) {
+  // none was required (-1) but now it is required ($create == TRUE).
...
       $sids[$uid] = session_api_get_sid($create);

Does a -1 sid actually mean 'none was required'?

Since session_api_get_sid() appears to have incomplete docs (http://drupalcode.org/project/session_api.git/blob/refs/heads/7.x-1.x:/s...) I'd actually quite like it if we noted what the return values meant here.

Something like above the call to session_api_get_sid():

// This returns one of the following:
// - 0 if blah blah
// - A positive integer session ID if yadayada
// - -1 if something else.

(Obviously, SessionAPI needs fixing too... but I don't know how active that is.)

Given these are all documentation changes, I'm happy for a new patch to go straight back to RTBC :)

I have contacted the sessions api maintainer for further clarification on the return values, so that we can put some accurate documentation in this patch. But, as of now, still no news from them. It doesn't look like the module is very active at the moment.

Once I do get the necessary information, I will go ahead and update the patch and post.

I was thinking it's more a case of documenting what we're *assuming* the return values to mean, in the absence of decent docs in the module we're using.

(Need I say that having a key part of Flag's functionality depend on a module that is possibly unmaintained, and definitely badly documented, gives me the heebie-jeebies? I've filed #2043043: Missing documentation for session_api_get_sid().)

#48 fixed the problem for me too.

Would be nice to get this into 3.1, but as it's been around for ages, I'm not going to tag it as a blocker.

I just sent a message to the Session API maintainer offering to become a co-maintainer. Awaiting his reply.

I'm happy for this patch to get in with the changes in #81.

StatusFileSize
new1.12 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch invalid-session-1619114-88.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I'll spare you Joachim, but the function could be much simpler than it currently is. If it will always return 0 for registered users, why not make a conditional for that at the top of the function? There's also no need to keep all UIDs in the drupal_static(), keeping a single item with the SID of the anonymous user suffices.

Status:Needs work» Needs review

StatusFileSize
new2.7 KB
PASSED: [[SimpleTest]]: [MySQL] 266 pass(es).
[ View ]

wow, that's a first... :)

Yeah ! I can't wait to use the new version :) Thanks !

I'm now co maintainer of the session API module, so I plan to go over it in the near future.

Status:Needs review» Fixed
StatusFileSize
new2.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1619114.93.flag_.invalid-session-id.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

> but the function could be much simpler than it currently is. If it will always return 0 for registered users, why not make a conditional for that at the top of the function? There's also no need to keep all UIDs in the drupal_static(), keeping a single item with the SID of the anonymous user suffices.

I see what you mean. We could just put a trapdoor at the top:

if ($uid != 0) {
  return 0;
}

I'll certainly take a patch for that in a new issue. It's probably not a big performance problem, as I expect that over one request we only come here for the current user (unless we're sending out emails with flag links or something?). But it certainly would help readability.

In the meantime, I'm committing this patch and finally laying this issue to rest. Patch is #90 with the @param docs tweaked to say '(optional)'.

git commit -m "Issue #1619114 by cspurk, a.ross, smartango, Shabana Blackborder: Fixed invalid session IDs causing exceptions when anonymous user no longer has a cookie." --author="git <git@2451710.no-reply.drupal.org>"

Thanks to everyone who's helped on this -- it's certainly been a long one!
And congrats a.ross on your new co-maintainership!

Hell yeah ! Thanks !

Nice! I just noticed a small problem:

+++ b/flag.module
@@ -2284,18 +2304,28 @@ function flag_set_sid($uid = NULL, $create = TRUE) {
+ *   (optional) Determines whether a session should be created if it doesn't
+ *   exist yet. Defaults to TRUE.

The default for the get function is FALSE, not TRUE.

Thanks for checking the patch and spotting that one! Fixed it.

While we're on those faffy params with their flip-flop, here's a potential follow-up:

  return flag_set_sid($uid, $create);
// ...
function flag_set_sid($uid = NULL, $create = TRUE) {

There is only ONE call to flag_set_sid() in the whole module. $create is passed in. Hence on flag_set_sid(), there is no point in the param being optional. In fact, there is no point in either param being optional there.

That sounds a bit like what I was saying in #7. I think there may be a better solution though: use the function pair as intended throughout the module. There is one flag_get_sid($uid, TRUE) call in the module (includes/flag/flag_flag.inc:729), which can just be converted to flag_set_sid($uid).

If you do that, flag_get_sid() won't need the second parameter at all.

By the way, I filed this: #2072491: Simplify flag_set_sid()
We can take #96 and #97 over to that issue.

Happy to see this finally committed :-)!

Status:Fixed» Needs review

First thank you for this great work and patches but I have to reopen this issue, as I come to the same problem with a PDO exception not from falgging content by anonymous user but with a newsletter confirmation.

We use the simplenews and flag module to get the last flagged newsletter in rules and to send it out when a new user has subscribed. Everything worked perfect until the moment I cleared the site wide Drupal cache in my admin menu. Now new subscribers fall in the PDO exception when they are confirming there subscription through the browser. As we configured rules to send emails to admin when a new subscription has been done, we can see that rules can't get the flagged content anymore. The log messages shows "Unable to get a data value. Error: Unknown data property flag_sid.".
I installed the latest dev version on a development server, applied all submitted patches but the error still exists. On the life server we are using the 7.x-3.1 version.

This is the first time I am writing on drupal.org, so please be a little bit patient with me.
I really appreciate all advises or help here, so thank you in advanced.

Status:Needs review» Needs work

The last submitted patch, 1619114.93.flag_.invalid-session-id.patch, failed testing.

Status:Needs work» Fixed

Please open a new issue for this -- though I have to say, this sounds like a case of doing things with flag that flag is not meant to do.

Status:Fixed» Closed (fixed)

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