Translatable image file is not working unless you also config the image field. Config can get lost anyway.

I'm unable to reproduce this. I followed steps 1-5, but then when I visit each of these paths (I'm using Italian as 2nd language):

/node/1
/it/node/1

I see the separate images for each translation. Same for the edit pages as well.

Am I missing something?

I tried both the latest stable version (8.02) and DEV version (8.0.x-dev).

The problem seems to be that the multilingual configuration of image field requires subfields title and alt to be translatable.
You have to edit the field and specify that all subfields of the image are translatable. But changes to edit different language content will be lost.

The solution of @yobottehg works momentarily

/admin/config/regional/content-language options will overwrite the fields of all types of content every time the form is saved.

The error appears when saving the options of image fields in /admin/config/regional/content-language

I can confirm this is a nasty bug since the image on other translations get overwritten and you lose them. Raised priority.

Maybe is an issue with the form at /admin/config/regional/content-language sending the checkbox values empty so they set the values in the filed config wrong. Every time you save that form you will revert the workaround suggested by @yobottehg for every content type that you have in your site.

I had a chat with @trebormc and I was not able to reproduce what he comments about new or already created nodes. If he is able to reproduce that it should be a different issue.

Was fixed by #2639352: File records, files themselves lost in translation

I'm sorry but that fix happened 9 days ago, we are testing on latest dev and we are able to reproduce.

I just read the test on that commit and it fixes the issue for file fields not for image fields. We can confirm it works for file fields.

I think the workaround proposed give us some clues about something happening with the alt and title subfield in the image. The file field does not have title and alt.

I'll try to write a test similar to the one on that commit but for image fields.

Bringing back the tags

Ah translation .. :)

The IS is still confusing a bit to me, will try to reproduce now too.

Ok, IS is indeed ok, sigh, stupid files :)

Moving to critical, this is data loss.

It is really surprising since imageItem also uses FileFieldItemList as its list class and ImageItem extends fileItem.

Oh man, this one is becoming even funnier in variations. If you set the alt title to translatable on the fields page, but not title, notices are thrown:

Notice: Undefined index: en in Drupal\content_translation\FieldTranslationSynchronizer->synchronizeFields() (line 90 of /home/drupal/drupal-core/core/modules/content_translation/src/FieldTranslationSynchronizer.php).

and the file is gone immediately as well on the source node as well, where otherwhise it's not (yet, but will be after cron runs).

Note: on my setup I didn't have the title field enabled, so I'm not sure if the notice is coming from this property or not.

Here is the patch with a test similar to the one at
#2639352: File records, files themselves lost in translation
but for images.

Added a proposed resolution about having consistent behavior in both forms. I don't know how to implement it.

The proposed solution makes sense. The config entities now have this:

third_party_settings:
  content_translation:
    translation_sync:
      file: file
      alt: '0'
      title: '0'

while it should probably be this:

third_party_settings:
  content_translation:
    translation_sync:
      file: file
      alt: alt
      title: title

I'm wondering if we need to make sure that we don't concentrate on those two properties alone, but make sure that, in case there's another field type out there, 'sub' properties follow the same behavior as well. (although I can't think of one atm, but I'm sure there might be)

- edit - : content translation already has a todo in code to wonder about those dependants (but not issue afaics), so fixing that globally is probably not the main topic here.

The tests were lying a bit, so I changed them so explicitly failed better.
Attached is a fix for the content translation overview page.

Haven't looked in detail yet at the field settings screen but it seems like the dependents javascript doesn't work there, so it might as well be working anyway, will look further now and add additional tests which checks explicitly for the translation_sync settings in the field config entity after saving both screens.

re-uploading my version of the test only patch

This fixes it when saving the field settings form as well (or even saving a field config via the API).

Patch includes:

- new test for saving field settings form
- fix for the javascript on the field settings form
- implements content_translation_field_config_presave() special casing the image field

Some more background re: the mention in the patch about line 116, this is what is in current HEAD already:

  // @todo This should not concern only files.
  if (isset($column_element['#options']['file'])) {
    $dependent_options_settings["settings[{$entity_type_id}][{$bundle}][columns][{$field_name}]"] = array('file');
  }

We should have more generic solution to fix the hardcodeness that's in the patch.
Could probably get rid of hook_entity_type_presave then in the patch.

Ok, I've been looking a bit at this. There be dragons. I've warned you.

* It seems this information is *almost* generic. The image field type has a column_groups annotation that contains information about the properties and how to deal with them.. default values, etc.
* The only thing that's hardcoded is that file requires all others. In content_translation_field_sync_widget(), without explanation or todo or anything.
* contentTranslationDependentOptions seems to be duplicated and defined twice, once in that helper function and then again outside? The one outside has the @todo as you said.

So, what if we add a new key somewhere (e.g. requires_all = TRUE, default to FALSE within the column group) and use that in that form build function and then we can also look this up in the submit and handle it there?

But... I get that this is clearly a bug, but that doesn't explain why the file gets lost? The *file* columns is still marked as translatable? the alt/title enforcement thing seems more a help for users (it doesn't make sense to have translatable files with untranslatable alt/title). What I'd expect to happen is that alt/title gets lost/is synced but not the file? This fix seems to fix that simply by preventing that any syncing is done, which is arguably kind of a workaround?

I spent some time debugging FieldTranslationSynchronizer but I can't make sense of that code. As far as I see, it has no way of actually syncing *columns*, it only syncs the full item:

          // If a synchronized column has changed or has been created from
          // scratch we need to override the full items array for all languages.
          elseif ($created) {
            $values[$langcode][$delta] = $source_items[$delta];
          }

* Why "full items"? Shouldn't this combine translatable new column values from the translation with untranslatable columns from the source?
* Note that source here is actually the new translation. Highly confusing but correct since we're syncing from the new translation to all others.
* There's a line "$created = $created && !isset($old_delta);" above and same for $removed. But $created is initialized to TRUE above and is always TRUE. That combined condition is pointless?
* I'm also missing an else case.. if it's not removed and unchanged, the value has to be kept? Where does that happen? Ah. I think that elseif can't ever be FALSE because then it would go into the if or elseif above.
* It uses oh so many different variables.. $values (used to be $field_values initially), $original_field_values (not consistently renamed), $items, $item,..

So what I get from this is that column synchronisation doesn't work. at all. It only seems to work because when you don't switch out he field, then those hashes are the same and it simply keeps the current values. You can reproduce that by switching out the image when creating a translation. Then the alt text that you enter in the translation is replaced in the source language.

You could also say that the file only gets lost if you actually enter a translation for the alt, if you leave it unchanged, it is kept ;)

So, based on that, the attached patch changes the behavior to work as I'd expect. The file is kept, but the alt text is synchronized. Not what the user would expect when saving the UI, we definitely need to fix that, but based on what's actually configured.

Didn't change the other confusing/weird parts about the code there yet.

Woah, those tests are fun. They showed that I need to handle the case where the delta doesn't exist in the current language. Currently copying the whole item over. I'm not sure if that's the expected behavior or if it shouldn't touch the items and only sync the columns in case the item actually exists. In a way, that would make more sense to me.

I refactored the second failing test to explicitly assert each check, with a useful debug message. Before it did a hundred checks and combined it into a single boolean and just asserted that. Really painful to debug. I had to add check if the column it is checking really is synchronized, it was testing obviously the wrong behavior.

Looking into merging it with the previous patches now.

Ok. Merged the two patches and worked on my suggestions in #22. Note that the interdiff is against the combination of those two patches.

* Introduced a require_all flag for the file column group on ImageItem
* Used that in content_translation_field_sync_widget()
* While trying to unify the duplicated drupalSettings, I noticed two things:
a) they used different form element names, fixed that by making that an optional argument.
b) the drupalSettings in that function were actually never sent to the browser, which is also why this feature didn't work on the field settings form. Apparently each checkbox is separately rendered. Fixed by moving #attached down to the first option. So this now also works on the field settings form.
* Instead of fixing when we save the settings, which I didn't like so much because it means we need to duplicate the code, i implemented the require_all check now directly FieldTranslationSynchronizer. This might be a bit weird at first as the settings are still saved "wrong", but it has a huge advantage: We don't need an update function (and update tests!) for existing images. It just starts working with your existing configuration as expected.
* However, that meant the test didn't work anymore because it explicitly checked that configuration. I removed that part and also removed testing the field settings form because it was kind of pointless. Instead, I enabled the title field as well and I'm now testing both fields, without caring how the settings are stored exactly. We just test that it works in the end.

By the way, the lost in translation move reference in the test is awesome :)

I also noticed a few things that we should/could open follow-ups for:
* The file fields description doesn't support this feature. Seems like it should. We just need to define similar column groups there.
* It's kinda weird that you can enable sync for e.g. title when it's not enabled on the field. But fixing that seems tricky, especially on the settings form (where it would to update based on other settings in the same form)
* I think our test coverage for the default mode (alt and title translatable) is incomplete. We need a test that uploads a new file while translating and ensures that we keep the existing alt/title texts. This is fixed by this patch, and I've updated the unit tests to work accordingly but we have no integration test for this.

So yeah, more elegant solution than mine and allows integration for other field types.

1. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -49,15 +54,17 @@ function content_translation_field_sync_widget(FieldDefinitionInterface $field)
   +      $element[key($options)]['#attached']['drupalSettings']['contentTranslationDependentOptions'] = array('dependent_selectors' =>
   +        array($element_name => $require_all)
   +      );
   

   Mostly a nitpick, but use new style of array with [] instead 'array' ?

2. +++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
   @@ -31,7 +31,8 @@
   + *       "require_all" = TRUE
   

   1. We should document this in content_translation_field_info_alter() and maybe create a change record ?
   2. Also, since this is new and has influence on the code above, I could only see this after clearing caches, so do we need an empty upgrade path just to make sure caches are flushed ?

Thanks for the review.

1. Updated the new/changed and close lines.

2.1: None of the properties were documented so far, I tried to document them all. Might need some improvements on the wording.
2.2: Added an update function. I actually explicitly cleared the cache, it will happen again after the update is finished, but the same is true for the router rebuild above and this makes it kind of self-documenting.

Looks good to me.

1. +++ b/core/modules/content_translation/content_translation.install
   @@ -43,5 +43,12 @@ function content_translation_update_8001() {
    /**
   + * Clear field type plugin caches to make the new require_all flag available.
   + */
   +function content_translation_update_8002() {
   +  \Drupal::service('plugin.manager.field.field_type')->clearCachedDefinitions();
   +}
   +
   +/**
     * @} End of "addtogroup updates-8.0.0-rc".
     */
   

   Need to add this to updates-8.0.x group...

2. +++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
   @@ -31,7 +31,8 @@
   + *       },
   + *       "require_all" = TRUE
   

   require_all what... I think this key name is pretty cryptic. I think improve this label might also help to improve the labels in content_translation_field_sync_widget() too. The thing is that this means if we're translating then we require all - but this setting looks way more generic than just being about translation.

2. Both me and @swentel tried to think about a better key for that, but I'm not sure. Anything non-cryptic would likely be very long. Note that the whole column_groups thing is currently about translations only, while someone could possibly rely on it for a different use case, it is defined by content_translation, so doing that for a purpose that doesn't involve translations seems... problematic. I also don't understand the second part about improving the labels in content_translation_field_sync_widget(), what labels?

@Berdir i meant the variable names in content_translation_field_sync_widget()

Ok, discussed that with @alexpott and we agreed on the longer but more specific key "require_all_groups_for_translation".

Also updated the issue summary.

Yay, thanks for working so hard on this, looks like all concerns are resolved. Would be happy to see this in :)

+++ b/core/modules/content_translation/content_translation.install
@@ -43,12 +43,22 @@ function content_translation_update_8001() {
+ * Clear field type plugin caches to make the new require_all_groups_for_translation flag available.

Hmm, over 80 chars, can be fixed on commit though.

Well update functions are special - must be on a single line. I'm not sure but they might have an exemption from the 80 chars.

A few coding standards cleanups and looking at the coding standards there is no exempt for update functions so fixed it to say what is being fixed rather what it does.

Committed 2f49e8c and pushed to 8.0.x and 8.1.x. Thanks!

1. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -19,11 +19,13 @@
   + *   The name that the element will have, used applying javascript.
   

   "used applying javascript" is a bit vague and sounds incorrect from a grammar point-of-view.

2. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -110,13 +118,9 @@ function _content_translation_form_language_content_settings_form_alter(array &$
   -              // @todo This should not concern only files.
   -              if (isset($column_element['#options']['file'])) {
   -                $dependent_options_settings["settings[{$entity_type_id}][{$bundle}][columns][{$field_name}]"] = array('file');
   -              }
   

   Nice that we get to fix this.

3. +++ b/core/modules/content_translation/src/FieldTranslationSynchronizer.php
   @@ -66,6 +66,17 @@ public function synchronizeFields(ContentEntityInterface $entity, $sync_langcode
   +        // require_all_groups_for_translation flag set, there are no
   +        // untranslatable columns. This is done because the UI adds Javascript
   +        // that disables the other checkboxes, so their values are not saved.
   

   The word that in the first line is superfluous.

Fixed on commit...

diff --git a/core/modules/content_translation/content_translation.admin.inc b/core/modules/content_translation/content_translation.admin.inc
index 4136afb..893fb4e 100644
--- a/core/modules/content_translation/content_translation.admin.inc
+++ b/core/modules/content_translation/content_translation.admin.inc
@@ -20,7 +20,8 @@
  * @param \Drupal\Core\Field\FieldDefinitionInterface $field
  *   A field definition object.
  * @param string $element_name
- *   The name that the element will have, used applying javascript.
+ *   (optional) The element name, which is added to drupalSettings so that
+ *   javascript can manipulate the form element.
  *
  * @return array
  *   A form element to configure field synchronization.
diff --git a/core/modules/content_translation/src/FieldTranslationSynchronizer.php b/core/modules/content_translation/src/FieldTranslationSynchronizer.php
index 33c2cdc..32f6034 100644
--- a/core/modules/content_translation/src/FieldTranslationSynchronizer.php
+++ b/core/modules/content_translation/src/FieldTranslationSynchronizer.php
@@ -67,10 +67,10 @@ public function synchronizeFields(ContentEntityInterface $entity, $sync_langcode
         // single list.
         $groups = array_keys(array_diff($translation_sync, array_filter($translation_sync)));
 
-        // If a group was selected that has the
-        // require_all_groups_for_translation flag set, there are no
-        // untranslatable columns. This is done because the UI adds Javascript
-        // that disables the other checkboxes, so their values are not saved.
+        // If a group was selected has the require_all_groups_for_translation
+        // flag set, there are no untranslatable columns. This is done because
+        // the UI adds Javascript that disables the other checkboxes, so their
+        // values are not saved.
         foreach (array_filter($translation_sync) as $group) {
           if (!empty($column_groups[$group]['require_all_groups_for_translation'])) {
             $groups = [];

Woot, amazing! Thanks all!

In the test, while I think the intent is to upload different image files, what really happens is that different copies of the same image-test.png file are uploaded. This broke #2377747: Incorrect node create validation error when an invalid image is attached to a field and can be fixed there.