Flag starts a session on anonymous user loads which is bad for pressflow/varnish

subscribing

tracking
The patch works on beta-5 and master branch.

I will try and see if everything is ok...

Edit marked #1095910: Flat 6.x-2.x starts a session on anonymous user loads which is bad for pressflow/varnish as a duplicate no need for one issue for each version.

+++ b/flag.module
@@ -1726,8 +1726,9 @@ function flag_get_user_flags($content_type, $content_id = NULL, $uid = NULL, $si
+  $sid = !isset($sid) ? flag_get_sid($uid, FALSE) : $sid;
+// set no data for anon user with no data
+if ($sid != NULL && $sid >= 0) {
   if (isset($content_id)) {
     if (!isset($flagged_content[$uid][$sid][$content_type][$content_id])) {
       $flag_names = _flag_get_flag_names();
@@ -1767,10 +1768,21 @@ function flag_get_user_flags($content_type, $content_id = NULL, $uid = NULL, $si

@@ -1767,10 +1768,21 @@ function flag_get_user_flags($content_type, $content_id = NULL, $uid = NULL, $si
         $flagged_content[$uid][$sid][$content_type]['all'][$flag_names[$flag_content->fid]][$flag_content->content_id] = $flag_content;

This part become hard to follow, why not just teste if we are anonyme user and returne

 // set no data for anon user with no data
  if ($sid == NULL && $sid == 0 && isset($content_id)) {
    return $flagged_content[$uid][$sid][$content_type][$content_id];
  }
  if (isset($content_id)) {
   ...

Same patch but for D6

Thanks for these patches gagarine and trrroy! I'm trying to figure out why I had put that flag_set_sid() in flag_init(), because I can't figure out any negative consequence of removing it (or commenting it out as you did in your patches).

I've revised these patches so that they're a little easier to follow and require fewer changes. I'm guessing that you set the default $create parameter to TRUE because you wanted to match the session_api_get_sid() function signature, but setting the $create parameter to default to FALSE in Flag module save us a lot of code changes. The default parameter should be the most common one, so I've flipped the default $create to be FALSE instead. The resulting patch is much smaller because we have fewer changes to make.

I also changed the flag_get_user_flags() approach, instead of wrapping the entire chunk of code in another IF statement, I simply added a 3-line IF statement near the top of the function that bails out if the user doesn't have a session.

Overall, this patch should act identical to yours, I've just changed the way the code is written to make it easier for maintenance in the future (for me at least). Could you review this approach and confirm it still works for your setup? I've tested that the session isn't started unnecessarily as it was previously and everything seems to work well in both D7 and Pressflow.

Subscribing

subscribe

The D7 patch in #7 didn't work for me against the beta5 or newer versions.

Could you clarify, "didn't work"? What's the behavior you're expecting and what is Flag module doing incorrectly after applying the patch?

I've confirmed that the patches apply directly with no fuzz or failed hunks against the 7.x-2.x branch in git. Note that I recently branched off from "master" in Git, you may need to pull down the 7.x-2.x branch (or just do a fresh checkout). I didn't remember to update the dev release to point at the new branch though, but after tonight's repackaging it should be up-to-date also.

Super. Committed to both 2.x branches. Will be in the beta7 release.

I just tested the 6.x-dev version and still had problems with varnish and anonymous users.
- Using session_api 6.x-1.4, flags are not set. Anonymous flagging doesn't work at all.
- Using session_api 6.x-1.2, flags are set, but all pages get a cookie and "no-cache, must revalidate", so turns off varnish on all pages

this seems to be a thread on the same problem: http://drupal.org/node/1058960

Sorry to be the buzzkiller once again, but the changes made with this patch break all global flags. The flagged status is no longer reported correctly, and thus no unflag links are being displayed.

The problem is this piece of code added to flag_get_user_flags():

  // If the user is anonymous and doesn't have a session, return an empty list.
  if ($uid == 0 && empty($sid)) {
    return array();
  }

get_flagging_record() calls this function in the context of is_flagged().
For global flags it always passes $uid = 0.

Doh, right again ralf.strobel! No worries, this is exactly why we made the beta6 release before committing all these architectural changes.

To fix this problem, we could either check if the flag is global, or we could check the global $user->uid.

Hm, actually on further thought, perhaps the best way to fix this (at least in an easy to understand way) is simply to remove the conditional altogether. I can't think of any negative consequences this would have, and it should only retrieve global flags and nothing else, since there can't (or shouldn't) ever be a situation where an anonymous user has flagged something without an SID.

How about it?

I can't find any problems with the above approach. It will unnecessarily do a query for non-global flags for anonymous users, but overall this is negligible since the page is going to be cached anyway.

I've committed the patch in #21. Let's open new issues for any further bugs. I don't like having multiple commits attributed to the same issue number.