When creating a new translation of an entity, translatable filed values are prepopulated with the same values included in the source language.
This is great, but this is not enough in some cases. For instance, if you're creating a new translation for a Field Collection (Cardinality Unlimited) field and you click the "Remove" button before the translation is saved, this leads to some weird behaviors, where disappear all the values but the first.

This bug comes from the $form_state['field'][$field_name] that it is not updated accordingly to the prepopulated values and contains data related to the empty translation values.

I think that $form_state['field'][$field_name] values should be also prepupulated to keep consistency and avoid these problems.

Regards

Comments

plopesc’s picture

Status: Active » Needs review
FileSize
2.79 KB
PASSED: [[SimpleTest]]: [MySQL] 721 pass(es). View

Here is a patch that synchronizes also the $form_state['field'] array.

james.williams’s picture

This attached patch goes a little further -- populating the field state data structure recursively, so that child elements (e.g. within field collection fields) are copied across. In the case of field collection through, there is also the field collection entity itself that needs cloning for the new language as the field collection module needs this. It's totally not the right place to have this logic though, in the entity translation module, so I don't really know the best way forward with this. So I've clearly marked the relevant code in the patch with comments -- I would suggest this is tested and confirmed as working for all scenarios, for other entities/fields and field collections too, then the best way of handling field collections' specific case should be worked out.

Hopefully this is at least a step towards getting translatable field collection fields (i.e. the field collection field itself, not the field collection entity itself nor the fields within the field collection entity) working, and allows others to use the patch if it suits their use case.

Perhaps the field collection and entity translation module maintainers need to communicate between them to decide how to best solve this? Field collection relies on the field state data structure in form state, whilst entity translation assumes there is nothing special in that so doesn't do anything with it (without this patch).
Even with this patch, if the code specific to field collections was removed, the logic would just use the source field value (i.e. the source field collection entity) as the translated field value (because the whole field state data structure is copied across). Perhaps a hook needs to exist for a field type providing module to act on creating new translations? (i.e. in the case of field collection, to create a clone of the field collection entity, but presumably other field type providing modules could have other needs?)

mpp’s picture

Priority: Normal » Major

The fix in #1 partially solves the issue. It only works for multi-text fields, not for a multi file upload.

Scenario to reproduce:
Create a node type with three fields:
- multiple value, translatable text field A
- multiple value, translatable file field B
- single value translatable file field C

create an english node with
- 3 or more texts in A
- 3 or more files in B
- 1 file in C
- save

now translate the node to another language (eg dutch)
- scenario 1: save, all fields are there => OK
- scenario 2: but if you delete image C before saving, only item 1 from A and B are stored in the translation => NOT OK

The post data for fields A & B in both scenario's are identical so there must still be an issue somewhere in the form state.

The patch in #1 will fix the multiple text field A, however the multiple file field still only saves the first item.
There is a typo in the patch in #2 lb_general_entity_translation_form_element_state_replace will cause a fatal error "not found", it should be entity_translation_form_element_state_replace to recurse properly.

Further investigation shows that the issue is related to an AJAX callback in the file module. The following code in a form_alter prevents the issue from happening by disabling the cache of the form_state:
$current_path = current_path();
if (strpos($current_path, 'file/ajax') !== false) {
$form_state['no_cache'] = TRUE;
}
Note: this is not a fix and will cause a lot of regression bugs.

mfernea’s picture

I have another opinion on this:
- entity_translation_prepare_element gets adds values to the fields from the source translation. This is done when building the form. The code works just fine.
- when doing an AJAX action, drupal_process_form is called (it starts with the form just fine) but then it gets to $form = drupal_rebuild_form($form_id, $form_state, $form); and the form is being rebuild. In this process entity_translation_field_attach_form doesn't add entity_translation_prepare_element as #process callback for the element. Because of the rebuild. So entity_translation_prepare_element won't be executed anymore. But this is somehow logic as the user might have edited the field already. So why we should prepopulate with data from the source entity? I don't think we should. But still, why don't we have the values that were initially retreived from the source entity? I'm still investigating this.

Kristen Pol’s picture

I tested patch #1 using a multiple value Field Collection field and still get the ajax error when clicking the Remove button.

For patch #2, after applying patch and going to the translate page I get:

Fatal error: Call to undefined function lb_general_entity_translation_form_element_state_replace() in /Users/kristen/Sites/testing/docroot/sites/all/modules/contrib/entity_translation/entity_translation.module on line 1337
loopduplicate’s picture

Regarding #5, I changed the function calls from lb_general_entity_translation_form_element_state_replace() to entity_translation_form_element_state_replace(). Here's an updated patch.

Kristen Pol’s picture

Status: Needs work » Reviewed & tested by the community
+++ b/entity_translation.module
@@ -1314,6 +1321,70 @@ function entity_translation_prepare_element($element, &$form_state) {
+ * ¶

Nitpick - Extra space at end of line.

But, this patch is working for us so marking RTBC.

The last submitted patch, 2: entity-translation-prepopulate_form_state-2339315-2.patch, failed testing.

loopduplicate’s picture

Kristen Pol’s picture

Confirmed nitpick from #7 was fixed in patch in #9. Thanks!

joseph.olstad’s picture

Thanks loopduplicate, great work! I tested patch #6
see related issue:

Sebastien @Actualys’s picture

Any news about a new release with this patch ?
Many thanks

gnucifer’s picture

I just discovered and had a go at this myself (https://www.drupal.org/node/2715091). Would have saved me a lot of pain if I had seen this issue before that. :) I like this approach better than my fix which although more compact was a bit of a hack, I had also missed the issue with "entity" being stored by field collection. The special case for field collection module could be more nicely implemented providing a hook (for field collection module to implement). Will get back with patches for this.

gnucifer’s picture

Will include the hook implementation (the part of the patch removed) in the patch currently being worked on in: https://www.drupal.org/node/1344672

james.williams’s picture

@gnucifer that's a great idea -- I'm looking forward to your patch over on #1344672: Field Collection: Field translation (entity_translation) support.! (Assuming it works ;-) ) Whilst this issue is already RTBC, based on the patch in comments 7 (or 9), I suggest that yours should be considered RTBC instead, assuming the tests pass in a moment.

gnucifer’s picture

@james.williams Thanks! The patch for field collection can now be found here: https://www.drupal.org/node/1344672?page=1#comment-11134159

joseph.olstad’s picture

great work gnucifer, rtbc your patch #14

plach’s picture

Status: Reviewed & tested by the community » Needs work

Committed and pushed #14, thanks!

Not marking fixed yet because we need a follow-up to provide the hook documentation, regardless of what happens in the Field Collection issue.

  • gnucifer authored 5e00730 on 7.x-1.x
    Issue #2339315 by loopduplicate, gnucifer, plopesc, james.williams:...
plach’s picture