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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
332.42 KB

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.

yched’s picture

Status: Needs work » Needs review
FileSize
13.85 KB

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.

yched’s picture

Status: Needs work » Needs review
FileSize
5.6 KB
14.98 KB

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

yched’s picture

Slightly better. But fids still don't match.

plach’s picture

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.

yched’s picture

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

plach’s picture

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

yched’s picture

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

plach’s picture

Ok, although not immediately

yched’s picture

Issue tags: +Field API NG blocker

Tagging

yched’s picture

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

plach’s picture

Assigned: Unassigned » plach

Working on this.

plach’s picture

Assigned: plach » Unassigned
Status: Needs work » Needs review
FileSize
13.42 KB
11.77 KB

This should pass tests.

yched’s picture

Wooohoooo ! Thanks a lot @plach !

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

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Looks good indeed.

Berdir’s picture

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

yched’s picture

#19: true, here we are.

yched’s picture

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

YesCT’s picture

Issue summary: View changes

template and follow-up -yesct

YesCT’s picture

Issue summary: View changes

found one I think might be blocked by this

YesCT’s picture

Issue summary: View changes

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

YesCT’s picture

Issue summary: View changes

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

yched’s picture

Further filled the summary.

YesCT’s picture

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.

YesCT’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b3f2fb1 and pushed to 8.x. Thanks!

yched’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
683 bytes

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

alexpott’s picture

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.

Anonymous’s picture

Issue summary: View changes

Filled motivation & API changes