Alright this time, this one wasn't so obvious... however, it's obvious when you look at the code. The function flag_rules_action_fetch_entity_by_user only searches for the user, the entity type, and the fid.

In reality, those columns alone do not make for a unique record. For anonymous users we must take into account the user's sid. This causes for the result that is returned here to be a list of entity ids that were retrieved by multiple anonymous users the way it is now. So when trying to retrieve the last entity of a specific type a user has flagged, you have issues.

Additionally, there is no way to gather the sid for a user via rules currently because we can't always assume that the user being entered is the user that is currently using the site.

I propose that we add a required SID field for these actions for when the user is anonymous that defaults to 0. This way the rule will be backwards compatible for all regular users as they are now (since they don't have SIDs).

The next problem that I don't have a solution for is retrieval of a SID for a user object that is not the current user. I'm not sure it's even possible because other modules do not store the user with the flags SID.

As for the current user, I can throw in some code that adds a token that lets you retrieve it on the site:current-user: object.

Files: 
CommentFileSizeAuthor
#8 flag-get_sid_for_anonymous_user_when_fetch_flagged_entities_by_user-2054927-8.patch3.07 KBgeneralredneck
PASSED: [[SimpleTest]]: [MySQL] 266 pass(es).
[ View ]
#4 flag-get_sid_for_anonymous_user_when_fetch_flagged_entities_by_user-2054927-4.patch2.37 KBgeneralredneck
PASSED: [[SimpleTest]]: [MySQL] 266 pass(es).
[ View ]

Comments

I propose that we add a required SID field for these actions for when the user is anonymous that defaults to 0. This way the rule will be backwards compatible for all regular users as they are now (since they don't have SIDs).

Yes, I agree that is the best approach so that it is backward compatible as well.

The next problem that I don't have a solution for is retrieval of a SID for a user object that is not the current user. I'm not sure it's even possible because other modules do not store the user with the flags SID.

Is this really a relevant use-case? Do we really need to worry about this?

As for the current user, I can throw in some code that adds a token that lets you retrieve it on the site:current-user: object.

Will you be working on the patch for this?

Yeah.... I already started it on Wednesday, but due to the nature of my availability I won't be able to start work on it again till Monday... at that point you can see what I did... I was planning on adding to the hook_entity_property_info_alter() hook and adding the SID as info to the user when it's available. This should make it available to rules, and then in the flag_rules_action_fetch_entity_by_user function we just check the SID property and if it is there... we use it in the search and if it's not... we probably should just ommit the condition instead of hard coding it to 0. This would preserve current functionality... and wouldn't be limiting the data... what do you think? Because techincally, as it stands right now, the function returns a "list" and you can iterate though all of the anonymous entries if you want... Limiting it to 0 would in fact let you go through none of the anonymous entries... (my bad)

Thoughts?

The next problem that I don't have a solution for is retrieval of a SID for a user object that is not the current user. I'm not sure it's even possible because other modules do not store the user with the flags SID.

Is this really a relevant use-case? Do we really need to worry about this?

The only time I would imagine something like this happening is in an administrative type situation where someone might want to perform some action for specific anonymous users. For instance, if someone flags an item, then there is a rule to unflag another user's flags. But this is a VERY limited usecase and doesn't fit in the 80-20 rule... maybe not even the 90-10 rule. (features people use most - features for specific usecases). In any case... if someone needs it... it's probably best to let them figure it out :P

we use it in the search and if it's not... we probably should just ommit the condition instead of hard coding it to 0.

Yeah, I think that is the best call. We shouldn't do anything to alter the current functionality as obviously, that could lead to unexpected results for a lot of people already implementing the rule. We should basically default to the query without the SID (this would give us a list of the anonymous entries as well, just as it was doing before) and add this extra 'condition' to the query only if the SID property exists.

Status:Active» Needs review
StatusFileSize
new2.37 KB
PASSED: [[SimpleTest]]: [MySQL] 266 pass(es).
[ View ]

Yay... patchy patch patch

Sorry it took so long. It took me some screwing around with the entity api to get the feel of altering existing entities and their properties.

Thanks. Looks good, will test it out...

+++ b/flag.module
@@ -2372,3 +2372,11 @@ function flag_properties_get_flagging_users($entity, array $options, $name, $ent
+ * Getter callback that returns the SID of the user that is being retrieved.

Let's assume that when this gets documented (#1060986: Add API docs for all property info callbacks.) in Entity API it'll be called callback_entity_property_getter() -- and thus split this into two lines, the first being the (new!!!) standard form for documenting a callback. (See documentation standard docs at https://drupal.org/node/1354 for the details.)

@generalredneck: I have tested this now and there is a small mistake in the flag_rules_action_fetch_entity_by_user function. Check the comment for the uid condition below:

+function flag_rules_action_fetch_entity_by_user($flag, $entity) {
+  $user = entity_metadata_wrapper('user', $entity);
+  $sid = $user->flag_sid->value();
+  $query = db_select('flagging', 'fc')
     ->fields('fc', array('entity_id'))
     ->condition('entity_type', $flag->entity_type)
     ->condition('uid', $user->uid) //Should be ->condition('uid', $user->uid->value()) as we are using a wrapper
-    ->condition('fid', $flag->fid)
-    ->execute();

Hope you can add the documentation in #6 and add this change and re-post the patch.

StatusFileSize
new3.07 KB
PASSED: [[SimpleTest]]: [MySQL] 266 pass(es).
[ View ]

This look right?

Status:Needs review» Fixed

Committed to revision 7.x-3.x-dev.

Status:Fixed» Closed (fixed)

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