node flag count error with i18n translated nodes

I notice that flag_get_counts() bypasses flag_node::get_translation_id()

This just tripped me up, and may also be responsible for this attendance count issue?

edit: or at the very least, it seems that flag_node::get_count() should be implemented?

This patch implements flag_node::get_count()

I suspect this will fix the original issue, but I've not tested that.

Assuming it does, I'm still unsure whether this should be considered sufficient for a solution, or if flag_get_counts() needs to become translation-aware as well?

flag_get_counts() has to remain generic.

This patch looks fine to me but I'd appreciate testing from someone who has a multi-language site.

Better title.

Set strong for the bug.

I got same issuse with 7.x-3.3. IF i choose 'Flag translations of content as a group' option in flag edit form, the flag count will be always show the count of source node.

Here is the patch for 7.x-3.3 which is tested on my multi-language site, please review.

I won't change the status as I'm not running 7.x-3.x, but this looks functionally identical to my patch in #2 (defining flag_node::get_count() with an identical definition), so it gets my vote.

Hmm so hang on, let me see if I understand.

You have a node that is in English, and also a translation of that node in Japanese. Someone flags the English node, and its count is now 1.

You expect the Japanese node to have a count of 1 too?
What if you wanted to specifically flag translations? Say you're a site translator, and you have a flag called 'Needs review from a translator'?

Also, do you expect just the count to go up, or do you want the flagging to somehow carry over too? If I flag the English one with a non-global flag, and then look at the Japanese one, should I see that as being something I've flagged or not?

Well we're talking about the effect of the "Flag translations as a group" option.

If that's enabled, then a flagging should affect all nodes in the translation set, and therefore the count should be shared across all nodes in the translation set.

IIRC the former happens, and this patch is to fix the latter.

@joachim what #5 patch does is to get the number of flags of the translation of the node, if Translation Helpers Module exists, by overriding the get_count() in flag_node.

There exists similar codes doing the translation for the action flag, please see http://drupalcode.org/project/flag.git/blob/HEAD:/includes/flag/flag_nod...

function flag($action, $entity_id, $account = NULL, $skip_permission_check = FALSE, $flagging = NULL) {
   $entity_id = $this->get_translation_id($entity_id);
   return parent::flag($action, $entity_id, $account, $skip_permission_check, $flagging);
 }

Therefore, #5 patch looks solid for me.

I'm using patch #5 without problems.

We have this option in the UI:

    // Support for i18n flagging requires Translation helpers module.
    $form['i18n'] = array(
      '#type' => 'radios',
      '#title' => t('Internationalization'),
      '#options' => array(
        '1' => t('Flag translations of content as a group'),
        '0' => t('Flag each translation of content separately'),
      ),

but AFAICT, the functionality in the patch is not paying heed to it.

But that option is precisely what the get_translation_id() method deals with, right? (Or is this some additional option? I don't have my editor available right now, so I can't easily search the code to check; apologies if I'm confusing two different things.)

Ah, I get it now. Thanks for persisting with my lack of understanding!

This should have tests though, as it's functionality that's not that commonly used, and also quite complex to reproduce. I'm working on them :)

Oops.
Turns out though that we don't need translation_helpers for this test, as we're not deleting translations.