Record uid for global flags

> If so, should it be an option on global flags or just the default behavior.

I'd say just the normal behaviour. In the bigger picture it makes sense to know who created an entity. And let's not burden ourselves with an option that won't mean anything to most users.

> 2. Store the uid and return it when loading global flags

That seems like the best option to me. The alternative seems fudgy (and also, if we're always fudging the uid to 0, how would you actually find out what the uid on a Flagging is?)

> I don't know if anyone relies on global flags having uid of 0 on them or not which is why I'm asking before posting a patch, but I'm happy to work on this.

I'd need to know more about which parts of Flag rely on that, and the parts of the API which other modules might be relying on.

But my general feeling is that if you are checking to see if a flagging is global, then you should be checking the flag configuration. Checking the flagging is asking the monkey rather than the organ grinder ;) If you like, file a preparatory issue that cleans that up first -- if that'll make it easier to review.

Another thing to note is that a quick round-trip of the Flag ecosystem modules (https://drupal.org/node/1748160) shows that only one of them (https://drupal.org/project/og_flag) is supporting 3.x -- the others are loudly warning that they do not!

So tentatively, I'm okay with this change at this point of the release cycle.

I think on 7.x-2.x we'd be breaking a lot more things!

Also, 7.x-2.x is no longer supported. It'll get bugfixes if they're important, and other bugfixes if backport patches get submitted. I really don't want to be maintaining 4 branches, which is what we nearly have now that 8.x is on my radar.

I'd very much like it if people upgrade to 3.x, but at least if they remain on 2.x we should leave them in peace and not break their stuff :)

I've had a brief search through the module code for things using '->uid' and I can't find anything that uses the 0 on the flagging to determine whether the flag is global.

Could you let me know which bits you found?

Two API functions that are going to be affected by this change are flag_get_entity_flags() and flag_get_user_flags().

- people may be using flag_get_user_flags() to get all global flags for one or all entities by passing in a $uid of 0.
- people may be using flag_get_entity_flags() to get all flags for a single entity, and be expecting to see a [0] array key for global flags.

Now I'm not sure why people would be doing those things, as those functions seem to me to be geared towards working with non-global flags... but the longer we leave it the more it'll feel like a change we shouldn't be making.

I've been looking at this one and it looks like $flag->get_flagging(...) calls $flag->get_flagging_record(...) which in turns calls the flag_get_user_flags(...). Am I right in thinking that flag_get_user_flags(...) will need to allow for this? or is there an alternative mechanism for picking up a global flag?

If we are reworking the flag_get_user_flags(...) function, we could rework it so a NULL $uid means that we are not restricting it on uid. We could then conditionally add the uid condition ONLY if $uid passed in is NOT NULL.

Would this be a reasonable solution?

> we could rework it so a NULL $uid means that we are not restricting it on uid.

A NULL uid already means 'use the current user'. So we can't change that.

Currently, flag_get_user_flags() will only return flagging data that is for the given user, for non-global flags.

I think we can either:

* keep doing that
* add an optional parameter that allows ALSO returning flagging data for the global flags the given user has made. Not that I'm keen on adding yet another parameter to an already long function definition.

Would it make more sense to just implement the global flag lookup in the flag->get_flagging_record(...) method itself if its a global flag?

That way, there are no api changes...

Uploaded patch. Modified queries to check global status based on the actual flags table, not on the uid field. Removed the uid 'fudge' so they save properly on global flags, etc, etc.

Here are my test results:
1. rules: we are now able to set the uid for global flags; removed warning from description.
2. views: works as normal, except that adding a global flag with a relationship "included only flagged content" "by this user" has no effect.
3. flagging_form: works as normal with auth'd or anon users; all field values set as normal
4. session_id: anonymous flagging works as normal; all users are able to set/unset the global flag if they have access

Uploaded another patch which fixes the Views issue.

New patch includes everything from #2, plus allows us to arbitrarily switch a flag to be global or local.

When going from global to local, everything worked out of the box.

When going from local to global, there was a chance that multiple users had flagged the content before the flag was made global. This would cause them to have to click the unflag link multiple times for it to take effect. _is_flagged() now returns an array, and $flag->flag() now loops through it to unflag every potential flagging on a global flag. Works as expected now.

Flag counts could be >1 for global flags now, in this specific scenario.

Combined the flag_get_user_flags() static queries into one dynamic query.

> When going from local to global, there was a chance that multiple users had flagged the content before the flag was made global. This would cause them to have to click the unflag link multiple times for it to take effect. _is_flagged() now returns an array, and $flag->flag() now loops through it to unflag every potential flagging on a global flag. Works as expected now.

That's why we don't allow flags to be changed from global to non-global.

I don't think this is a desirable change, sorry.

Understandable; patch #2 is the one we want.

Conversion from global to non-global is not really related to this issue -- and either with this new feature or without it, it's problematic:

* global to non-global: with this patch, it's easy. But without it, how do you pick which user to assign the former global flag to?
* non-global to global: without this patch, it's easy. But with it, which user to you let keep the flagging that's now global?

I consider it to be an interesting but somewhat academic problem -- filed under 'you built your site wrong' ;)

After patch #2 or #3, there is no difference from the way global and non-global flags are saved in the database, so changing a flag from global to local really just determines the front-end behavior: non-global flags are flaggable once by each person, and global flags are flaggable once by anyone--the magic is done in _is_flagged().

The answer to bullet point 2 is "all of them": the multiple flaggings still exist in the database, even for the now-global flag. This is what allows you to go back and forth without losing data. Our existing API functions seem to handle this just fine. The main point of weirdness is that a flag count for a global flag converted in this way could be greater than 1, which is normally not expected for a global flag. So I could see where you would want to lock this out to encourage site builders not to do this.

On the other hand, it's a useful feature for site builders whose needs have changed over time (not necessarily that they've done anything "wrong"... they just can't predict the future), and with this patch, there are no technical barriers to prevent this from working.

Patch #3 has the uid fix and allows the global/non-global switch, whereas patch #2 only has the uid fix. Final call is up to you; I can see where both viewpoints are valid.

I've been using patch #2 on a production site where we see 6000 flaggings per month; I haven't been able to find any issues.

If anyone else has tested this patch, please comment so that we can mark RTBC.

Re-rolled for version 3.5 via git. Working for me with Rules and Views.

RTBCing since I've been using this for quite a while with no issues.

Hmm, I guess I shouldn't set my own patch to RTBC. Please test this out an give it thumbs up or thumbs down!

Here is what this patch will allow you to do:

1. You can build views that show you who flagged what, even if the flag is global. For example, I'm using it for people to flag a node as 'Assigned', and then you can have a Views block next to it that shows you different information about it. If somebody else views that node, they can see exactly who assigned it, when they assigned it, field values that they selected when they assigned it, etc.

2. You can use Rules to work with global flags, and it knows which user did the flagging. So, for example, you could have a global flag trigger an email to the user that flagged it.

3. The user that flagged a global flag is now stored in the database, so that you can do something with it later. Previously, all this data was just set to 0.

All in all, it's been working really well for me, and I think that this could help out a lot of site builders.

Fixed two oversights: in _is_flagged, we should only check the uid if the flag is not global. Removed the $flag->flag() global flag check for anonymous users, since uids are being recorded now.

Using this on multisite environment, where I needed to find out which user flagged a node for deletion (custom flag), and notify the user using rules. On debugging I found out that uid was not being saved in the flagging for global flags. This patch works good :)
Being bold and marking RTBC, since this is sitting idle for many days.

A few things need clarifying or tweaking:

1. +++ b/flag.rules.inc
   @@ -432,17 +432,15 @@ function flag_rules_action_fetch_entity_by_user($flag, $entity) {
   -  // For global flags the user parameter is ignored, so we add the
   -  // extra 'uid' condition when the flag is NOT global.
   

   Why should we not continue ignoring this? What does this function do?

2. +++ b/includes/flag/flag_flag.inc
   @@ -727,19 +727,14 @@ class flag_flag {
        // Find out which user id to use.
   

   This comment is now obsolete.

3. +++ b/includes/flag/flag_flag.inc
   @@ -1039,14 +1034,13 @@ class flag_flag {
   +    if (!$this->global) $query->condition('uid', $uid);
   

   'if' blocks should always have braces.

   Also, what's being changed here is for non-global flags, which have not changed their behaviour. What is this change doing?

Why should we not continue ignoring this? What does this function do?

This change essentially reverts the patch from issue 2238349; since we now have uids associated with global flags, we have no reason to ignore them. This function is used in the "Fetch _____ flagged by user" Rules actions to return a list of entities that have been flagged by a user.

Also, what's being changed here is for non-global flags

The purpose of this change is to ignore the uid check for global flags, since if a flag is global, and you want to know if it's flagged or not, it doesn't matter who actually flagged it. The query for local flags has the uid check intact. Without this change, $flag->flag() won't be aware of an existing global flagging made by another user.

Other things in this patch:

• Patch 25 missed get_flagging_record(); that has been corrected
• Removed the obsolete comment and added braces for the 'if' statement

If anyone can help out testing/reviewing this patch, please comment.

This is working well for me with a custom flag and views. Thanks for posting it!

This also works for me on 7.x-3.5. I came across the same problem - I wanted to know who'd set the global flag. Thanks for creating this patch you've been really helpful! - Ian

I was just thinking about this issue recently...

I hate to do this, but we're going to have to move this to D8, and furthermore, I don't think it can ever be committed to d7. I think changing the data in this way mid-cycle would affect other contrib modules and custom code, so it's not the sort of change we can make.

Hi joachim

I'm going to continue to use this patch since I doubt I'd take my site through to D8. I've created fallback code for if the patch is not applied in case I need to revert the patch or upgrade.

The logic behind a global flag not recording the uid of who flagged it is limiting design decision imo.

Hope this feedback is helpful.
- Ian

I'm using this in production to prevent multiple people from working the same item in the global "work" queue.

Seeing as Drupal 8 isnt released yet, and Drupal 7 still has a good 4 years of life left, I would like to see this make it into a 7.x release.

Could we turn global flag user tracking into an option that is set on the flag configuration? If so, it wouldnt be hard to wrap the functionality in some if/then statements that way it wouldn't break existing implementations.

This would be great in Drupal 7!

Thanks for the patch. Rally helped me.

I applied this patch and I can see that the uid is associated with the global flag. Thanks.
How do I get Rules to work with global flags? I am trying to create a rule component, that I can use from Views Bulk Operations, so that if a node in the View was flagged by the current user, only then must the Rule execute on that node via VBO operation. As of now the Rule executes for all nodes. I want to put in some condition or some thing in the Rule, so it checks if the node's global flag's uid is the same as current user and only if so, executes action on that node. Is this possible?

@annie2512: Yes, the uid recording with this patch is usable as normal within Rules. However, actually using it within your specific case is another matter.

I vaguely remember doing something like this with a "content is flagged" condition, which made a "flagging" entity available, and then I used the rules_conditional module to make decisions based off the flagging uid value.

I would recommend you ask on drupal.stackexchange.com, since we want to keep discussion here strictly related to this patch. Best of luck!

Re-roll for 3.7 dev

Hey micromegas, your patch (from #41) was extremely helpful for my project. I hope this gets rolled into the module. Thanks!

Wait...don't we already store the UID for global flags in 8.x?

From FlagService::flag()...

$flagging = $this->entityManager->getStorage('flagging')->create([
      'uid' => $account->id(),
      'flag_id' => $flag->id(),
      'entity_id' => $entity->id(),
      'entity_type' => $entity->getEntityTypeId(),
      'global' => $flag->isGlobal(),
    ]);

The patch is for the Drupal 7 version; Drupal 8 might not need the patch at all.

Since this is already the case in 8.x, I'll move this back to 7.x and @joachim can decide if this is a won't-fix.

See my comment 34. This could break lots of people's code, so it's not a change we can make mid-cycle.

Could this maybe be done as a contrib module?

The UID being set to zero was a relic from views_bookmark way back in the Drupal 5 days. That module didn't have the flag API like we do today, so the only way you could tell if a flagging was global was if the UID and SID were zero.

People have an expectation that UID data is not destroyed when a flagging is saved, and I think that's a reasonable expectation.

Is there some way that we could put this into an upcoming branch, or new release?

Re-roll for 3.8.

For a simple work around the flag limiter module https://www.drupal.org/project/flag_limiter can make a non-global flags behave like a global flag by limiting the number of flaggings to 1 across all users.