Multilingual content in Drupal core presents new challenges for Flag as for many other Drupal modules. See this handbook page for some technical details: http://drupal.org/node/304047. Specifically: when flagging a node, should the flag apply only to a given translation or should it apply to all members of a translation set (the set of versions of a given piece of content in different languages)?

Probably both of these are valid use cases.

Here's a possible approach:

1. Introduce a configuration option for flag types. Each flag type can be set either to (a) 'default' - flags apply to the node or (b) 'source' - flags apply to the source translation or, put another way, to the translation set.

2. Wherever we now have a nid reference, we selectively substitute the tnid if available. We could use helper functions like this:


/**
 * Return the node field to be used for flagging.
 */
function _flag_translation_get_field($flag) {
  return $flag->translation == 'source' ? 'tnid' : 'nid';
}

/**
 * Return the node ID field to be used for flagging.
 *
 * If the flag type is set to use the source translation for flagging, the {node}.tnid
 * value will be returned if set. Otherwise, the {node}.nid value will be returned.
 */
function _flag_translation_get_value($flag, $node) {
  $field = _flag_translation_get_field($flag);
  return (isset($node->$field) && !empty($node->$field)) ? $node->$field : $node->nid;
}

Result: flags may be set to and read from the source translation, such that a translation set is flagged rather than an individual translation.

Does this sound right? Comments on the proposed approach? I'll work up a patch once we get the approach nailed down.

CommentFileSizeAuthor
#46 flag_i18n.patch7.62 KBhailu
#41 flag_i18n.patch7.34 KBquicksketch
#38 flag_i18n.patch7.33 KBquicksketch
#31 flag_i18n.patch7.1 KBquicksketch
#29 flag-i18n.patch6.94 KBwmostrey
#26 flag-i18n.patch6.48 KBwmostrey
#25 flag-i18n.patch6.45 KBwmostrey
#24 flag-i18n.patch4.9 KBwmostrey
#21 flag-i18n.patch4.03 KBwmostrey
#18 flag-i18n.patch6.61 KBwmostrey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mooffie’s picture

mooffie CreditAttribution: mooffie commented

And what of the possibility to flag all the nodes in the translation-set? (That is, not just the 'source').

There are two posibilities:

1. To flag/unflag the translation source (that's what you suggest).
2. To flag/unflag all the nodes in the translation set.

The first method has some disadvantages. The biggest I can think of right now is Views support. How can the admin list all flagged nodes? It's not the nodes directly that are flagged, but their 'source'. Yes, there are solutions, but why introduce complexity?

We could use helper functions like this:

(Or it's possible to continue, in the API, to use NID and not TNID. The node flag object will internally convert the NID given to TNID. The user of the object (the programmer) won't even have to know that she flags the translation source and not a node directly.)

======

The second method, of flagging/unflagging the whole translation-set, has some advantages:
- Views support works as is.
- Much of the current code can stay.
- The admin/designer deals only with NIDs, not with TNID, which she may find more familiar.
- The implementation is relatively easy.

The base flag class has a flag() method. The derived flag_node class could override it to make it flag, or unflag, the whole tranlation-set:

function flag(..., $content_id, ...) {
  $node = $this->fetch_object($content_id);
  $nodes = translation_node_get_translations($node->tnid);
  foreach ($nodes as $node) {
    parent::flag(..., $node->nid, ...);
  }
}

There are other issues to consider. For example, the Actions support: It's obvios that upon flagging a node as "spam" we would want to 'Unpublish' all the translation set. On the other hand, when, in addition, we email the node, we wouldn't want to send all the translation-set. So an Action may have to have a radio selector:

- Operate on all the tranlation set.
- Operate only on the node directly flagged.
- Operate only on the translation source.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

@moofie

Thanks for your comments. You've certainly identified the core challenge here--how to choose correctly between nid and tnid as the primary identifier.

See the related discussions on Voting API #306567: Handle multilingual content and on Views #307032: Support for displaying translation sets.

It is a complicated and interdependent mix of issues we're dealing with and certainly we need to keep an eye on what's emerging elsewhere. That said, my feeling is that we're probably (as usual) doing well if we try to get our own problem right rather than compensating for problems elsewhere.

What I mean is, sure, maybe there are limitations in what Views can do now with translation set data, or in how actions and triggers work with translation sets as opposed to individual nodes. But that shouldn't push us to e.g. register flags on all members of a translation set individually, so they all get actions/views/whatever exposure. Rather, we should choose the data handling that's right for our case, and so help promote correct solutions elsewhere.

My feeling is, at least in many or most cases, we're flagging a piece of content, not a translation. So introducing the need to aggregate or synchronize or filter so we only see a certain language is not necessary.

This code in Panels is relevant:


  // Support node translation
  if (module_exists('translation')) {
    if ($translations = module_invoke('translation',
'node_get_translations', $nid )) {
      if( $translations[$GLOBALS['language']->language] )
      {
        $nid = $translations[$GLOBALS['language']->language]->nid;
      }
    }
  }

It basically says: if the node to be displayed in a panel is a source translation, show instead the version in the current language, if there is one. Or, more generally, store data for the source translation but, on display, provide language-appropriate substitution. My hunch is, that's the option we should provide in flagging.

Sound right?

mooffie’s picture

mooffie CreditAttribution: mooffie commented

It basically says: if the node to be displayed in a panel is a source translation, show instead the version in the current language, if there is one.

Ah, I see. It makes sense.

This is new to me, because I don't know what is the "proper" way to list localized content. I'll read the material, and the links you provided, very soon --till then I'm afraid I won't be able to contribute much to the discussion.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented

I've been digging into this and the most important thing for me is to keep things simple. I think it makes the most sense to provide two options, selectable on a node-per-node basis:

1/ Flag just this node
2/ Flag this node and all its translations

Without i18n enabled you would receive the default message of "Are you sure you want to flag this content?". With i18n enabled you would have this simple radio form. I also think it makes more sense to do provide this option node-per-node basis but with the option for the admin to set a default.

Mooffie and Nedjo, do you agree this is a good solution? If you agree it is, then I'll start working on a patch.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

@wmostrey : thanks for looking into this.

I think we need a bit more discussion and clarity before we're ready for a patch.

I see the potential flexibility that a per-node choice would make, but I suspect most users will simply be confused by this. And I suspect that, if indeed there is a strong use case for per-translation flagging (something I'm beginning to doubt), this will be determined by flag type, rather than individual flag.

As I noted above, I lean towards a solution that involves the minimum required data storage. If we want to flag all translations, we should store this as one datum (probably, the nid of the source translation). We can then display that any way we want. E.g., the patch in the Views issue I linked to above should soon provide us with a lot of flexibility for working with translation sets. The advantages include:

* efficient data storage--avoid duplication
* avoid need to synchronize data when changes are made (e.g., unflag a given translation)
* avoid need to remove duplicates on display

Assuming that, if we use the source translation (tnid) for to register a flag, we can do things like display all flagged content in any given language, etc., is there any drawback to using the tnid? Is there something missing that would justify the problems associated with independently flagging a whole set of nids?

Let's try to define a use case for per-translation flagging. So far as I can see, this is a very limited need. In almost every case, what a user is flagging is the content per se, not the way it was translated. So the limited use case for per-translation flagging might be: flag bad/good translations. Are there other valid use cases? It's worth asking if this is even a strong enough use case to justify the per-translation flagging functionality.

Take polling as a somewhat similar example. When you're voting in a poll, are you voting on a translation (nice translation!) or are you answering the poll question in whatever language (what is your favourite colour/cual es tu color preferido)? In almost every case, it's the latter. So the multilingual polls module (i18npoll, part of the internationalization package) assigns votes to the source translation. When you're viewing a poll, you get the votes for the full translation set--that is, the votes assigned to the source translation.

moofie, have you given this further thought?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

I think nedjo's headed down the right path. I think "per-node" translation flagging is configuration-heavy. I'd much rather see it implemented at the flag settings level, so that the "site builder" can setup that portion, while the people creating content don't have to worry about it.

I was thinking through the per-flag need, and came up with a few situations where you'd want a flag to behave in either a "direct node flag" or a "parent node flag".

- Say you want to "bookmark" a node, as long as you have the Views setup correctly, you could get your list of bookmarks in any language (cool).
- An edge-case I can think of is an "offensive" flag where the translations are community-provided, and a single translation is offensive somehow.
- Or say you have a "translation needs work" flag, then you'd definitely want to flag the translation instead of the parent node.

So there's definitely a need for both options, but I still can't think of any reason for a per-node setting rather than configuring this setting while setting up the flag.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented

OK I agree per-node settings are over the top, because of the reasons Nedjo and Nathan bring up. Nedjo is making good progress on the views issue as well.

So I think we're getting clear on what we want: the site administrator sets if he wants the flag to be on the current node or the translation node set, and this applies to each flag set. Do we want this to be configurable per node type? I think that would make sense.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

Do we want this to be configurable per node type?

I think per flag type is probably enough.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented

With the views issue also fixed (nice work Nedjo!), do we now have a clear description of where we want to go with i18n support for the flag module in #7?

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

the site administrator sets if he wants the flag to be on the current node or the translation node set, and this applies to each flag set

Yes!

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented

I wonder what needs to happen when a user goes to, for instance, nl/flags/bookmarks when the i18n option is selected. Does he see all translations of the nodes, or only the node in the active language? And what about previously flagged nodes that don't have a translation in the active language? Do those nodes show up or not?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

I doubt we'll change the default views provided with Flag to accommodate for internationalization, meaning that all flagged nodes in all languages would show up there. If needing to change it to suit a particular purpose, you'll probably have to override it and customize it to whatever purpose is needed on your site.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented

Nathan, so you would prefer to flag all translated nodes instead of just the "source" node?

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

@wmostrey: I think it'll work best to focus first on the basic functionality in #10. From there we can return to consider what if anything needs to be done with default views, possibly in a separate follow-up issue.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented

Hi Nedjo, I'm good under way to implement this but it's not clear to me if I should flag the tnid or if I should flag all translations separately (which would result in what Nathan describes in #12).

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

I don't think that's what Nathan meant. The spec as I understand it is:

1. Each flag type can be set on creation to apply either to (a) individual translations or (b) the translation set, with (b) being the default.

TBD: is this choice available only if translation module is enabled? My initial feeling: yes.

TBD: is it possible to change a flag type from (a) to (b) after it was created? My initial feeling: no.

2. When flagging occurs for type (b) flag types, the flag is registered to the tnid if present.

Note: we could test for translation_supported_type() here and look for tnid only if TRUE.

3. When loading the flags for a given piece of content, the tnid value (if present) is used for type (b) flag types.

When we have something like that in place, we can return to work out the details of how this should work in default views. (I think the new translation set functionality in views will get us most but not all of the way.)

Sound good?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Yep, Nedjo, sounds like you have the right idea. I probably wouldn't bother with your second TBD item (possible to change a flag type from (a) to (b) after it was created), I think it'd be fine to allow users to change this option at any time, though provide a message in the option description informing the user that existing flagged content will not be updated.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented
Status: Active » Needs review
FileSize
flag-i18n.patch6.61 KB

Here's a first grab at this.

What we might need is an extra i18n field in the flag_content field to allow users to switch between i18n enabled en disabled. This would not be needed if we go with Nedjo's suggestion:

is it possible to change a flag type from (a) to (b) after it was created? My initial feeling: no.

But I agree with Nathan that we should grant this option.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs review » Needs work

Thanks Wim! This looks like a pretty solid start. Some things that need addressing:

- We don't need to add a DB column for i18n, store this in the serialized options column (I think moofie and I are headed towards putting nearly all information in options actually, as we find more and more options only apply to certain types of content).
- All this translation code is currently specific to nodes, so we should put the new functions into flag.inc as methods of the flag_node class.
- There are some minor spacing issues in the PHP doc. Precede all the lines except the first of PHP doc with a space.
- The changed delete code in nodeapi would cause all flag information to be lost on a parent node when any of it's translations were deleted. This should be changed so that flaggings are only deleted if the node being deleted is the same as the tnid.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

Some notes for a later version of this patch, once the basics are in place.....

One thorny issue is how to handle changes in the translation set. Since we're storing information by tnid rather than nid, we need to handle the cases where either

(a) the 2nd to last translation in a translation set is deleted, so the tnid is reset to 0 or

(b) the translation source is deleted so a new tnid is assigned for the full translation set.

This change is handled in translation_remove_from_set(). We need to replicate that logic and then respond accordingly.

Because the logic is complex and will be needed by multiple contrib modules, core should be informing contrib modules about translation set changes. #318328: Hook to respond to change of source translation is a proposal to introduce this hook into D7.

For D6, I've written a small helper module, translation helpers (http://drupal.org/project/translation_helpers) that introduces this hook. Proposed implementation in this patch:

1. In hook_requirements() in flag.install, if translation module is enabled, test for translation_helpers and throw an error if not enabled.

2. In hook_nodeapi(), respond to changes in translation set. Specifically:

For case (a), we need to update existing data for flag types that use tnid to use the nid of the remaining former member of the translation set.

For case (b), for flag types that use tnid, we need to substitute the new tnid value for the old one.

See the translation_helpers project page for a sample hook implementation that can be used as a model.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented
Status: Needs work » Needs review
FileSize
flag-i18n.patch4.03 KB

This patch integrates Nathan's first 3 comments. What I'm wondering before diving in the delete issue and Nedjo's suggestions I would like to get the confirmation that the nodeapi call also gets run when using the AJAX call to flag/unflag a node instead of the node/edit screen. From what I can see this is not the case and I'm not sure where to set the translation calls for this. It would be great if I could get some guidance on this.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

@wmostrey: Thanks for the new patch version!

Minor notes: It's got a couple of debugging watchdog calls in it. The #description added should have a single space between sentences.

More stuff:

1. Probably we'll need some more help describing this functionality, as it's pretty hard to understand. Maybe just a longer #description for the form element.

2. We should show the form elements only if translation module is enabled--otherwise it won't make sense. Maybe add a #type 'value' form element if translation module is not present.

3. For now we should lock the i18n setting--it should be possible to edit only for new flag types. I guess test for the flag id on the incoming $flag variable and, if present, pass the form element in a locked version.

Re using nodeapi as I've suggested to handle changes in the translation set, this is for responding when a member of the translation set is deleted, so it doesn't need to be called when a node is flagged/unflagged.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented

Thanks for the input Nedjo. I actually meant flag's hook_nodeapi.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented
FileSize
flag-i18n.patch4.9 KB

Another follow-up patch which incorporates Nedjo's suggestions in #22. I'm now working on the integration with the translation_helpers module for flag deletion.

I could definitely use some help with the wording of what exactly the i18n setting does.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented
FileSize
flag-i18n.patch6.45 KB

Attached patch also integrates translation_helpers and adds it as a hook_requirement when the translation module is enabled. From what I can see we have the complete functionality covered now.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented
FileSize
flag-i18n.patch6.48 KB

I added a small correction because we can't use t() in the .install file.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

This is looking good. I haven't tested. A few comments:

1. coding standards:

* This line has a tab rather than spaces for indenting:

+  	if(module_exists('translation') && !module_exists('translation_helpers')) {

* There should be a space after if

2. nodeapi $op = 'delete'

The $id value won't be set, because it's set in a different $op. But in any case we need to rewrite this handling.

We can't delete the data pegged to a translation source, because then when translation_helpers runs it won't have data to work with, so when the 'translation_change' op is called nothing will be updated.

Rather, what we need to do is *skip* deletion for any source translations where the flag is set to use the translation set.

Here's suggested code, untested:


      foreach (array_keys($node->flag) as $name) {
        $flag = flag_get_flag($name);
        // If flags are being tracked by translation set, we don't delete if there are 
        // existing translations.
        // Instead, existing data will be updated in the 'translation_change' op, below.
        if ($flag->i18n && translation_node_get_translations()) {
          continue;
        }
        else {
          db_query("DELETE FROM {flag_content} WHERE content_type = 'node' AND content_id = %d", $node->nid);
          db_query("DELETE FROM {flag_counts} WHERE content_type = 'node' AND content_id = %d", $node->nid);
          break;
        }
      }

Probably this needs a bit more work though, since it could delete data for one flag type that's needed by another.

3. This line in options_form() seems awkward/brittle:


+      if(arg(3)=="add") {

Is there some $form element or value we can test for to distinguish between creation and editing? E.g., is there a flag ID already set when editing? If not, it would be better to set e.g. a hidden form element passing the flag ID.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

This might work to delete only the appropriate flags:


      foreach (array_keys($node->flag) as $name) {
        $flag = flag_get_flag($name);
        // If flags are being tracked by translation set, we don't delete if the node is part
        // of a translation set.
        // Instead, existing data will be updated in the 'translation_change' op, below.
        if ($flag->i18n && isset($node->tnid) && $node->tnid) {
          continue;
        }
        else {
          db_query("DELETE FROM {flag_content} WHERE fid = %d AND content_type = 'node' AND content_id = %d", $flag->fid, $node->nid);
          db_query("DELETE FROM {flag_counts} WHERE fid = %d AND content_type = 'node' AND content_id = %d", $flag->fid, $node->nid);
        }
      }
wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented
FileSize
flag-i18n.patch6.94 KB

This patch incorporates the input of #27 and #28.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

Notes on testing needs.

This is a relatively difficult patch to test. Steps would be:

Installation and requirements
1. Enable locale module. Install French and Spanish modules.
2. Enable translation module.
3. Enable flag module.

Expected behaviour:
hook_requirements error notifying that translation_helpers module is required (since translation module is enabled).

4. Enable translation_helpers module.

Content creation
5. Make 'page' content type translatable. Create an English version and then translate into Spanish and Frensh.
6. Create a new flag type. Make translatable.
7. Flag the Spanish version of the node for that flag type.
8. Test to see if the English version is flagged. Expected result: the English version is also flagged.
9. Unflag the English version of the node for this flag type. Test to see if the Spanish version is flagged. Expected result: the Spanish version is not flagged.

Content deletion:
10. Flag the French version of the piece of content.
11. Delete the English version of the piece of content. Test to see if the Spanish version is still flagged. Expected result: the French and Spanish versions are still flagged. Test the ID value in the flag. Expected result: it is the same as the {node}.tnid value of the French and Spanish translations.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs review » Needs work
FileSize
flag_i18n.patch7.1 KB

This is a revamped patch that makes the following corrections:

- Allow i18n option to be toggled on exising flags.
- Removed translation_get_field() and translation_get_value(). We already have the method get_content_id() which can be used for the same purpose.
- Cleaned comments to fit within phillips bar (80 characters)
- Removed changes to flag overview table (since i18n only applies to node content).
- Set i18n option to TRUE by default on new node flags if the translation module is available.
- Moved hook_requirements message outside of "install" $phase, since this message is important even after the installation or when translation is enable after flag is installed.
- Added weights to the flag edit form to put options in better places.
- Changed i18n option to radio buttons for clarity.

Overall, this whole thing works really quite well. However, there is some strangeness when dealing with deleting the "source" translation that has been flagged. An example:

1. Create translatable piece of content and an i18n enabled flag.
2. Translate the content into English (source), Spanish, and French
3. Flag any translation of the content, effectively flagging the source translation.
4. Delete the English (source) translation.

Expected: The Spanish and French translations are now individually flagged.
Actual: The Spanish version is individually flagged, but the French translation is not flagged at all.

Also, the Spanish and French translations are now completely unrelated to each other as far as Drupal knows. I expected they'd still be associated and one of them would take up the role of being the "source" translation. So this makes me think that our implementation works perfectly, but Drupal is not re-associating the remaining translations when the source is deleted. Perhaps this should be handled in the translation_helpers.module?

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented

Thanks for the additional cleanup and testing.

The bug you're hitting may be this core bug: #260372: All translations are removed from set when deleting one node. It's been fixed in HEAD and 6.x dev.

I suppose we could try to include a fix for this in translation_helpers, testing for Drupal core prior to 6.6, when presumably this fix will hit a stable release. Should we?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Yep, Nedjo you're correct. With that core patch (or by using 4 languages instead of 3), everything works perfect! I don't think a temporary work-around in translation_helpers is a necessary. If we can get a final review on this, I'll go a head and put it in.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Needs work » Needs review
mooffie’s picture

mooffie CreditAttribution: mooffie commented

(Nathan, I won't be here for a day or two (holiday) so I won't be able to review this right away. Of course, you don't have to wait for my review. Incidentally, it's nice that this patch is so small. Wim and Nedjo, thank you for this work!)

hailu’s picture

hailu CreditAttribution: hailu commented

Hey all, I am going to start to draft a simpletest for this multilingual functionality, i'll report back when i have more.

mooffie’s picture

mooffie CreditAttribution: mooffie commented

I haven't tested this patch but I read it carefully.

This patch seems to break snippets that use the $flag->is_flagged() method, including our own code:

- Checkboxes shown on the node editing form won't reflect the correct state of the flag, will they?
- Ditto for the links flag_create_link() produces (unless the site builder passes it $tnid, not $nid).

Another issue: have you considered moving this i18n support one level down, to the API level: to flag::flag() and flag::is_flagged() ?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
FileSize
flag_i18n.patch7.33 KB

Here's a revised patch that takes the suggestion by mooffie and puts it into code. I think this approach is *much* more resistant to breaking. Now you can flag any node you want (translated or not), and when the flag is actually saved, Flag makes the change from NID to TNID for you if needed. Same goes for retrieving the status of a node, so the node form and links are all updated properly.

mooffie’s picture

mooffie CreditAttribution: mooffie commented

It looks better now!

Looks RTBC to me.

Comments:

  1. This feature is turned on by default. However, the default views we provide won't work correctly (the flag/unflag link reflects the status of the direct node, not of its translation set). We can't ship something (the default views) that doesn't work by default, so I think this feature (i18n) should be turned off by default. (I'll write a handbook page explaining how to construct views that work with this feature.)
  2. case 'delete': $flags = flag_get_flags(); This should be changed to ... = flag_get_flags('node'); because we want to examine node flags only. The content_type = 'node' can then be removed from the SQL that follows.
  3. case 'translation_change': [...] "UPDATE {flag_content} [...] Doesn't this SQL modifies rows of flags that have i18n tunred off as well? Shouldn't the previous `for ()` loop be used here as well?
  4. '#default_value' => empty($form['#flag']->fid) This could be written as [...] => !$this->fid (because 'fid' always exists (it's set to NULL for new flags)).
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#38 looks nice to me and testing went well. Good that this can be achieved with only minimal changes.

Flagging source nodes in sets and translations all works fine - the rest of the set gets updated great. Deleting one or the other also keeps the flags on nodes in that set intact. When translating nodes which are already flagged one way or the other, the new nodes are flagged correctly. Applying translations to an individual node as opposed to translation set is also still fine. I short, I tried hard to break it, and couldn't.

One minor issue:
If nodes from the translation set are displayed on the same page, the javascript link update only applies to the individual node actually being flagged - on page refresh the rest appear as flagged of course. This is ultra-minor and possibly won't fix, but since I noticed it I guess it's worth mentioning.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
FileSize
flag_i18n.patch7.34 KB

Mooffie's comments in #39 are all valid. This patch corrects each problem.

1. This feature is turned on by default. However, the default views we provide won't work correctly (the flag/unflag link reflects the status of the direct node, not of its translation set). We can't ship something (the default views) that doesn't work by default, so I think this feature (i18n) should be turned off by default. (I'll write a handbook page explaining how to construct views that work with this feature.)

A valid point. I also have some thoughts on the default views we provide: #333984: Remove Dynamic Default Views. This could make implementing support for translation module into our default view a little easier. But for now I've simply defaulted the i18n flag behavior to "off".

2. case 'delete': $flags = flag_get_flags(); This should be changed to ... = flag_get_flags('node'); because we want to examine node flags only. The content_type = 'node' can then be removed from the SQL that follows.

Yep, I shortened this up a little further, since we never use the $flags variable at all, so we can just do foreach(flag_get_flags('node') as $flag).

3. case 'translation_change': [...] "UPDATE {flag_content} [...] Doesn't this SQL modifies rows of flags that have i18n tunred off as well? Shouldn't the previous `for ()` loop be used here as well?

Nice catch. This would've been an obscure bug to find later. Fixed in this version.

4. '#default_value' => empty($form['#flag']->fid) This could be written as [...] => !$this->fid (because 'fid' always exists (it's set to NULL for new flags)).

Now that we default this value to off, this line became much simpler anyway. '#default_value' => $flag->i18n,

Also, what should we do about Drupal 5? Just not support this feature at all? The significant differences between Drupal 5 and 6 make this a bit challenging. The translation_helpers module would also need to be ported to Drupal 5, or included as part of the Drupal 5 version of translation.module.

mooffie’s picture

mooffie CreditAttribution: mooffie commented

catch, thank you for the time you took to test this. It's appreciated.

mooffie’s picture

mooffie CreditAttribution: mooffie commented
Status: Needs review » Reviewed & tested by the community

Looks fine!

I don't think we need to bother with D5.

(I won't be here till next week so I won't be able to contribute to the other issues you opened today.)

mooffie’s picture

mooffie CreditAttribution: mooffie commented

I think the `$flag->applies_to_content_id($node->nid)` test could be removed (in flag_nodeapi). Or else deleting a node whose type was later unchecked in the flag settings form won't remove its entry from our tables.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Reviewed & tested by the community » Fixed

Thanks mooffie that's a very good idea. I added a note about the feature in Drupal 5 and committed almost everything to both branches (Drupal 5 is missing the hook_requirments() notice for translation_helpers). So this is now a Drupal 6-only feature. We can work on Drupal 5 later if there is adequate support behind it.

hailu’s picture

hailu CreditAttribution: hailu commented
Status: Fixed » Needs review
FileSize
flag_i18n.patch7.62 KB

Here's a quick re-roll with moofie's last comment on the test in the delete case of flag_nodeapi

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Status: Needs review » Fixed

Nice! Thanks all for sticking with this complex issue. It was worth the time to refine the solution.

Gábor just commented on #318328: Hook to respond to change of source translation. He might have alternate suggestions to the nodeapi hook we've used here and in translation_helpers. I've opened an issue to track this: #334371: Update for (potential) changes in translation_helpers. We can make any needed changes there, after #318328 is applied.

(Looks like Hailu crossposted with his patch update. Setting back to fixed.)

Status: Fixed » Closed (fixed)

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

volocuga’s picture

volocuga CreditAttribution: volocuga commented
Component: Code » Flag core

So what s about drupal 5?