A proposed hook to enable modules to respond to a change in source translation.

Explanation:

In translation_remove_from_set(), called from hook_nodeapi(), we respond to node deletion by conditionally removing a now-empty translation set or selecting a new source translation when an existing source translation is deleted.

Any other module tracking information by source translation will need to respond to this event. Multilingual content means that some data are best tracked by tnid (if present) rather than nid. For example, votes, flags, or nodequeues might be tracked by tnid. If so, these modules need to be able to respond to a change in source translation.

Yet doing so is cumbersome and difficult. First there would there be the need to replicate the code in translation_remove_from_set() in a function called from hook_nodeapi(). More importantly, the module would need to have a higher weight than translation, since translation removes/changes the data that need to be tested. Requiring a module weight is a heavy barrier.

Instead, we should provide a built-in way for modules to respond to this change.

Attached patch introduced a hook, hook_translation_source_change(), which feeds the old and new tnid values. Modules can then make appropriate changes in their data.

CommentFileSizeAuthor
#28 translation_change.patch6.64 KBstella
#24 translation_change.patch4.72 KBstella
#21 translation_change.patch6.66 KBstella
#16 translation_change_0.patch6.66 KBcatch
#14 translation_change.patch6.66 KBcatch
#12 translation_change.patch6.25 KBcatch
#11 translation-change.patch2.29 KBcatch
#4 translation-change.diff2.29 KBnedjo
translation-source-change.diff1.45 KBnedjo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nedjo’s picture

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

Included this hook invocation in a helper module for D6: http://drupal.org/project/translation_helpers.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Status: Active » Needs review
nedjo’s picture

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

Maybe this would be better as a new $op to hook_nodeapi(): 'change translation source'. We could pass a $node->new_tnid property. This would be something like the existing 'prepare translation $op.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
FileSize
translation-change.diff2.29 KB

Yes, I'm thinking this is better as a hook_nodeapi hook.

Here's an updated patch. It includes a fuller approach based on working through a bit more the needs of modules responding to translation set changes.

Any module that tracks data by nid if untranslated or tnid if translated will need to know that a translation set has been removed and will need to know the nid of the remaining former member of the translation set.

Here's a sample implementation of this hook:


function example_nodeapi_translation_change($node) {
  if (isset($node->translation_change)) {
    // If there is only one node remaining, track by nid rather than tnid. Otherwise, use
    // the new tnid.
    $new = $node->translation_change['new_tnid'] == 0 ? $node->translation_change['remaining_nid'] : $node->translation_change['new_tnid'];
    db_query('UPDATE {example} SET id = %d WHERE id = %d', $new, $node->translation_change['old_tnid']);
  }
}

More context on why this hook is needed:

In flag module we're working on a patch to support multilingual flagging, #307810: Multilingual support for flagging. In many or most cases, when flagging a translated node, we mean "flag this piece of content", not "flag this specific translation". So it makes sense to track data by tnid.

But flag module will need to know when a translation set has been removed or the source translation changed.

For D6, I'm working up a backport of this hook in the translation helpers module: http://drupal.org/project/translation_helpers.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

I agree this hook is a useful thing, since it would allow tying stuff to translation sets as well as individual translations cleanly on the extending module's wish. I am not convinced this should be a new hook or should be this exact creative use of the nodeapi hook. Both seem hackish to me, but I don't have a better idea just yet.

So +1 for the functionality, not positive on the exact implementation however. I am leaning towards doing as the prepare translation op and use the nodeapi though. Possibly not this way however.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

After finally getting a grasp on understanding the problem I can agree this is a highly useful addition. Though I agree it is a little strange hooking into nodeapi(). However, we're already doing the exact same thing for the "prepare translation" nodeapi $op, which is equally important. We're now using the translation_helpers module in Flag module (#307810: Multilingual support for flagging)and I'm expecting the same in Fivestar (#307207: Multilingual voting: option to tally votes by translation set), and it'll be great to remove the dependency on translation_helpers for i18n users.

Status: Needs review » Needs work

The last submitted patch failed testing.

lilou’s picture

lilou CreditAttribution: lilou commented
Status: Needs work » Needs review

See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476

Status: Needs review » Needs work

The last submitted patch failed testing.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Issue tags: +i18n sprint
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
translation-change.patch2.29 KB

Here's a re-roll of #4, added the beginnings of some API documentation in translation.api.php although it could do with a better example. Leaving at needs work for some more lucid docs and a test.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
translation_change.patch6.25 KB

Added tests and hook documentation. Should be ready for review.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
FileSize
translation_change.patch6.66 KB

Converted a few queries to dbtng for good measure. Will try to add some additional tests since the hook is called from some different situations, but could do with some eyes on it.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
translation_change_0.patch6.66 KB

re-sending.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
stella’s picture

stella CreditAttribution: stella commented

Re-rolled patch against latest HEAD and with one minor fix for a typo in a comment. All tests pass (old and new) and coder gives it the okay. I think this patch could be marked RTBC. Here's a summary of what it does:

  • Changes translation.module to use DBTNG when updating the node with the new tnid value and setting translate to 0.
  • Invokes the new nodeapi_translation_change hook (using node_invoke_nodeapi()) after the tnid for a node has been updated. This allows other modules to act on the translation set change.
  • Includes simpletests for testing that the new hook_translation_change() function is invoked correctly.
  • Adds a simpletests/tests/translation_test.module (and info file) which is needed by the new simple tests. The module just contains an implementation of hook_translation_change().
  • Includes api documentation for hook_nodeapi_translation_change().
stella’s picture

stella CreditAttribution: stella commented
FileSize
translation_change.patch6.66 KB

with re-rolled patch this time.

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Status: Needs review » Reviewed & tested by the community

Issue warrants fixing, solution appropriate and parallel to the closest existing code (prepare translation nodeapi op), testing in place.

Gábor in #5 seemed to be musing about a slight variation on the implementation but didn't have any specific suggestions or objections and agreed with the approach of using nodeapi. Aside from that, no remaining issues. This looks ready to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

stella’s picture

stella CreditAttribution: stella commented
Status: Needs work » Needs review
FileSize
translation_change.patch4.72 KB

Patch reroll

Status: Needs review » Needs work

The last submitted patch failed testing.

stella’s picture

stella CreditAttribution: stella commented
Status: Needs work » Needs review

Tests pass on my install, changing to needs review so it can be retested

Status: Needs review » Needs work

The last submitted patch failed testing.

stella’s picture

stella CreditAttribution: stella commented
Status: Needs work » Needs review
FileSize
translation_change.patch6.64 KB

Re-roll

stella’s picture

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

Changing back to RTBC

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

A couple of minor nits:

1. Since we're piggy-backing off of the node hooks, the API documentation should go in modules/node/node.api.php, not modules/translation/translation.api.php.

2. I'd also like to see the API documentation describe a couple of use cases for this hook so I'd know why I might want to implement it or not in my module. If this came with a slightly less arbitrary and made-up test example, that would be even better, but this is not a commit blocker since we have lots of arbitrary and made-up test examples. :P

drewish’s picture

drewish CreditAttribution: drewish commented
Version: 7.x-dev » 8.x-dev

i think this will have to wait until 8.x

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Sadness. I'm updating Flag module for Drupal 7 and it seems we'll still need that ridiculous Translations Helpers module to properly handle translations.

webflo’s picture

webflo CreditAttribution: webflo commented

Related issues in contrib

webflo’s picture

webflo CreditAttribution: webflo commented
Issue tags: -i18n sprint +D8MI, +sprint

Tagging.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue tags: -sprint

I'd not encourage people to work on this at this point (removing D8MI sprint tag) since we are aiming to remove this translation method in core altogether.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

Can this be closed? Is there a relevant issue to link to?

jair’s picture

jair CreditAttribution: jair commented
Issue tags: +Needs reroll

Needs reroll

nedjo’s picture

nedjo
he/him/his
Primary language English
CreditAttribution: nedjo commented
Issue summary: View changes
Status: Needs work » Closed (works as designed)
Issue tags: -D8MI, -Needs reroll

Not needed for D8. For D7, module developers can use the API provided by Translation Helpers.