field_attach_prepare_translation needs to be updated for D7 API

Regardless of what happens with #367595: Translatable fields, we probably need this now. Body now being a field, the 'prefill body field on new node translation form' is currently broken.

The current code contains unfinished code for field_attach_prepare_translation(), field_default_prepare_translation() (directly taken from CCK for nodes).
They simply need to be updated to the D7 Field API style - and then node.module needs to call field_attach_prepare_translation() appropriately.

bumping to critical. Just needs to be done.

If D6-style translation (e.g different nodes) remains in core (which is still not sure, if I read webchick's comments on one of the TF followup threads, I think it was 'TF UI'), then we need the field_attach_prepare_translation() func and hook to support "body prefilled with source language in 'new translated node' form".

The functions are nothing more than stub, not to-date code for now.

subscribing

This one depends on the destiny of #539110: TF #4: Translatable fields UI. If the current node translation stays in core I'll work on this.

@plach: What's the future of this issue?

@sun: Simply didn't have time to work on it, yet. If someone else wishes to take it, feel free :)

Nope, this is for D7. Although, not critical.

Oh, that's why I couldn't find this anymore.

Here's an initial draft, definitely needs work. Let's see if the bot is happy.

Discussed this patch with plach. To solve Translation module's requirements, we need to introduce a new hook_field_prepare_translation() for every field module. The current logic in Translation module is based on D6 and does not account for field API at all.

I'm also inclined to bump this issue to critical, but plach just tells me that Translation module currently doesn't work like it should anyway. A quick fix without API change would lead to a critical security issue. The API change (addition) is required to not introduce a security issue. But leaving as normal for now.

From CCK D6 :

function content_prepare_translation(&$node) {
  $default_additions = _content_field_invoke_default('prepare translation', $node);
  $additions = _content_field_invoke('prepare translation', $node);
  // Merge module additions after the default ones to enable overriding
  // of field values.
  $node = (object) array_merge((array) $node, $default_additions, $additions);
}

First line called the '"default" behavior : content_field($op = 'prepare translation') - in D7, that would be a field_default_prepare_translation() func.
Second line called the field type's hook_field($op = 'prepare translation') - in D7, that would indeed be hook_field_prepare_translation()
Nodereference D6 indeed implemented this to replace do some advanced replacement (trying to find the translation of the referenced node if it exists - code was by nedjo, IIRC :-p)

So true, we do need this field-type hook, in addition to the default method.

My bad: I wrote the patch some time ago and I thought I introduced hook_field_prepare_translation() in #12. Perhaps we are missing hook_field_attach_prepare_translation(), though.

This should fix the format_access() issue by implementing hook_field_prepare_translation() in the Text module; I don't think the other core fields need implementing it.

Docs are stil needs work but code should be ok. Again let's see if the bot is happy.

+++ modules/field/field.attach.inc	9 Apr 2010 00:06:30 -0000
@@ -1320,23 +1320,23 @@ function field_attach_preprocess($entity
+function field_attach_prepare_translation($entity_type, $source_entity, $entity, $source_langcode, $langcode) {

I'm not sure whether other field_attach functions are following a certain argument order/pattern, but if they do, it would be good to make these arguments follow those.

Without looking up other functions, I guess that the following order would be more natural:

field_attach_prepare_translation($entity_type, $entity, $langcode, $source_entity, $source_langcode)

+++ modules/translation/translation.module	9 Apr 2010 00:28:47 -0000
@@ -65,6 +65,7 @@ function translation_menu() {
+    'theme callback' => '_node_custom_theme',

If this change is required for this patch, then I guess we need a short inline comment to explain it.

+++ modules/translation/translation.module	9 Apr 2010 00:28:47 -0000
@@ -239,13 +240,10 @@ function translation_node_prepare($node)
     // Let every module add custom translated fields.
     module_invoke_all('node_prepare_translation', $node);

Do we still need this?

Powered by Dreditor.

Fixed #17 and the documentation.

If this change is required for this patch, then I guess we need a short inline comment to explain it.

Actually this is not needed: it just enables the admin theme for the translation page. I was unsure this deserved a dedicated issue.

Do we still need this?

Probably if we go on introducing hook_field_attach_prepare_translation_alter(), hook_node_prepare_translation() won't be needed anymore. We just need to understand if this is still feasible.

+++ modules/field/field.api.php	9 Apr 2010 10:33:43 -0000
@@ -512,8 +510,12 @@ function hook_field_delete_revision($ent
+function hook_field_prepare_translation($entity_type, $entity, $field, $instance, $langcode, &$items, $source_entity, $source_langcode) {
 }

Copy the code from text_field_p_t() ?

+++ modules/field/field.attach.inc	9 Apr 2010 10:34:09 -0000
@@ -1320,23 +1320,43 @@ function field_attach_preprocess($entity
+ * Prepare an entity for translation.

Should be 'prepares' (3rd person)

+++ modules/field/field.attach.inc	9 Apr 2010 10:34:09 -0000
@@ -1320,23 +1320,43 @@ function field_attach_preprocess($entity
+ * Perform all the needed tasks to translate the given entity. By default copy

That's a bit too much. The scope of this function is only to fill-in the form default values for Field API fields when displaying the 'add new (translated) node' form.

+++ modules/field/field.attach.inc	9 Apr 2010 10:34:09 -0000
@@ -1320,23 +1320,43 @@ function field_attach_preprocess($entity
+ * This function allow to implement the per-entity translation pattern, which is
+ * the only one supported by core; per-field translation can be provided through
+ * contributed modules.

'allow' should be 'allows' - the sentence feels awkward even with this correction, though.

Proposal :
"This function is used as part of the 'per node' translation pattern implemented by translation.module."
?

Perhaps we should also state that it only makes sense for nodes, and that (unlike other field_attach_X() functions), entity-type modules probably don't need to call it ?

+++ modules/field/field.attach.inc	9 Apr 2010 10:34:09 -0000
@@ -1320,23 +1320,43 @@ function field_attach_preprocess($entity
+  // Perform module alterations after the default ones to enable overriding
+  // of field values.

I'd suggest : "Let field types handle their own advanced translation pattern if needed." ?

+++ modules/field/field.attach.inc	9 Apr 2010 10:34:09 -0000
@@ -1320,23 +1320,43 @@ function field_attach_preprocess($entity
+  // Let other modules alter the entity translation.
+  // FIXME: Do we need this?
+  $context = array(
+    'entity_type' => $entity_type,
+    'source_entity' => $source_entity,
+    'source_langcode' => $source_langcode,
+    'langcode' => $langcode,
+  );
+  drupal_alter('field_attach_prepare_translation', $entity, $context);

Not sure either. Since this is limited to nodes, the module_invoke_all('node_prepare_translation', $node); in translation_node_prepare() could be enough (and that hook is still needed for non-field node components)

+++ modules/field/field.default.inc	9 Apr 2010 10:39:28 -0000
@@ -218,10 +218,33 @@ function field_default_view($entity_type
+ * Copy source field values into the entity to be prepared.

3rd person

+++ modules/translation/translation.module	9 Apr 2010 10:43:10 -0000
@@ -65,6 +65,7 @@ function translation_menu() {
+    'theme callback' => '_node_custom_theme',

I'd say this deserves a separate patch.

Also, some tests around this in translation.test could be worth it ?

109 critical left. Go review some!

The addition of drupal_alter() was my suggestion, having the idea in mind that contributed modules might want to "really" prepare field values for translation. For example, consider machine-based translation web services or simple value conversions and stuff like that.

This addresses #19-#20.

Perhaps we should also state that it only makes sense for nodes, and that (unlike other field_attach_X() functions), entity-type modules probably don't need to call it ?

IMO field_attach_prepare_translation() makes sense for any entity (above all new ones we never thought about) implementing a per-entity translation pattern, that's why it replaces hook_node_prepare_translation() with hook_field_attach_prepare_translation_alter(): the latter lets achieve the same tasks as the former but in a more general way.

I forgot to update poll and node.api.php.

Related issue: #771082: Enable the admin theme for the translation overview page.

+++ modules/field/field.api.php	14 Apr 2010 12:26:17 -0000
@@ -512,8 +510,20 @@ function hook_field_delete_revision($ent
+  // If user has no access to the filter used for the field, we should avoid
+  // exposing the source text too.

+++ modules/field/modules/text/text.module	14 Apr 2010 12:26:17 -0000
@@ -590,3 +590,16 @@ function text_field_widget_error($elemen
+  // If user has no access to the filter used for the field, we should avoid
+  // exposing the source text too.

"If the translating user is not permitted to use the assigned text format, we must not expose the source values."

+++ modules/poll/poll.module	14 Apr 2010 12:30:21 -0000
@@ -450,11 +450,11 @@ function poll_validate($node, $form) {
+    $entity->choice = $entity->translation_source->choice;

+++ modules/translation/translation.module	14 Apr 2010 12:26:17 -0000
@@ -239,13 +239,9 @@ function translation_node_prepare($node)
     $node->translation_source = $source_node;
...
+    field_attach_prepare_translation('node', $node, $node->language, $source_node, $source_node->language);

That ->translation_source seems to be duplicate information now.

114 critical left. Go review some!

Fixed #24

That ->translation_source seems to be duplicate information now.

It's used across the whole Translation.module, it might be problematic to suppress it. I fixed poll_field_attach_prepare_translation_alter() to avoid using it though.

Do we need yched for a final pass here?

yched is short on time currently. But this patch looks good anyway, thanks!

Rerolled.

Upgrading to critical as we can't release D7 without this one.

Added function body to hook_field_attach_prepare_translation_alter() in field.api.php.

windows newlines...

#30: field_prepare_translation-362021-30.patch queued for re-testing.

This looks sane to me. Basically makes the existing node-centric translation stuff entity-centric instead. There are API changes here... I'll do my best to summarize to my current understanding, but it'd be great for one of the folks here to confirm:

1. hook_field_prepare_translation now takes a source_entity and source_langcode param.
2. There's a new hook_field_prepare_translation_alter()
3. field_attach_prepare_translation() now takes entity details rather than just a node object
4. Fields that deal with translatable data (e.g. "text" field) need to implement hook_field_prepare_translation().
5. hook_node_prepare_translation() is removed. Use hook_field_attach_prepare_translation() now.
6. If your entity module used to invoke hook_node_prepare_translation:

-   module_invoke_all('node_prepare_translation', $node);

you now do call field_attach_prepare_translation:

+    field_attach_prepare_translation('node', $node, $node->language, $source_node, $source_node->language);

And... committed to HEAD!

4. Fields that deal with translatable data (e.g. "text" field) need to implement hook_field_prepare_translation().

Not exactly. For simple data (text, number...), the default behavior in field_default_prepare_translation() (just copy over the values from the source entity as default values in the translated entity form) is enough.
This hook is for stuff like nodereference fields, that want to do advanced logic like 'if source node references node A, and node B is a translation of node A, then prepoluate node B as a default value in the translated node' (e.g transitivity - easier with a drawing :-p)

This hook is for stuff like nodereference fields ...

Just a specification: Text is implementing hook_field_prepare_translation() because it needs to skip copying a filtered text field value if filter_access() reports that the user cannot view it. Just like we did in translation_node_prepare() only for the node body.

+++ modules/field/modules/text/text.module	21 May 2010 08:33:56 -0000
@@ -590,3 +590,16 @@ function text_field_widget_error($elemen
+/**
+ * Implements hook_field_prepare_translation().
+ */
+function text_field_prepare_translation($entity_type, $entity, $field, $instance, $langcode, &$items, $source_entity, $source_langcode) {
+  // If the translating user is not permitted to use the assigned text format,
+  // we must not expose the source values.
+  $field_name = $field['field_name'];
+  $formats = filter_formats();
+  $format_id = $source_entity->{$field_name}[$source_langcode][0]['format'];
+  if (!filter_access($formats[$format_id])) {
+    $items = array();
+  }
+}

Giving another look to this code, I'm afraid it does not handle multiple values properly :(

Powered by Dreditor.

Working on a followup patch with some tests, hope to post it soon.

The change is pretty straightforward: hide just the items the user has no access to, instead of all items based on the first one. The patch ships a test that shows that the current behavior is broken.

Related issue: #822128: "Textarea + summary" widget broken when field allows multiple values (followup) and associated JavaScript uses fragile selectors.

I'm wondering if there's something we should or could do to signal that the 'translate node' form is not populated with all source values. Would be nice to have sun's feedback on this.

Meanwhile, code looks good and is obviously more correct than the current. RTBC.

#37: field_prepare_translation-362021-37.patch queued for re-testing.

+++ modules/field/modules/text/text.module	8 Jun 2010 20:09:02 -0000
@@ -601,8 +601,10 @@ function text_field_prepare_translation(
+  foreach ($source_entity->{$field_name}[$source_langcode] as $delta => $item) {
+    $format_id = $item['format'];
+    if (!filter_access($formats[$format_id])) {
+      unset($items[$delta]);
+    }
   }

unset? Shouldn't we empty the value and format instead?

I'm not 100% sure, but when considering that I would not know of an originally existing source value in between of other values, then I'd likely prefer to see an empty widget/value among others to at least have a clue that something must have been there...?

57 critical left. Go review some!

plach just tells me that this is actually the case and even covered by the added tests.

Here is a screenshot taken from the latest simpletest debug message.

#37: field_prepare_translation-362021-37.patch queued for re-testing.

Committed to CVS HEAD. Thanks!

Related issue: #879910: text_field_prepare_translation() broken

This was apparently never updated on the 6/7 module update page. Needs to be.

EDIT: See comment #32 above for a summary.

This hook has no equivalent in D6 core, so this doesn't belong to the 6/7 module update page.

Duh. Sorry about that.