Using the "Flag Fetch $type By User" rule returns the wrong result when grabbing for Anonymous users. [#2054927]

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.

Comment	File	Size	Author
#8	flag-get_sid_for_anonymous_user_when_fetch_flagged_entities_by_user-2054927-8.patch	3.07 KB	generalredneck
#8
#4	flag-get_sid_for_anonymous_user_when_fetch_flagged_entities_by_user-2054927-4.patch	2.37 KB	generalredneck
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

shabana.navas CreditAttribution: shabana.navas commented 1 August 2013 at 06:24

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?

Comment #2

generalredneck

him/his

English

Texas, USA 🇺🇸

CreditAttribution: generalredneck commented 2 August 2013 at 14:17

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

Comment #3

shabana.navas CreditAttribution: shabana.navas commented 6 August 2013 at 06:23

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.

Comment #4

generalredneck

him/his

English

Texas, USA 🇺🇸

CreditAttribution: generalredneck commented 6 August 2013 at 14:49

Status:

Active

» Needs review

File	Size
flag-get_sid_for_anonymous_user_when_fetch_flagged_entities_by_user-2054927-4.patch	2.37 KB

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.

Comment #5

shabana.navas CreditAttribution: shabana.navas commented 7 August 2013 at 09:20

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

Comment #6

joachim CreditAttribution: joachim commented 7 August 2013 at 21:08

+++ 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: [meta] 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.)

Comment #7

shabana.navas CreditAttribution: shabana.navas commented 14 August 2013 at 09:31

@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.