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...

This one is tagged with feature freeze but has no patch yet. @plach, did you have something started, or should anyone jump in?

Here is a first draft. Tests missing.

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 ;-)

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.

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.

rerolled

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

Updated issue summary.

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.

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.

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

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/

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

I also updated the issue summary.

This looks great, only remark are 2 small typos

typo

typo, 'we'

RTBC anyone?

I'm testing this. but first had to reroll.

git apply --index et-sync-1807692-73.patch
error: patch failed: core/lib/Drupal/Core/Entity/EntityBCDecorator.php:75
error: core/lib/Drupal/Core/Entity/EntityBCDecorator.php: patch does not apply

there were no conflicts.

Contains
http://drupal.org/coding-standards/docs#files

\Drupal
http://drupal.org/coding-standards/docs#namespaces

http://drupal.org/node/325974 does need some updating but I think the bit about no php doc on getInfo() and setUp() is right.

no problem here. I'm just wondering what the unshift is for.

no problem here, just a question. it's not reordering the fields in the entity, is it? is this changing the order the translations are done?
It gets the fid from the index (shifted delta) but stores it in the [$langcode][delta]

Or perhaps this is the key to the sync'ing. Without the reordering the syncing would not be needed. And this test is making sure, things are sync'd by the fid?

That sounds strange. The default language was the source language. Source means something different here?

was one of the files removed from the original language? ... Or is this checking the translation to see if the original value for the translation that was not done is dropped? (only 2 of the three were translated)

This is just translating the remaining file? (only 2 were translated before)

the langcode on the third image, that was added in the language (not the default language), has a langcode of the langcode? why didn't the other's that were translated get that langcode overridden too?

http://drupal.org/coding-standards/docs#hookimpl

Implements hook_form_FORM_ID_alter() for field_ui_field_edit_form().

does this php check each field setting to see if it should by sync'd? I think I'm confusing that a field can be checked to be sync'd and then there is also a setting to say what part of the field (columns?) gets sync'd. Oh, if it's set, it's set to the columns that should be sync'd! "This functionality is provided by defining a 'translation_sync'
+ * key in the field instance settings, holding an array of column names to be
+ * synchronized."

Maybe re-write the comment to match the order in the condition.

$entity->isNew() is clearly if it's a "new entity".

"no information about the translation being saved" is that... ? empty($sync_langcode) and also !$original_langcode

why is count translations being tested for < 2? I would think that "if we have no translations" would be count($translations) < 1

Oh yeah, it counts the language of the entity too. http://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData%...

but why the && ?

I guess I would suggest:

But how do you know if there is more than one translation, if that is the translation being changed and needs the original_langcode?

Maybe the test for having enough information about the translation needs to come later, comparing something like the next block: the original/unchanged values with the current values?

Really? seems like there might be.

I admit, I dont know what NG stands for and google was less than helpful. I thought maybe if I searched for EntityNG... http://api.drupal.org/api/drupal/core!lib!Drupal!Core!Entity!EntityNG.php/8

comment seemed to imply that empty fields were not translatable... which was just a little odd wording.

I started with Sync when possible. There is nothing to synchronize when the field is empty, if synchronization is not turned on for the field, or it's not translatable. but that's double negatives. So I suggest:

Use the source of the translation as the sync source?

You are checking if a translation is being created by checking if the $original_langcode arg was set? I think that means that the first condition in the function checking if we can even do sync'ing cannot know if we "have enough information about a translation".

What are we looping through here? overall, we are doing each field (that it makes sense to sync). are the items each language's value of the field? or is it taking into account the cardinality of the field, so it's the sync source language field's values?

Sorry, that's as far as I got... I didn't actually test it yet.

There is an interdiff with just the code style changes if that is all that is really needed.

Oh, I think in the tests and the code, maybe with some better variable names, things might be easier to understand. source is tricky as this is kind of dealing with translations, which have sources (or original values) and also with the sync, which has a source. Also, values and items were confusing. What to do if anything probably depends on the target audience of the comments.

I know the condition is wrong still logically... just want to see.

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 ?

+ // 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/ :)

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

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?

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.

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 :)

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.

Rerolled

The last submitted patch, et-sync-1807692-59.patch, failed testing.

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

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

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

More substantial stuff :

"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 ?

"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 ?

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 ?

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.

screen shots before the patch

A.

B.

opps. missed some files.

updated previous comment to include them.

@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.

follow up
for 2 #1909202: Use modals in operations column of language settings config page
for 3 #1909212: warn when sync enabled on field with previous values, they are not sync'd until one is changed
for 4 #1909218: add (all languages) hint to synchronized fields on translation add/edit form

Here is a reroll.

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

yched, thanks for the thorough review.

Once we hear back from plach, and follow-ups are opened, then this sound ok to proceed with.

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" :)

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.

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.

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.

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

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

@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?

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.

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.

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 :)

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

postponed #1909218: add (all languages) hint to synchronized fields on translation add/edit form on this. it will needs to be looked at again after this lands.

in general this code is very well documented, and is even beautiful in some places.

some of the following are nits, and I'm happy to do them now, or after commit. documenting them here so I dont forget them.
I dont think any of these are blocking.
I did not (yet) test it manually. When I do, I'll check the hidden labels on the fields, but the code looked to add the necessary accessibility things. (or maybe someone will beat me to it)

-------------

should this be max, or source + unchanged?

spurious? as in bogus?

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)?

Oh... it can be neither...

http://drupal.org/node/1354#types says to use "int"

add int to function declaration for $delta.

setUp should be public
http://drupal.org/node/325974
(in the past this was not generally done though)

shifted one position ahead (remove of).

http://drupal.org/node/325974 says setUp should be public.

its -> it's (or: it is)

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

what does proxied indicate?

add a similar for ... field_ui_field_edit_form ?

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.

wow! quick turn around. glad some of that was helpful. I'm going to do manual testing now.

@plach

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

I thought that doc block was changed lines in the patch...

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:

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!

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.