Even though the documentation for flag_trim_flag() function suggests that the trimming will only take place for the passed $account, it actually trims all flaggings for the current user AND all anonymous flaggings. Makes for some strange behavior.

Basically, it just needs to take into account the session ID, and that would solve it. Patch coming shortly.

CommentFileSizeAuthor
#7 anonymous-user-trimming-1439168-9.patch1.19 KBjoelstein
#3 trimming-improvements-1439168-3.patch1.12 KBjoelstein
#2 rules-trim-with-anonymous-1439168-2.patch714 bytesjoelstein
#1 trim-with-sid-1439168-1.patch438 bytesjoelstein

Comments

joelstein’s picture

joelstein commented
Status: Active » Needs review
StatusFileSize
new trim-with-sid-1439168-1.patch438 bytes

Here's the one-line patch to solve this bug. Thanks!

joelstein’s picture

joelstein commented
Title: Trimming a flag shouldn't be a semi-global action » Trimming a flag, via Rules, in response to flagging a node, for anonymous users, is broken
StatusFileSize
new rules-trim-with-anonymous-1439168-2.patch714 bytes

Upon further investigation, it seems that if in a certain situation the Rules "trim a flag" action does not receive the $flagging_user, but instead receives a FALSE value. Here's how to reproduce this.

1) Create a vanilla installation of Drupal, Flag, Session API & Rules.
2) Create an flag which is flaggable by anonymous users.
3) Setup a Rule to respond to the event "A node has been flagged, under ___" (the name of your flag).
4) In that rule, add a "Trim a flag" action, and trim to 1 for the flagging user. (This is supposed to keep it so only one of this flag per user is ever available.)

Now, go flag something as an anonymous user. You'll get an error, "Notice: Trying to get property of non-object in flag_trim_flag() (line 1015 of sites/all/modules/flag/flag.module)." If you use Devel to debug the $flagging_user variable in flag_rules_action_trim(), you'll see that it's not the expected user object, but a FALSE value.

Note that this doesn't happen for authenticated users, or if you trim a flag in response to other types of Rules events.

I'm not sure why this is happening, but it seems that somewhere in the Rules execution flow, the anonymous $flagging_user object is lost.

Solutions include figuring out why that user is lost, or simply checking if the $flagging_user is FALSE, and then setting it to the current anonymous user, since we know that it only happens for anonymous users.

The attached patch proposes the second solution, since I don't know how to go about troubleshooting the Rules execution flow.

joelstein’s picture

joelstein commented
Title: Trimming a flag, via Rules, in response to flagging a node, for anonymous users, is broken » Trimming a flag improvements
StatusFileSize
new trimming-improvements-1439168-3.patch1.12 KB

Well, it seems that both of these patches are needed to make trimming a flag possible for both anonymous and authenticated users, in such a way that trimming a flag for one user doesn't trim it for multiple users (as is certainly the case when trimming for an anonymous user, especially).

In summary, this patch does two things:

1) It accounts for the session ID (sid) of the user when trimming a flag.
2) It makes sure that there's a user object when trimming a flag via Rules, for an anonymous user.

mattcasey’s picture

mattcasey commented

There's a similar issue posted for 6.x: http://drupal.org/node/1134384.

Also, for what it's worth, I have the same setup in #2 using 6.x-beta2, but I don't get those errors.

joachim’s picture

joachim commented
Status: Needs review » Needs work

Thanks for the patch.

Here's a quick eyeball review:

+++ b/flag.module
@@ -1013,6 +1013,7 @@ function flag_trim_flag($flag, $account, $cutoff_size) {
+    ->condition('sid', flag_get_sid($account->uid))

I'd like some code comments to explain what's being done here please.

+++ b/flag.rules.inc
@@ -277,6 +277,12 @@ function flag_rules_action_unflag($flag, $entity, $flagging_user, $permissions_c
+  // For some reason, when this action fires in response to a flagging event,
+  // as an anonymous user, then the $flagging_user is sent through as FALSE.
+  // Not sure why. This workaround fixes the problem in this specific case.
+  if ($flagging_user === FALSE) {

Has anyone had any luck in tracking the cause for this down in Rules?

joachim’s picture

joachim commented
Version: 7.x-2.x-dev » 7.x-3.x-dev
joelstein’s picture

joelstein commented
Status: Needs work » Needs review
StatusFileSize
new anonymous-user-trimming-1439168-9.patch1.19 KB

This has been updated for 7.x-3.x-dev, and a comment was added to help clarify why we need a condition for the SID.

joachim’s picture

joachim commented

Tests pass, but that is no indication -- we have no test coverage of Rules at all.

Could someone try this out on their Rules set up and report back?

I'll commit this when it's set to RTBC :)

nlisgo’s picture

nlisgo commented

I've implemented patch #7 manually on alpha4 and it works in rules for me.

joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

I'll take that as a RTBC. Thanks! :)

joachim’s picture

joachim commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

git commit -m "Issue #1439168 by joelstein: Fixed flag trim removing flaggings for the anonymous user." --author="joelstein "

Status: Fixed » Closed (fixed)

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