Updated: Comment #21

Problem/Motivation

The "field synchronization" feature provided by the FieldTranslationSynchronizer class is written with the "old / BC" syntax for accessing field values. It works in HEAD because calling code takes care of passing entities wrapped in the BC layer.

#1969728: Implement Field API "field types" as TypedData Plugins rewrites a fair amount of the code flow surrounding FieldTranslationSynchronizer, and triggers hard to debug test fails related to the BC layer.

At this point in the cycle, all fieldable entity types in core implement EntityNG, aside from the 'test_entity' used in some Field API tests, that do not leverage "field synchronization", that is fully tested separately.
So, it makes more sense to move that code to work on EntityNG, which will need to happen anyway, and state that it will not work on the one entity type that is not NG yet, rather than spending time in debugging the BC layer.

Proposed resolution

Rewrite field sync on top of NG entities

#7 - #11
using item arrays in ::synchronizeFields() and setting the updated values back onto the entity afterwards,
allows keeping synchronizeItems() easily unit testable by working on plain arrays

User interface changes

No.

API changes

The "field synchronization" will not work on the 'test_entity' entities still used by some Field API tests.

This is blocking these:

Just Related

Files: 
CommentFileSizeAuthor
#26 minor-followup-2013837.patch683 bytesyched
PASSED: [[SimpleTest]]: [MySQL] 58,566 pass(es).
[ View ]
#20 field-sync-NG-2013837-20.patch13.43 KByched
PASSED: [[SimpleTest]]: [MySQL] 57,537 pass(es).
[ View ]
#20 interdiff.txt1.86 KByched
#16 field-sync-NG-2013837-16.interdiff.do_not_test.patch11.77 KBplach
#16 field-sync-NG-2013837-16.patch13.42 KBplach
PASSED: [[SimpleTest]]: [MySQL] 57,393 pass(es).
[ View ]
#6 field-sync-NG-2013837-6.patch15.45 KByched
FAILED: [[SimpleTest]]: [MySQL] 55,274 pass(es), 6 fail(s), and 3 exception(s).
[ View ]
#6 interdiff.txt1.42 KByched
#5 field-sync-NG-2013837-5.patch14.98 KByched
FAILED: [[SimpleTest]]: [MySQL] 55,311 pass(es), 9 fail(s), and 3 exception(s).
[ View ]
#5 interdiff.txt5.6 KByched
#3 field_sync-NG-2013837-1.patch13.85 KByched
FAILED: [[SimpleTest]]: [MySQL] 56,038 pass(es), 325 fail(s), and 76 exception(s).
[ View ]
#1 field_sync-NG-2013837-1.patch332.42 KByched
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new332.42 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Initial patch - naïve, possibly/probably buggy rewrite.
Fails, but I couldn't really investigate why so far.

Status:Needs review» Needs work

The last submitted patch, field_sync-NG-2013837-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new13.85 KB
FAILED: [[SimpleTest]]: [MySQL] 56,038 pass(es), 325 fail(s), and 76 exception(s).
[ View ]

er - forgot to merge HEAD before I rolled the patch

Status:Needs review» Needs work

The last submitted patch, field_sync-NG-2013837-1.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.6 KB
new14.98 KB
FAILED: [[SimpleTest]]: [MySQL] 55,311 pass(es), 9 fail(s), and 3 exception(s).
[ View ]

A couple fixes, at least synchronization runs.
Still fails, fids don't match.

StatusFileSize
new1.42 KB
new15.45 KB
FAILED: [[SimpleTest]]: [MySQL] 55,274 pass(es), 6 fail(s), and 3 exception(s).
[ View ]

Slightly better. But fids still don't match.

It seems this is making ::synchronizeItems() heavily depend on field NG internals: would it be possible to switch to item arrays in ::synchronizeFields() and set the updated values back onto the entity afterwards? I suspect this might make things easier.

@plach: yeah, I've been hesitating to go that way.
On the plus side it would keep synchronizeItems() easily unit testable by working on plain arrays. On the minus side, it's doing back and forth translation to an old-style array format that doesn't actually exist anywhere else anymore.
Both approaches seem valid to me, your call :-)

Status:Needs review» Needs work

The last submitted patch, field-sync-NG-2013837-6.patch, failed testing.

Both approaches seem valid to me, your call :-)

No strong preferences but actually this might make things more resuable, although right now I cannot imagine code needing to re-use this stuff ;)

You're probably right, writing the algorithm with arrays sounds right.
Can you take over from there ? :-)

Ok, although not immediately

Issue tags:+Field API NG blocker

Tagging

Issue tags:+Entity Field API

From @Berdir:

@yched: Note that all issues also need the Entity Field API tag to show up at http://entity.worldempire.ch/conversions

Assigned:Unassigned» plach

Working on this.

Assigned:plach» Unassigned
Status:Needs work» Needs review
StatusFileSize
new13.42 KB
PASSED: [[SimpleTest]]: [MySQL] 57,393 pass(es).
[ View ]
new11.77 KB

This should pass tests.

Wooohoooo ! Thanks a lot @plach !

Looks good to me, but I guess we need to find someone to RTBC that for us :-)

Status:Needs review» Reviewed & tested by the community

Looks good indeed.

+1 to RTBC.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/FieldTranslationSynchronizer.phpundefined
@@ -52,17 +60,14 @@ public function synchronizeFields(EntityInterface $entity, $sync_langcode, $orig
-      if (!empty($entity->{$field_name}) && !empty($instance['settings']['translation_sync']) && field_is_translatable($entity_type, $field)) {
+      if (!$entity->{$field_name}->isEmpty() && !empty($instance['settings']['translation_sync']) && field_is_translatable($entity_type, $field)) {

Unrelated, but is there already an issue for $field->isTranslatable()? If not then we should totally create one :)

Code uses offsetGet($delta) instead of [$delta] which is a bit longer which is a bit longer but possibly easier to follow. get($field_name) vs. {$field_name} is a bit inconsistent and I think I prefer get() with dynamic field names as {} stuff looks ugly. Downside is that get($field_name)[$delta] doesn't work but we don't use that anyway.

Maybe make that a bit more consistent before committing?

StatusFileSize
new1.86 KB
new13.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,537 pass(es).
[ View ]

#19: true, here we are.

is there already an issue for $field->isTranslatable()? If not then we should totally create one :)

True - dunno if there are plans already, but I opened #2018685: Remove field_is_translatable().

Issue summary:View changes

template and follow-up -yesct

Issue summary:View changes

found one I think might be blocked by this

Issue summary:View changes

added some detail about resolution. needs motivation spelled out. -yesct

Issue summary:View changes

I think it relates to the issue where we introduced translation sync.

Further filled the summary.

I looked at the code. Coding style wise looks great.
It seems to be removing BC stuff, which is great.
I'll update the issue summary, but it could use some help from someone more familiar with the entity field api stuff to make sure the motivation section is accurate.

Also, it looks like no api changes. So I'll put that in the summary too. (but correct it if I'm wrong)

[edit: yched updated those! :) ]

Small question:

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSyncImageTest.phpundefined
@@ -180,30 +180,29 @@ function testImageFieldSync() {
     // Add back an item for the dropped value and perform synchronization again.
-    // @todo Actually we would need to reset the contextual information to test
-    //   an update, but there is no entity field class for image fields yet,
-    //   hence field translation update does not work properly for those.

Does this mean there is a entity field class for image fields?

What issue added that?

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/Tests/EntityTranslationSyncImageTest.phpundefined
@@ -122,10 +122,10 @@ function testImageFieldSync() {
       // the entity.
       $item = array(
         'fid' => $fid,
-        'alt' => $this->randomName(),
-        'title' => $this->randomName(),
+        'alt' => $default_langcode . '_' . $fid . '_' . $this->randomName(),
+        'title' => $default_langcode . '_' . $fid . '_' . $this->randomName(),
       );

this is done a couple times, is it a nice improvement to tests to help make the output of the tests easier to understand? Looks like a good idea.

Question just for my curiosity (not finding anything wrong).

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/FieldTranslationSynchronizer.phpundefined
@@ -9,6 +9,8 @@
+use Drupal\Core\Entity\Field\FieldInterface;

phpstorm told me this is not used.

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/FieldTranslationSynchronizer.phpundefined
@@ -52,17 +60,14 @@ public function synchronizeFields(EntityInterface $entity, $sync_langcode, $orig
     // @todo Use Entity Field API to retrieve field definitions.
     $instances = field_info_instances($entity_type, $entity->bundle());
     foreach ($instances as $field_name => $instance) {
-      $field = field_info_field($field_name);
+      $field = $instance->getField();

or is that the class used for this getField()?

Status:Reviewed & tested by the community» Fixed

Committed b3f2fb1 and pushed to 8.x. Thanks!

Status:Fixed» Reviewed & tested by the community
StatusFileSize
new683 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,566 pass(es).
[ View ]

Sorry for my "minor followup" spree - @YesCT was right in #24, needless "use" statement.

Status:Reviewed & tested by the community» Fixed

Committed 8810170 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Filled motivation & API changes