Introduce a column synchronization capability and use it to translate alt and titles through the image field widget

This probably can be fully taken from D7, see http://drupalcode.org/project/entity_translation.git/blob/refs/heads/7.x... - http://drupalcode.org/project/entity_translation.git/blob/refs/heads/7.x...

Yes please! Alt and title are used in (views) slideshows as overlaying text and it makes *a lot* of sense to have these translated in order to display the overlayed text in the proper language. So, providing an easy way to translate these texts is key and should be in core.

Of course there is this case where the images included in the slideshow might already include text. These cases call for a separate image per language, but that is OT here and IMO an edge case that has an easy solution.

I'm planning to post a patch for this during the upcoming weekend. @bforchhammer will help us with code reviews and coding, if needed.

Here is a first draft. Tests missing.

+++ b/core/modules/translation_entity/translation_entity.module
@@ -613,6 +624,139 @@ function translation_entity_form_field_ui_field_edit_form_alter(array &$form, ar
+  // current entity, or we haveno contextual information about the translation

typo

1. +++ b/core/modules/translation_entity/translation_entity.module
   @@ -574,12 +574,23 @@ function translation_entity_field_extra_fields() {
   +      '#states' => array('visible' => array(':input[name="field[translatable]"]' => array('checked' => TRUE))),
   

   This doesn't seem to work when there's existing data, because then the checkbox is replaced with textual information, e.g. "This field has data in existing content. [Enable translation]".
   There should also be a validation function in case JavaScript is not available.

2. +++ b/core/modules/translation_entity/translation_entity.module
   @@ -613,6 +624,139 @@ function translation_entity_form_field_ui_field_edit_form_alter(array &$form, ar
   +/**
   + * Performs field column synchronization.
   + */
   +function translation_entity_sync(EntityInterface $entity) {
   

   Missing @param.

3. +++ b/core/modules/translation_entity/translation_entity.module
   @@ -613,6 +624,139 @@ function translation_entity_form_field_ui_field_edit_form_alter(array &$form, ar
   +  // current entity, or we haveno contextual information about the translation
   +  // being saved, there is nothing to synchronize.
   

   - missing space: "haveno"
   - add a "then" after the comma?

4. +++ b/core/modules/translation_entity/translation_entity.module
   @@ -613,6 +624,139 @@ function translation_entity_form_field_ui_field_edit_form_alter(array &$form, ar
   +      $langcode = $original_langcode ?: $source_langcode;
   

   The "?:" looks rather strange ;-)

Performed some code clean-up. Could not test it as head is broken atm.

I suppressed the state on the checbox for now. We can restore it once the migration code has been dumped.

Note: the "Synchronize translations" checkbox would also be good to have in the configuration/operation column on the new settings page that is being added in #1810386: Create workflow to setup multilingual for entity types, bundles and fields.

Yes, once we have the modal :)

Ok, here is test coverage. This should be more or less ready to go, hopefully we will be able to remove those ugly request attributes once we figure out how to deal with entity translations properly in #1810370: Entity Translation API improvements.

Any feedback here?

rerolled

#13: et-sync-1807692-13.patch queued for re-testing.

I don't think this needs an accessibility review or manual testing: it's only exposing a checkbox at UI level. And we have test coverage for the implented functionality.

Updated issue summary remaining tasks.

I updated the issue summary: I don't see a great value in copying/pasting generic portions of handbook pages in every issue summary. I'd say tt's better to keep it slim and focused.

Updated issue summary.

Sure, but some of the stuff that was in the issue summary previously was not relevant to this issue and IMO could mislead new contributors :)

Putting test only patches with the fix-and-tests patch in the issue queue for the test bot, is just for when there is a bug the patch is fixing. (right?)

Correct. A failing test-only patch indicates some missing code in core, but new features are expected to be missing :)

Ok, cool.

Bringing on D8MI sprint board.

#13: et-sync-1807692-13.patch queued for re-testing.

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -608,6 +608,162 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
+function translation_entity_sync(EntityInterface $entity, $sync_langcode, $original_langcode = FALSE) {

Good inline code comments in the function itself, but I'm having a hard time figuring out the big picture of what the function actually does overall. Could we expand on "Performs field column synchronization" in the func's phpdoc, that's a bit cryptic.

Also - I might vey well be wrong, but it seems the behavior will only fire on form submission ? Not on programmatic saves ?

28 days to next Drupal core point release.

I wanted to just jump in here to question why this issue is seeking to synchronize alt & title text in images.

Generally with a screen reader it is best to just stick with the alt text. Screen readers may actually read both the alt text & title to the user and if they are identical it really doesn't help.

As stated here:

The title attribute, by definition, can be used to provide advisory information. It should:

NOT provide vital information or information necessary for accessibility.
NOT provide the same information as is available in text or alternative text.

http://webaim.org/articles/gonewild/

I think there's a misunderstanding here: this issue is about synchronizing the image accross translations, while leaving the possibility to enter different alt and title per language. They still are totally independent from one another.

Updated issue summary.

Updated issue summary

Here is a reroll to match the BC decorator stuff and improving PHP docs as per @yched request:

4051f83 Issue #1807692: Fixed BC decorator.
7fecbcc Issue #1807692: Improved PHP docs.

I also updated the issue summary.

The BC fix has now its own issue: #1880926: Fatal error when retrieving $entity->original on a BC entity. Go and RTBC it!

@yched:

Any other concern?

This looks great, only remark are 2 small typos

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSyncTest.phpundefined
@@ -0,0 +1,259 @@
+   * Tests entity field synchrnonization.

typo

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -634,6 +634,169 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
+              // we language we fall back to the new source value.

typo, 'we'

Thanks, typos fixed.

@yched:

Sorry, I forgot:

Also - I might vey well be wrong, but it seems the behavior will only fire on form submission ? Not on programmatic saves ?

Nope, the synchronization happens on field_attach_presave(). If you are referring to the request attributes stuff, it's a temporary solution. I am planning to find a way to pass along the "active" translation, however for now this works also on programmatic saves if you properly fill-in the request attributes.

RTBC anyone?

Rerolled

The reroll looks good, it just removed the hunk that was necessary to make tests pass and that had been moved to #1880926: Fatal error when retrieving $entity->original on a BC entity, which has been committed.

OK, I understand the goal better now, thanks ! Wow, that's a nasty/complex feature to implement :-/

So, yeah, using drupal_container()->get('request')->attributes to set info and get it back in other parts of the code looks like an unholy hack - globals without globals :-). What are the possible ways of getting rid of this ?

There is some hairy logic in translation_entity_sync() to "recognize" and track items between old an new values and figure out which item got reordered where - the part from "// Make sure we can detect any change in the source items." to the end of the function.
It could really use an "algorithmic overview" comment to explain the logic IMO - even though the "step by step" comments that are present are good, I still have a hard time understanding the overall logic and figuring out if it's valid.
- For example, the case where there are several synced columns sounds like it might be complex ? And that case is not covered by the tests. And ouch, testing that case is not going to be trivial. Maybe splitting that logic to a helper func that works with raw arrays and not $field, $instance and $entities, would help make it more unit testable.
- Also, what if several items have the same value for a 'synced' column (e.g same image) ? how do we tell which is which ? I mean, the image might be the same but the alt text and their translations might differ, isn't that a problem ?

+            // If a synchronized column has changed we need to override the full
+            // items array for all languages.
+            elseif ($created) {

Looks like the comment doesn't match the logic : "has changed" vs if ($created))

+            $value = $source_items[$delta][$column];
+            $change_map[$column][$value]['new'] = $delta;

Are we 10%% sure that $value is safe to use as an array key ? I don't remember offhand what can and cannot be a valid array key...

+ // The module ensures we start from the beginning after reaching the
+ // maximum allowed delta.
+ $index = ($delta + 1) % $this->cardinality;

Minor : s/The module/The modulo operator/ :)

Good suggestions, I'll try to implement them as soon as I can.

hm, that's a pretty complex feature and problem space. I must say I'm not in favour of the syncing solution, but it seems to be the most feasible one.

Thinking about how it could be done with EntityNG, we could support adding 'translatable' flags on the field value level there while by default they are all translatable, and then implement the synching logic inside of the entity field objects - i.e. if translatable is set and FALSE for a property being changed, do something like

$entity = $this->getRoot();
$entity->$field[$delta]->$name = $value;

$field, $delta and $name could be determined via $this->getPropertyPath(). Of course we'd have to apply it to all translation languages.

That way, we'd have full support for 'translatable' in the API while the storage layer would only have to take care of field translatability.

Maybe, it would make sense to do it inside EntityNG?
Update: I'm not sure it really makes sense in EntityNG as the field is not really translated anymore. It's kind of being shared while some values are translated - how would this feature be exposed in the UI?

@fago:

Honestly I thought converting this to NG would be pretty straightforward, once all the entities are converted.

I just spent other 3 hours to improve the documentation and write a brand new unit test. Would you consider letting this feature in as-is and talking about the NG conversion later? This could happen also after feature completion, while I'm pretty sure I won't have time to rewrite the code here from scratch since I have more urgent stuff to work on.

This should address #43. Hopefully this can go in now, otherwise I'm afraid we won't have this functionality in D8 core.

Tests were passing here because I had ET enabled. The attached patch uses DrupalUnitTestBase instead of UnitTestBase.

Would you consider letting this feature in as-is and talking about the NG conversion later?

Sure, it's not my intent to block this in any way - I was just trying to figure out the best way to implement this quite complex feature.

So am I right, that this can be enabled in addition to the general translation option, i.e. you have the choice to do both?

Then, while thinking more about it, I don't think we should have the sync-ing feature in the storage layer as we need to be able to switch between full translation and sync-ing anyway - so it can't just be magically the translation of the field. So the approach taken in the patch makes totally sense for me :-)

So am I right, that this can be enabled in addition to the general translation option, i.e. you have the choice to do both?

Yes, you can choose to have you fields vary independently by language or have part of their values synchronized across languages. There's a new checkbox in the field settings.

So the approach taken in the patch makes totally sense for me :-)

Glad to hear that :)

So, I guess we are just missing @yched's ok here, right?

Any other concern/suggestion?

Sorry, very little drupal time lately :-/
Will try to make some time for this, but if it's not tomorrow it won't be before another week.

#49: et-sync-1807692-49.patch queued for re-testing.

Rerolled

Sorry, wrong one.

#58: et-sync-1807692-59.patch queued for re-testing.

#58: et-sync-1807692-59.patch queued for re-testing.

@yched:

Even a "yes" or "no" would be welcome :)

Rerolled after #1807776: Support both simple and editorial workflows for translating entities went in.

#63: et-sync-1807692-63.patch queued for re-testing.

Sorry for the delay, the last fortnight has been pretty dense in my "non-drupal" mode :-/. Slowly getting back at the core queue.

Minor / nitpicky stuff first :

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSyncUnitTest.phpundefined
@@ -0,0 +1,176 @@
+  protected $unchangedFieldValues;
+
+
+  public static $modules = array('language', 'translation_entity');

Double empty line (& after getInfo() too)

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSyncUnitTest.phpundefined
@@ -0,0 +1,176 @@
+    $this->cardinality = mt_rand(2, 5);

Can we avoid random values, use, like, 4 instead ?

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSyncUnitTest.phpundefined
@@ -0,0 +1,176 @@
+    foreach ($this->langcodes as $langcode) {
+      for ($delta = 0; $delta < $this->cardinality; $delta++) {
+        foreach ($this->columns as $column) {
+          $sync = in_array($column, $this->synchronized) && $langcode != $this->langcodes[0];
+          $value = $sync ? $this->unchangedFieldValues[$this->langcodes[0]][$delta][$column] : $langcode . '-' . $delta . '-' . $column; //$this->randomName();
+          $this->unchangedFieldValues[$langcode][$delta][$column] = $value;
+        }
+      }
+    }

A bit cryptic.
"Set up an intitial set of values in the correct state, i.e. with "synchrnozed" values being equal." ?

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -766,6 +766,209 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
+ *   The unchanged items ti be used to detect changes.

s/ti/to

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSyncUnitTest.phpundefined
@@ -0,0 +1,176 @@
+    $sync_langcode = $this->langcodes[mt_rand(0, count($this->langcodes) - 1)];

Likewise, pick one language instead of random ?

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -766,6 +766,209 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
+function translation_entity_sync_field(&$field_values, $unchanged_items, $sync_langcode, $translations, $columns) {

Yay @ the new unit testable helper func, & extensive comments. I'd just suggest prefixing the func name with an underscore so that it's not mistaken for an API entry point.

-3 days to next Drupal core point release.

More substantial stuff :

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -766,6 +766,209 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
+ * available languages. For this to properly work the column values must be
+ * integer or strings. The synchronized column values are assumed to be unique

"The column values must be integer or strings" - because they're used as array keys, IIRC ?
What about NULL ?
What about decimal / floats ?
What about LONGTEXT ?
Also, where / how do we document that the 'synchronize' feature only works on integer / text values ?

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -766,6 +766,209 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
+ * integer or strings. The synchronized column values are assumed to be unique
+ * among the various items, this is necessary to detect and replay changes in
+ * the order. They are also assumed to change simultanously, so that a change in

"The synchronized column values are assumed to be unique among the various items".
OK, but in lots of actual cases they won't be, what happens then ?

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -766,6 +766,209 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
+ * the order. They are also assumed to change simultanously, so that a change in
+ * any column indicates that all of them have changed.

Yeah, so that's the "how does this work when there are several synced columns ?" part.
Not sure what are the practical implications of this though ?

+++ b/core/modules/translation_entity/translation_entity.moduleundefined
@@ -766,6 +766,209 @@ function translation_entity_form_field_ui_field_settings_form_alter(array &$form
+        // (...) A changed value will be interpreted as a new
+        // value, in fact it did not exist before.

This means that any change in the value of any of the synced columns is seen as "some delta disappeared, that gets removed in all other languages, and a new one appeared, that gets copied over to all languages" ?

Meaning, if I change the image (value for fid column is changed) but not the alt / title texts, then the alt / title texts in other languages (that were possibly translated) will be overwritten with the ones of the language I made the change in ?
Is that really what we want ?

All in all, major kudos for all the work and dedication that were put in this.
I have to admit I'm still scared at the complexity of the feature, and not fully convinced that a) it can be achieved 100% bullet-proof rather than "best effort", and b) that this "best effort" will be "good enough" rather than "unpredictable in practice".
It's not directly in my queue and I won't be in charge of handling followups or bugs ;-), so I won't be blocking this if everyone wants it, though.

@yched:

Thanks for the review(s)! I'll try to address them ASAP. A couple of answers meanwhile:

"The column values must be integer or strings" - because they're used as array keys, IIRC ?

Yes, this is a limitation we need to document (and probably better enforce). Our main use case are fids so this perfectly works for now.

"The synchronized column values are assumed to be unique among the various items".
OK, but in lots of actual cases they won't be, what happens then ?

One possible solution is that we dected this situation and we bail out without syncing anything. I'd like to explore the possibility to support multiple values, but I'm not sure it's possible without complicating the algorithm too much.

Yeah, so that's the "how does this work when there are several synced columns ?" part.
Not sure what are the practical implications of this though ?

Well, we could refactor the algorithm to combine the column values so that we have a unique key (a primary key). This would allow us to support any combination of the synced columns, provided that at least one value differs from the others.

This means that any change in the value of any of the synced columns is seen as "some delta disappeared, that gets removed in all other languages, and a new one appeared, that gets copied over to all languages" ? [...] Is that really what we want ?

Yes, I am afraid we cannot get around this: a change in a synced column cannot be intepreted in any other way but "a new value is here". We might not propagate it to all the translations or leave the unsynced columns empty, but in the case of alt/titles a language fallback looks like the most reasonable behavior.

I have to admit I'm still scared at the complexity of the feature, and not fully convinced that a) it can be achieved 100% bullet-proof rather than "best effort", and b) that this "best effort" will be "good enough" rather than "unpredictable in practice".

I agree we probably cannot achieve a 100% bullet-proof behavior. What I think we can get is something that works in the 80% of use cases and does not break things in the remaining 20%. The code here has been almost straightforwardly ported from contrib ET (D7), which has around 6000 installations, and no bug about it has been reported so far. Hence I'm pretty confident we can have something that can actually be useful.

Here is a new patch, I think it's considerably more solid than the previous one: it should now support multiple elements having the same values for the synchronized columns and the ability to detect changes when just one synced column changes.

This should also take care of #66.

As a bonus all the synchronization logic has been moved to an autoloadable class, to reduce memory footprint. Not sure we need to register it into the DIC: would it make sense for the sync logic to be swappable?

Anyway, the attached interdiff shows only the changes in the sync logic and the related tests. The actual one wouldn't be very useful.

@YesCT:

The attached patch incorporates the proposed textual changes, but it collapses them into the checkbox description: the label felt too long in the original proposal.

About #72:

@1: Yes, this should be solved by making translatability an instance setting.
@2: Also @bforchammer propoed something similar above: I think it would be nice if we could have all these settings in a modal dialog, as originally proposed by Bojhan.
@3: I think a warning message after enabling the setting should be enough. Do you wish to provide a text or a patch?
@4: Looks a bit verbose to me: maybe just "Synchronized"? I'd prefer to address this in a follow-up to be sure to have a UX feedback and not hold this patch on it.

It would really important to get this RTBC before feature completion.

update remaining tasks, added need to decide on follow-ups

Here is a reroll.

#79: et-sync-1807692-79.patch queued for re-testing.

My remarks on the latest code. Mostly nitpicks / code organisation remarks.

+   * must be integer or strings.

Nitpick : both singular or both plural ?

core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSyncUnitTest.php
+      // Continuous field values: all of values are equal.

Minor: missing word or extra 'of'.

class FieldTranslationSynchronizer:
+   * @param $sync_langcode
+   *   The language of the translation whose values should be used as source for
+   *   synchronization.
+   * @param $original_langcode
+   *   (optional) If a new translation is being created, this should be the
+   *   language code of the original values. Defaults to FALSE.
+   */
+  public function synchronizeEntity(EntityInterface $entity, $sync_langcode, $original_langcode = FALSE) {

Missing type hints for the langcode args.

Also, "$original_langcode is either a string or FALSE" looks sloppy, that's precisely why NULL exists.
But more generally, the method is always called with a value for $original_langcode, so why is it optional ?
[Edit : oh ok, FALSE is because the value comes from EntityTranslation::getSourceLangcode(), which currently returns "string or FALSE".
I'd be inclined to change that to NULL upstream, but then I guess that's not for this patch.]

FieldTranslationSynchronizer:synchronizeField()
+          $old_delta = -1;
+          $new_delta = -1;
...
+            $created = $created && $old_delta < 0;
+            $removed = $removed && $new_delta < 0;

Looks a bit hackish, relying on the fact that "real" deltas are positive, and then interpret "< 0" for "not found".
Initializing $old_delta & $new_delta to NULL and checking isset($old_delta) / isset($new_delta) should work.

class FieldTranslationSynchronizer:
+  protected function itemIdentifier(array $items, $delta, array $columns) {

Could it be named itemHash() instead ?
What this does is compute a hash for (the values of the synced cols of) an item, not an id. What makes the patch complex is precisely that field values don't have ids that we could use to track them, but this does not add ids, just works around the absence thereof :-).

FieldTranslationSynchronizer::itemIdentifier() :
+          $values[] = $this->validateValue($items[$delta][$column]);
...
+    return !empty($values) ? implode('.', $values) : FALSE;

Minor : implode($empty_array) == '', so the last line could be just return implode('.', $values); ?
+ I'd tend to think validateValue() as a separate method adds an unnecessary mental indirection, could be inlined within itemIdentifier() now.
+ I guess we could use a real hash, like md5(serialize(array_intersect($item, $synced_cols)) and therefore support more than number & strings, but maybe that's not worth the runtime overhead ?

FieldTranslationSynchronizer::validateValue()
+    if (!is_numeric($value) && !is_string($value)) {
+      throw new \InvalidArgumentException('A synchronized value can only be a number or a string.');

is_numeric(NULL) == FALSE, is_string(NULL) == FALSE
That's annoying because a numeric db column could have a value of NULL. (for image fields that would mean the field is empty and is not stored, but for multi-column fields that could happen). It seems allowing NULL would be doable ?
implode('.', array(1, NULL, 2)) == '1..2', works.

+    // Set contextual information that can be reused during the storage phase.
+    $attributes = drupal_container()->get('request')->attributes;
+    $attributes->set('working_langcode', $form_langcode);
+    $attributes->set('source_langcode', $source_langcode);

This is still an ugly hack :-(. Not sure what are the plans to get rid of that ?
@todo + followup issue ?

FieldTranslationSynchronizer :
+  protected function loadUnchanged($entity_type, $id) {
+    $controller = $this->entityManager->getStorageController($entity_type);
+    $controller->resetCache(array($id));
+    $result = $controller->load(array($id));
+    return reset($result);
+  }

Interesting.
In the previous patch, this was translation_entity_sync() function calling entity_load_uncached().
So now that the code moved to a class, we avoid calling functional stuff like entity_load_uncached() within a method and go for the OO equivalent ?

Does that mean that entity_load() too is considered bad practice in OO code, and that any class willing to load an entity needs to get an injected EntityManager and do $this->entityManager->getStorageController($entity_type)->load(array($id)); instead ? Ew :-/
Definitely not a debate for this thread, but IMO that would deserve a spin off discussion to establish the practice.
At the very worst, this loadUnchanged() method should be provided by EntityStorageController, not recoded in FieldTranslationSynchronizer and in each other class that might have a use for entity_load_uncached().

For this patch, I'd be inclined to keep FieldTranslationSynchronizer::synchronizeEntity() calling entity_load_uncached() for now...

Regarding the behavior itself :

FieldTranslationSynchronizer::synchronizeField()
+            $field_values[$langcode][$delta] = $source_items[$delta];
...
+            $field_values[$langcode][$new_delta] = $item;

As I mentioned earlier, I still don't get why we overwrite the whole item in the target languages, instead of only the values of the synced columns, which would be easily doable ?
The current code does : if I edit the node in german and change an image, the english and french alt & title texts that I previously entered will be overwritten with the german values, and I need to edit the other languages to switch them back.
I would think we want : if there are corresponding values in english and french, only synchronize the columns that are supposed to be synchronized and keep the unsynchronized ones untouched ?
In other words, the patch introduces the notion of "synchronized columns", but those are in fact only triggers for the sync behavior, and all columns are in fact synchronized.

But @plach says the current behavior comes straight from D7 ET contrib, where it received no major complaints, so I'm fine with committing the current and possibly discuss / revisit in a followup.

Thanks for the review, I think I agree with almost everything. I'll be a bit busy this weekend. Hope to be able to squeeze some time out and get this done...

This should address more or less everything in #83. Could not launch tests so I hope this is still good.

Some answers:

+ I'd tend to think validateValue() as a separate method adds an unnecessary mental indirection, could be inlined within itemIdentifier() now.
+ I guess we could use a real hash, like md5(serialize(array_intersect($item, $synced_cols)) and therefore support more than number & strings, but maybe that's not worth the runtime overhead ?

I originally thought about real hashing too, but I dismissed this solution because of performance concerns. After rethinking about it I went for a hybrid approach: a hash is computed only for non-string and non-int values.

This is still an ugly hack :-(. Not sure what are the plans to get rid of that ?
@todo + followup issue ?

In #1810370: Entity Translation API improvements we agreed to try and introduce an EntityLanguageDecorator that should allow us to remove this hack. Since this would be the only example in core right now, I think it would be fine to fix it there. Added a @todo about this.

Does that mean that entity_load() too is considered bad practice in OO code, and that any class willing to load an entity needs to get an injected EntityManager and do $this->entityManager->getStorageController($entity_type)->load(array($id)); instead ? Ew :-/
Definitely not a debate for this thread, but IMO that would deserve a spin off discussion to establish the practice.

I think we already implicitly dediced to go this way when we introduced the DIC: no point in using procedural functions which are in the global scope and make everything harder to unit-test. That said nothing would prevent us from somehow improving the entity manager DX. Agreed on having this dicussion elsewhere. Reverted the change for now, since loadUnchanged() totally belongs to the storage controller.

[...]
I would think we want : if there are corresponding values in english and french, only synchronize the columns that are supposed to be synchronized and keep the unsynchronized ones untouched ?

Well, the point is that if you change a synchronized value, supposedly you are invalidating also the other ones: think of a node with an image of an apple (english alt = "An apple", french alt = "Une pomme"), then you replace it with the photo of a kitten (I cannot image why you would do this, but let's suppose for a moment :) and enter an english alt the says "A cute kitten". For me it makes far more sense to have a french translation where you have an untranslated english alt rather than having the photo of kitten with a alt saying "Une pomme" :)

No idea of wtf just happened :(

After rethinking about it I went for a hybrid approach: a hash is computed only for non-string and non-int values

Yup, great idea !
Although, my bad, md5() is actually forbidden in core now even for non security-related uses, hash('sha256') is the standard. See http://drupal.org/node/845876 and the recent #1884826: Regression - replace md5 in Block module calls with sha2 hashes.

elseif ($old_delta >= 0 && $new_delta >= 0) {

Shouldn't that be changed to isset($old_delta) && isset($new_delta) too ?

md5() is actually forbidden in core now even for non security-related uses, hash('sha256') is the standard

Indeed, fixed. Thanks!

Thanks !
RTBC if green.

This should be more like it :)

:-)

OK I don't have time for a full review but I don't see any discussion here about converting these to fields on the file entity instead of serialized data in the field storage. The problem here is really with the image field for me, and introducing this to work around that doesn't feel good.

@catch:

This has a far broader scope than just image fields: it will work with any field having item values needing to be synchronized across entity translations. Image fields are just a prominent example. This API would make sense even if we had no use case in core. Please, reconsider the issue status.

I don't think we can realistically make file entities fieldable in core one week before feature freeze, since this would require a dedicated UI, unless there already has been significant work on this that I totally missed. Which could be very well possible, btw :)

Retitling to be more clear about the goal of this issue.

Tagging for D8MI.

@catch:

This is the only issue I was able to find where the discussion you are missing might have happened: #376287-16: Make files fieldable.

I have no idea how we could get this done in the scope of the multilingual initiative. My approach with this was: if the Media initiative succeeds and we have fieldable files in core, wonderful, otherwise we need an alternative solution. Hence I came up with the current proposal which meanwhile matured in something that can totally live without image fields exploiting it. AAMOF if you review the patch you'll discover that only 2 lines of it, leaving alone a dedicated test, is dealing specifically with images/files.

To sum up I think this is a cool feature to exploit in contrib and leaving out from core will mostly kill it since I probably won't maintain a D8 version of contrib ET just for it, although people upgrading from D7 might expect to find it still there.

Talked to plach about this in irc. I asked for use-case other than image fields (which should really just be a reference). Plach mentioned a map field with a label - pointing out you probably wouldn't want that to be a reference to an entity with a text field.

Moving back to CNR - I still haven't reviewed the code here, only worried about the use cases at this point.

Actually we have at least another use case in core: the link field, which has an optional Title textual column.

Also, the displayed label for a referenced image field might vary by usage? Having it in the file entity would not account for that.

Looks like plenty of other use cases to back this up, back to RTBC then.

The UI elements introduced in this patch don't cut it for me. 'Synchronize translations' is just too cryptic of a setting for the end user. It's misleading actually as its suggests that the strings will be synchronized. In the case of the Image field, as far of the site builder is concerned, it would be better to write something like [ ] Use different image for all translations. If we do this right, we may be able to get rid of the help text too. Right now, it seems like the current patch exposes implementation details in the UI.

@Dries:

I must say I completely agree :)

I spent some time thinking about an alternative when writing the first patches but nothing really user-friendly came to my mind and then forgot about it. Now I spent some more time thinking about this: would something like the screenshot below work better?

yched's comments were addressed. also follow-ups opened.

I think that's an improvement, but how about this idea?

Instead of making the field type module decide which columns are 'sync' worthy, and implicitly assuming that everything else is "textual", and then needing to add an extra form element to the field settings form, we instead:

- Let field modules specify "column groups". For example, image_field_info() could add the following:

'column_groups' => array(
  array('title' => 'File', 'columns' => array('fid', 'width', 'height')),
  array('title' => 'Alternative text', 'columns' => array('alt')),
  array('title' => 'Title', 'columns' => array('title')),
),

- Have translation_entity.module enhance the admin/config/regional/content-language form (where the administrator decides which fields to make translatable), such that when a field that has 'column_groups' is set to translatable, that a subrow appears for each column group, initially starting off checked, but the admin could uncheck as desired. Basically, the same pattern as how a bundle is expanded into its fields, just applied again to a field expanding to its column groups.

@plach: what do you think?

I don't see any discussion here about converting these to fields on the file entity instead of serialized data in the field storage.

Yeah, this would have been great, but it didn't happen. The challenge with making file entities fieldable is that it's only useful if you define bundles. You don't necessarily want an "alt" field on all files, just on the image bundle. But implementing administrator-customizable bundles that properly relate to mime types turns out to be non trivial. #1260050: Provide administrative UI for adding/removing file types is an issue that's been open over a year in the File entity queue. Looks like it's gonna need more time in contrib, but would be awesome to get into D9.

Also, the displayed label for a referenced image field might vary by usage? Having it in the file entity would not account for that.

Also a good point, and there's been discussion in various places (sorry, don't have links handy) about needing a "File usage" entity in addition to file entities to support that cleanly.

That's just background for folks interested in helping out with fieldable files. I also agree with #99 that the Link field provides another use case and several contrib field types do as well.

Had a short call with plach. This looks like a good idea, I agree with Dries that the word sync is misleading. I really like effulgentsia his proposal, I think thats exactly what people will expect!

The ability to toggle translatability on properties, this should come with some sensible defaults and should be displayed on both the i18n setup page as one the field settings of an individual field. Otherwise we might end up to require way too much setup work for fields with properties.

Ok, I will work on this ASAP. I'd just want to point out that the items order when we have a multiple field is not covered by the current proposal. Bojhan suggested to make it explicit by having an "Order" checkbox, but since this would force us to totally revisit the sync algorithm and that it would be more stuff for the user to configure, I'd suggest that that we just make it implicit and sync also the order when there is at least one synchronized column.

I'd suggest that that we just make it implicit and sync also the order when there is at least one synchronized column.

That makes complete sense to me. I can't think of a use case where it makes sense to vary item order by language, but require syncing of some part of each item. (says a person who's never operated a multilingual site)

FYI, just to avoid confusion I just figured that would be a way to handle such a use case - if the use case does not exist we should definitely not support it.

@effulgentsia your last remark, is quite funny. I challenge you to just try it sometime :)

Here is a patch implementing #104 + #107.

As suggested by @Bojhan in the content language settings we have a slightly different behavior wrt fields: instead of making all columns translatable, when making a field translatable smart defaults are applied. In the screenshot only the textual columns are made translatable by default (thus triggering column synchronization). When making a field untranslatable column settings are reverted to their defaults. The same settings with their defaults are available on the instance settings page when the field is translatable.

Since the behavior implemented now might not satisfy every possible use case, above all because of #107, I decided to make the field sync logic swappable and registered the implementing class into the DIC.

Moreover while fixing tests I noticed that the image field sync test was not extending the ET base test, hence I updated it accordingly.

The end result is that the interdiff is pretty useless :)

I didn't foresee to work on this so much, I'd really like to see this going in now :)

I haven't reviewed the code in-depth, or manually tested the syncing functionality, but from the little I've seen, I think this is really good. I hope @YesCT or someone else more familiar with this issue than I am can RTBC it in the next 24 hours or so. Otherwise, I'll try to detail review it then.

Great review, thanks!

should this be max, or source + unchanged?

Max is enough, since we just need to ensure we reach the end of both source and unchanged items arrays. Anyway this is not needed when populating the change map, so moved it down and cleaned-up the related code.

spurious? as in bogus?

Yep :)

why && with created or removed, they are hard coded to be TRUE.
should there be an else in here? can something be both created (not old) and removed (not new)?

There was a lot of cruft here left out from previous iterations. Cleaned-up.

add int to function declaration for $delta.

Nope, type-hinting works only with classes.

what does this test, that the other tests do not?

It tests the sync behavior when multiple items have the same values to ensure they are retained and moved around if needed.

what does proxied indicate?

That it is the actual hook implementation. The proxy one is in the .module file. This helps reducing the memory footprint.

add a similar for ... field_ui_field_edit_form ?

That one is not touched by this patch, so it would be out of scope.

Opened #1919322: entity_load_unchanged() should be part of the storage controller to address #82-#85.

suggestion, i) expect people will figure this out. ii) dont show fields are translatable if they are not enabled.
is this image specific? if so, go with i)

Yes, this is image-specific: we could have a novice follow-up to add a tiny bit of state magic in the image.module and hide the Title and Alt groups when the related form items are disabled.

stuff gets weird when the file is translatable but the alt and title are not.

Yep, this is why I originally went for a less flexible approach: less stuff to figure out for the user. Also here we might have a follow-up and add some JS to make the dependent groups always checked (and readonly) when the master group is checked. E.g. when I check "File", "Alt" and "Title" are automatically checked and cannot be unchecked.

Yep, probably. Already created #1919322: entity_load_unchanged() should be part of the storage controller for:

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/FieldTranslationSynchronizer.php
@@ -0,0 +1,191 @@
+    // @todo Use the entity storage controller directly to avoid accessing the

Updated issue summary.

The old issue summary linked to #89/90 and I was very scared because that UI needed a ton of work. But very stoked to see the screenshots at #110! That's exactly what I expected.

I did not test this patch myself, but it's clear that YesCT did a bang-up job of this already, and both she and yched have given it a thorough code review. I also think that being able to translate fields, but not being able to translate attributes of fields, is a WTF so am going to re-classify this as a major task.

Committed and pushed to 8.x. Thanks!

Wonderful, thanks!

Removing from sprint, thanks everyone for the help!

@yched:

Opened #1935762: Remove entity_load_unchanged() call from FieldTranslationSynchronizer::synchronizeFields() so we can dicuss the DX implications of injecting the entity manager.

Updated issue summary, added follow-up section and some follow-ups

In #1992138-52: Helper issue for "field types as TypedData Plugins" (the "unify Field API with Entity/FieldNG" issue), we could use some help with failures in EntityTranslationSyncImageTest :-)

...removed the "Related issues" section from the issue summary and moved all issues listed there as child issues of this one. That was a pain because we currently do not have this: #2130889: Allow adding child/follow-up issues directly from the parent issue and converting related issues to children.

...

@YesCT, @klonos (and @plach) : opened #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition for the issue about config schema for 'translation_sync'.