Summary

Patch for Flag CTools integration to allow displaying content that has been flagged by anyuser rather than just the current user allowing for better Panels visibility rules. The patch has a settings form and has been coded so that if nothing is set, it defaults to the existing behavior (that is, true if flagged by current user). So it adds an option, but should not in any way effect existing installs and should be safe to apply to any existing site.

Problem

Can only create visibility rule for content flagged by current user. Want to display content flagged by any user.

The CTools integration added by #332956: Flag Ctools integration creates a visibility rule Node is flagged (if the flagged entity is of type node of course)). Assuming the entity is a node and the flag being tested for is "Likes", the summary is Node being viewed is flagged with "Likes". Really, though, it should be Node being viewed is flagged with "Likes" by the current user.

In point of fact, though, it is actually using the is_flagged() method, which tests for things that are flagged by the current user. This means that you can't create, for example, a Panels visibility rule that would show content if it has been flagged, but only if it has been flagged by the current user.

So

  • The description of how this works is imprecise
  • And sometimes you want to check for content flagged by anyone at all (like for displaying a flag count).

Solution

It's fairly straightforward to modify the CTools plugin and make it so that the Panels settings screen lets the user choose between flagged by anybody and flagged by current user and add the appropriate description to the summary. Then for the actual access check, depending on the setting us is_flagged() or get_count().

Usage

Install the patch as you would normally do. Patch is based on current git pull on the day of this post, but should apply cleanly to 7.x-3.2 I think.

Once installed, for Panels where Flag is working correctly, do nothing. However, if you want to change the behavior, you now have an options form in the visibility rules. You can now select whether you want to show items flagged by the current user (default behavior as with official release) or content that is flagged by anyone at all.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ergophobe’s picture

This patch does not change existing behavior at all. It always checks that the new $conf array member is set before trying to use it (perhaps obsessively so) and it defaults to the original behavior, so it shouldn't create any problems with existing installs.

And it adds the new option and adds more precise descriptions for the summary.

ergophobe’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1: flag-panels-access-check-2151157-1.patch, failed testing.

ergophobe’s picture

Adding one more check - making sure there is a default for the default - that is, if a user doesn't have a value for the new setting, make extra sure it defaults to 'user' which is the same behavior as existed before this patch.

Also patch was relative to site root, not module root. Hope that's why it failed.

ergophobe’s picture

Status: Needs work » Needs review

Moving back to needs review to trigger patch test

ergophobe’s picture

Title: CTools access check only for current user » Allow check for flagged by current user AND flagged by any user (patch)
Issue summary: View changes

Updated issue summary and title.

joachim’s picture

Status: Needs review » Needs work

Thanks for the patch!

Looks like the right approach generally, just a few things to tidy up:

  1. +++ b/plugins/access/flag_is_flagged/flag_is_flagged.inc
    @@ -46,23 +46,36 @@ function flag_flag_is_flagged_access_get_children($plugin, $parent) {
    +    '#title' => t('Flagged by user?'),
    

    Could this be changed to something like 'Flagging owner'. I don't think question marks in form element labels read very well; they're not used in Drupal core for instance.

  2. +++ b/plugins/access/flag_is_flagged/flag_is_flagged.inc
    @@ -88,9 +106,18 @@ function flag_flag_is_flagged_access_summary($conf, $context) {
    +    $flag_string = t('@identifier is flagged with "@flag"', array('@flag' => check_plain($flag->title), '@identifier' => $context->identifier));
    +  ¶
    +    if (isset($conf['flag_user']) && $conf['flag_user'] == 'user') {
    

    Stray whitespace here.

  3. +++ b/plugins/access/flag_is_flagged/flag_is_flagged.inc
    @@ -88,9 +106,18 @@ function flag_flag_is_flagged_access_summary($conf, $context) {
    +      $flag_string .= t(' by anyone');
    

    Don't break sentences across calls to t(). That won't work for translation -- some languages will need to build the sentence in a totally different order. You'll need two separate strings here.

    (While you're rewriting that, note there is an existing bug here with the redundant use of check_plain() on a '@' placeholder. It should just be a @, which does the check_plain() for us.)

  4. +++ b/plugins/access/flag_is_flagged/flag_is_flagged.inc
    @@ -78,7 +91,12 @@ function flag_flag_is_flagged_access_check($conf, $context) {
    +    return (boolean) $flag->get_count($id);
    +  }
    +  else {
    +    return $flag->is_flagged($id);
    

    Could we have some comments here to explain the logic?

    I presume you're using get_count() because all you care about is whether anyone has flagged it, but it the comments explain that it saves having to figure that out all over again in future.

    Also, I wouldn't use something that uses the flag_counts table for this. That's meant for reporting only, and it has some issues with how it keeps up to date with changes.

    Maybe something like flag_get_entity_flag_counts() instead.

Also, I've actually no idea where this actually gets used. I tried creating a panel page to see if it showed up anywhere, and I can't find it. Could someone maybe point me in the right direction?

ergophobe’s picture

Awesome! Thanks for the feedback.

Sorry about numbers 1-3. I actually thought of some of that as I was going to bed after posting and realized I should fix it. This is actually the first time I've installed Flag, so I don't even fully know everything it does, let alone all the internals. Although in following up on your comments, I've gotten to know the code a lot better!

something like flag_get_entity_flag_counts() instead

Hmm.... that is unfortunately named. It is really "flag_get_entity_type_flag_counts()" because, in spite of the comment, it does not get the count of flags on a given entity, but on a given entity type.

I can see that the function is only called once for the Rules action (in flag/flag_rules.inc, line 460):

function flag_rules_action_fetch_entity_flag_count($flag, $entity_type) {
  $count = flag_get_entity_flag_counts($flag, $entity_type);
  return array('entity_flag_count' => $count);
}

I want to keep each patch as small as possible, but it makes me wonder

  • Should that function be renamed to a name that indicates it deals with entity type?
  • Is that really the desired result for Rules anyway? I'm just trying to think of when I would actually care what the total number of flags applied to a given entity type is? I guess I might care when deleting an entity type, but maybe there's a use case that I'm not seeing.

Regarding caching and all that....

the comments explain that it saves having to figure that out all over again in future

The comments for the function itself explain that it's coming from a cache that allows you to get counts for all types of flags on a given entity in a single query. But it doesn't say anything about it being unreliable.

But again, that's a separate issue.

Let me try to get my patch acceptable, and then if you want to address the other issues, I'd be happy to help with that, but I'd rather keep them separate.

I'll try to have an updated patch ASAP

ergophobe’s picture

Status: Needs work » Needs review
FileSize
4.49 KB

Okay, new patch as per your comments.

Because of the naming issue mentioned above with flag_get_entity_flag_counts(), my almost identical function is flag_get_entity_flag_counts_by_id().

Also, since I guess I'm not explaining very well what this actually does, I made a 1-minute Jing video that shows it. I won't be submitting this at Cannes, but I think it should make the point of this obvious: CTools Integration patch demo video

joachim’s picture

Status: Needs review » Needs work

> Hmm.... that is unfortunately named. It is really "flag_get_entity_type_flag_counts()" because, in spite of the comment, it does not get the count of flags on a given entity, but on a given entity type.

Not only that, but its documentation is wrong:

* Get the count of flags for a certain entity.

I'll file another issue for that.

> Should that function be renamed to a name that indicates it deals with entity type?

Probably not until 8.x, as it's part of our public API.

> Is that really the desired result for Rules anyway? I'm just trying to think of when I would actually care what the total number of flags applied to a given entity type is? I guess I might care when deleting an entity type, but maybe there's a use case that I'm not seeing.

I don't know much about Rules, but if you think there's a problem there, do file a new issue for it!

> But it doesn't say anything about it being unreliable.

Not so much unreliable as buggy ;)
The problem is with the various parts of Flag where you can delete flaggings in bulk: these tend to not invoke the unflagging API for performance reasons. For example if you delete a user, then I don't think the flag counts are changed for all the things they had flagged. There's an issue for this somewhere!

In the meantime, I've found the API function I *should* have suggested: flag_get_entity_flags(). That should do what you need, so no need for a new function.

Looks like your patch still needs to fix the strings in the calls to t().

ergophobe’s picture

Arr!!! Sorry to make you work so hard for this little patch!

>>Rules

I'm not sure how people are using this with Rules and I personally am not, so I'll let it ride. It was more of a question.

>>your patch still needs to fix the strings

Damn! I don't know why I forgot that the translation tools are parsing the text itself, not the evaluated text. It's not like every single doc on t() doesn't clearly state that it must be a string literal for this reason.

>>I *should* have suggested: flag_get_entity_flags()

Ahh. I should have found that too. I saw the description related to users and didn't really look at what it did. It gives me an array of users and returns a zero-element users if no user has flagged the entity. That's perfect for my use. So that makes the patch simpler.

Hopefully I've got it this time. Thanks for your help and patience. I really appreciate your efforts to get this patch in shape (and I do think it is a good and worthwhile fix).

ergophobe’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: flag-panels-access-check-2151157-11.patch, failed testing.

ergophobe’s picture

Status: Needs work » Needs review
FileSize
3.09 KB

Updated to current git repo to avoid conflict from last commit (just a conflict in the comments)

joachim’s picture

Status: Needs review » Fixed

Committed. Thanks! Sorry for the delay, was hoping this would get community review... but it looks fine and your video was very helpful! :)

git commit -m "Issue #2151157 by ergophobe: Added ability to check for flagging by any user as well as current user to the CTools access plugin." --author="ergophobe "

ergophobe’s picture

Ha! Thanks! Good timing

I had completely forgotten about this issue. The funny thing is I just saw that patch yesterday comparing my current version and the distro and I couldn't remember where that patch was from. I forgot that I wrote it! (it is in the _patches folder for the project and it references this issue, so I would have figured it out eventually).

Status: Fixed » Closed (fixed)

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