Hook to respond to change of source translation
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | translation.module |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | i18n sprint |
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| translation-source-change.diff | 1.45 KB | Idle | Failed: Failed to install HEAD. | View details | Re-test |
#1
Included this hook invocation in a helper module for D6: http://drupal.org/project/translation_helpers.
#2
#3
Maybe this would be better as a new $op to
hook_nodeapi(): 'change translation source'. We could pass a$node->new_tnidproperty. This would be something like the existing 'prepare translation $op.#4
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:
<?phpfunction 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.
#5
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.
#6
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.
#7
The last submitted patch failed testing.
#8
See: #335122: Test clean HEAD after every commit and http://pastebin.ca/1258476
#9
The last submitted patch failed testing.
#10
#11
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.
#12
Added tests and hook documentation. Should be ready for review.
#14
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.
#15
The last submitted patch failed testing.
#16
re-sending.
#18
#20
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:
nodeapi_translation_changehook (using node_invoke_nodeapi()) after the tnid for a node has been updated. This allows other modules to act on the translation set change.hook_translation_change()function is invoked correctly.simpletests/tests/translation_test.module(and info file) which is needed by the new simple tests. The module just contains an implementation ofhook_translation_change().hook_nodeapi_translation_change().#21
with re-rolled patch this time.
#22
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.
#23
The last submitted patch failed testing.
#24
Patch reroll
#25
The last submitted patch failed testing.
#26
Tests pass on my install, changing to needs review so it can be retested
#27
The last submitted patch failed testing.
#28
Re-roll
#29
Changing back to RTBC
#30
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