Translated field denormalization creates duplicate values

I am going to try writing a test that takes the code from issue summary proves the bug.

@cburschka thanks for the patch. I was working on the test separately. I am uploading a test only patch that proves the problem and then a patch with #5 added. I don't think if fixes the problem but haven't had a chance to look.

The test I should be moved probably to the serialization module and then have test in HAL that extends it and just changes the format.

This bug is exactly why we need test coverage. Which is why we're doing #2135829: [PP-1] EntityResource: translations support. But that's where this was found. So, yay — we're working on the right things!

+++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
@@ -127,9 +128,43 @@ protected function createTranslatedInstance(FieldItemInterface $item, $langcode)
+    foreach ($translated_fields as $field) {
+      $field->setValue([]);
+    }

This is similar to what we do in \Drupal\serialization\Normalizer\FieldableEntityNormalizerTrait::denormalizeFieldData(). The difference is that that operates at the entity-field level, this operates at the field-property level. So this makes sense.

However, the test finds another problem in that denormalizing non-hal json doesn't seem to work with translations at all (not sure if that's a bug).

So let's make the test coverage only work for the hal_json format for now. Ensuring translated entity support works in all formats and all situations is out of scope here. This issue is only about fixing this one bug.

Great work here!

Changing test coverage to be only for hal_json format and changing component to hal.module

All of the test failures I think have to do with the Path field which is computed. So I am pretty sure we should not be executing this new logic for computed fields.

#20:

1. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -42,10 +43,11 @@ public function denormalize($data, $class, $format = NULL, array $context = [])
        // If this field is translatable, we need to create a translated instance.
   -    if (isset($data['lang'])) {
   +    if (isset($data['lang']) && !$field_item->getFieldDefinition()->isComputed()) {
   

   Comment needs to be updated. And actually… wouldn't it make more sense to never denormalize computed fields in the first place? Shouldn't the denormalization process simply ignore them? If they're computed, then they shouldn't be sent?

2. +++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
   @@ -127,9 +129,43 @@ protected function createTranslatedInstance(FieldItemInterface $item, $langcode)
   +    // Get the translated entity, or create it if it does not exist.
   +    $entity_translation = $entity->hasTranslation($langcode) ? $entity->getTranslation($langcode) : $this->createTranslatedEntity($entity, $langcode);
   

   The test coverage only covers the first case: if the translation already exists.

3. +++ b/core/modules/hal/tests/src/Kernel/TranslationNormalizeTest.php
   @@ -0,0 +1,49 @@
   +    $this->installEntitySchema('entity_test_mul_changed');
   +
   +  }
   

   Whitespace nit.

Unfortunately, it's probably not that simple. "computed" doesn't mean "read-only"

D'oh, true. Reminds me of #2392845: Add a trait to standardize handling of computed item lists.

Conclusion: what I wrote in #21.1 is wrong, and so is #20's interdiff, right?

Well this is tricky problem....

#24

I suspect what I should have used there is not ::isEmpty() but rather !count(). As far as I can tell, this returns the list's length without side effects - and even if some field plugin ever overrides it to do stuff, we should be able to assume that its return value will match the new list length.

I think you actually just found a bug with PathItemList. Count and isEmpty should act the same or at least count() should act the same whether you call it before or after a call to isEmpty which it doesn't

See this example from my tester module:

public function testCount() {
    $node = Node::load(1);

    $markup = 'results1: ';
    $markup .=  ($node->get('path')->isEmpty() ? 'isempty': 'no isempty') .  '----';
    $markup .=  (count($node->get('path')) ? 'count': 'no count') .  '----';
    /** @var EntityTypeManagerInterface $manager */
    $manager = \Drupal::service('entity_type.manager');
    $manager->getStorage('node')->resetCache([1]);
    $node = Node::load(1);
    $markup .= '<br>results2: ';
    $markup .=  (count($node->get('path')) ? 'count': 'no count') .  '----';
    $markup .=  ($node->get('path')->isEmpty() ? 'isempty': 'no isempty') .  '----';



    return [
      '#type' => 'markup',
      '#markup' => $markup,
    ];

  }

It shows that calling count() behaves differently after isEmpty() has been called because it calls ensuredLoaded and creates an item. So this is a bug in PathItemList and we can't rely on count() here to tell if it is empty.

UPDATE: I have to step out for a bit so can't file this bug issue. I will when I get back or if someone else does please report back here. Thanks

Here is the issue for the bug described in #28 #2905524: The count() method used on PathFieldItemList behaves differently if isEmpty() has been called.
If I am wrong and that is expected behavior we can just close that issue.

Right, it's totally possible that PathFieldItemList is not entirely correct. It was introduced in June in #2846554: Make the PathItem field type actually computed and auto-load stored aliases. But since it's the first of its kind (a field item list object specifically for a computed field that isn't really computed, but just backed by custom storage), it's totally understandable that there are oversights. A week ago, we already fixed one thing about PathFieldItemList: it needed to also override the ::equals() method (in #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior).

Pinging @Berdir, since he's one of the people who know best how translated entity fields are supposed to work, plus he helped introduce PathFieldItemList. I think we need his guidance here.

This is blocking #2135829: [PP-1] EntityResource: translations support, tagging blocker.

Woot! Looking forward to your patch 🙂

+++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
@@ -2,6 +2,7 @@
 use Symfony\Component\Serializer\Exception\InvalidArgumentException;

@@ -127,9 +128,43 @@ protected function createTranslatedInstance(FieldItemInterface $item, $langcode)
+    // Append an item only if the item list is empty or allows multiple values.
+    if (!count($field) || $item->getFieldDefinition()->getFieldStorageDefinition()->isMultiple()) {
+      return $field->appendItem();
+    }
+    return $field->first();
+  }

This block is confusing to me because we are checking if the field is empty or allows multiple values before appending another item.

Instead of isMultiple() should we also be checking ?

count($field)
 < $item->getFieldDefinition()->getFieldStorageDefinition()->getCardinality()

#36:

since that was causing one of the failures before

:( Do we understand why? Why did it only cause failures for Node entities, and then even only for the hal_json format? How do we ensure that this failure doesn't reappear when #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior is recommitted?

Hm, #28 and #29 seem to suggest this is related to the computed path field behaving incorrectly. That's fortunately being fixed in
#2916300: Use ComputedFieldItemListTrait for the path field type, which is blocked on #2392845: Add a trait to standardize handling of computed item lists.

As far as I can tell, the count() bug described in #28-31 should not require a change to this patch, and the problem in #32 was simply exposing a bug elsewhere (which will need to be fixed separately).

Based on my analysis above, I think you're right.

#38:

This block is confusing to me because we are checking if the field is empty or allows multiple values before appending another item.

Instead of isMultiple() should we also be checking ?

Good question! This is indeed confusing. Let's look at FieldStorageDefinitionInterface for an answer. It contains these:

  /**
   * Returns whether the field can contain multiple items.
   *
   * @return bool
   *   TRUE if the field can contain multiple items, FALSE otherwise.
   */
  public function isMultiple();

  /**
   * Returns the maximum number of items allowed for the field.
   *
   * Possible values are positive integers or
   * FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED.
   *
   * @return int
   *   The field cardinality.
   */
  public function getCardinality();

Which is still a bit confusing. But the actual implementation actually makes it very clear:

  /**
   * {@inheritdoc}
   */
  public function isMultiple() {
    $cardinality = $this->getCardinality();
    return ($cardinality == static::CARDINALITY_UNLIMITED) || ($cardinality > 1);
  }

Which means @tedbow is right, this should be essentially checking

$cardinality = $item->getFieldDefinition()->getFieldStorageDefinition()->getCardinality();
if (!count($field) || $cardinality === FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED || count($field) < $cardinality)) {
…

@Wim Leers thanks for analysis!

I change you suggest code slightly to
if ($cardinality === FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED || count($field) < $cardinality) {

We don't need to check both !count($field) AND count($field) < $cardinality)

Since \Drupal\Core\Field\FieldStorageDefinitionInterface::getCardinality docs state

* Possible values are positive integers or
* FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED.

The only way count($field) < $cardinality) will ever be checked is if $cardinality !== FieldStorageDefinitionInterface::CARDINALITY_UNLIMITED.
Since count() returns back an integer just checking if it is less than $cardinality works.

👌 Even better!

+++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
@@ -134,7 +135,8 @@ protected function createTranslatedInstance(FieldItemInterface $item, $langcode)
     // Append an item only if the item list is empty or allows multiple values.

You forgot to update the comment. Once that's updated, this is RTBC.

Actually, thinking about it — I don't think a comment is necessary!

+++ b/core/modules/hal/src/Normalizer/FieldItemNormalizer.php
@@ -127,9 +129,43 @@ protected function createTranslatedInstance(FieldItemInterface $item, $langcode)
+    // Remove all default values, except for the langcode.
+    $translated_fields = $entity_translation->getTranslatableFields(FALSE);
+    unset($translated_fields['langcode']);
+    foreach ($translated_fields as $field) {
+      $field->setValue([]);
+    }

Sorry for jumping so late on this, but this caught me a bit by surprise.

It seems that if we have to do this, we are working around some other, deeper issue. Looking at the patch and thinking about it a bit, I think that deeper issue is that the field item normalizer somehow has to decide where in the field item list the field item it is denormalizing belongs. That should not really concern a field item normalizer, however. Instead, shouldn't the field item list normalizer decide which field items go where - i.e. which field items go into which translation in which order - and then delegate to the field item normalizers that don't have to do any magic?

I.e. turn \Drupal\hal\Normalizer\FieldNormalizer::normalizeFieldItems into something like (untested):

  protected function normalizeFieldItems($field_items, $format, $context) {
    $field_items_by_langcode = [];
    $normalized_field_items = [];
    if (!$field_items->isEmpty()) {
      foreach ($field_items as $field_item) {
        if ($field_definition->isTranslatable() && isset($field_item['lang'])) {
          $langcode = $field_item['lang'];
          unset($field_item['lang']);
          $field_items_by_langcode[$langcode] = $field_item;
        }
        else {
          $field_items_by_langcode[$default_langcode] = $field_item;
        }
      }

      foreach ($field_items_by_langcode as $langcode => $langcode_field_items) {
        $entity_translation = $entity->getTranslation($langcode);
        $context['items'] = $entity_translation->get($field_name);

        foreach ($langcode_field_items as $field_item) {
          $normalized_field_items[] = $this->serializer->normalize($field_item, $format, $context);
        }

    }
    return $normalized_field_items;
  }

That way this whole HAL-specific langcode handling is completely opaque to the field item itself and is fully handled on the list level.

Thoughts?

I think that's a really good point! We'll look into that. Thanks for pointing that out! 👍😀

Here's a start on what I was thinking. It passes the test added in this issue at least, not sure if/what it breaks anything else. I probably won't be able to drive this home (in case it's viable), but I thought I should at least provide some starting point here, since I knocked back an already RTBC patch. The patch also contains two "@TODO"'s, so it's definitely work-in-progress.

Actually, the second @TODO in my patch kind of stuck in my head, and I now think that it make even make more sense to move the entire translation-related logic to ContentEntityNormalizer.

The received data, that we want to denormalize has the following structure:

entity -> field_a -> delta_0-en
entity -> field_a -> delta_1-fr
entity -> field_a -> delta_2-fr
entity -> field_a -> delta_3-en

And the way we structure our entity objects in memory requires to restructure that into:

entity -> translation-en -> field_a -> delta_0
entity -> translation-en -> field_a -> delta_3
entity -> translation-fr -> field_a -> delta_1
entity -> translation-fr -> field_a -> delta_2

(Of course the deltas will be recreated so that both translations then have 0 and 1 as deltas, but I wanted to make the link to original structure clear.)

Now since we denormalize from the outside in, we need to decide at which level in the structure we to the translation setup. The patch in #42 does this at the field item level, and my patch in #50 does it at the field item list level, but looking at the above, it seems sensible to actually do this at the entity-level itself and then just pass the correct field item list values to the correct translation.

Thoughts?

#51 I think you're right!

However, let's not forget that this is HAL-specific, and we're working in the hal.module component. In the HAL normalization, each delta has a lang key-value pair, which is why it's possible at all to send all translations simultaneously. That may not be possible in other normalizations.
That's fine, I just wanted us all to remember that.

I went to look at the HAL spec, and unfortunately … I found exactly zero mentions of how to deal with translations/languages: https://tools.ietf.org/html/draft-kelly-json-hal-08. So it seems this is a Drupalism we added to HAL :( It was introduced in #1924220: Support serialization in hal+json. And #1931976: Support deserialization for hal+json (needs more language handling tests) was supposed to add denormalization support, which it did. But then it still needed a test to prove that denormalizing normalized data actually worked (which it obviously does not), which never happened. Otherwise we wouldn't have this issue.

Having written #53, I realized something. #51 means that the normalizer is no longer dealing with the data/value object it is given, and then passing the next level on to the (de)serializer service again (which is the primary design element of the Symfony Serialization component). It means we're bypassing/working around/not using/not conforming to the very architecture we signed up to use.

Then my eye fell on this bit in the IS of #1931976: Support deserialization for hal+json (needs more language handling tests):

Because the language is identified in the field item data, we must be able to translate in the FieldItemNormalizer. However, because the language can only be managed from the entity object, the entity object must be passed in. To do this, create the 'target_instance' for both the field and field item in the calling denormalizer (e.g. entity gets the next field object and passes it into FieldNormalzier using $context['target_instance']). Then, the FieldItemNormalizer can traverse the property path back up to the entity and create a new path to a translated field item.

This was written 4.5 years ago, by one of the architects of D8's REST API. IOW: it was already decided back then to not conform to the very architecture we signed up to use. 😱

The more skeletons I encounter in the closest, the more I wonder how D8 shipped with a "stable" REST module. 🤐

So … while I think #51 is a great idea, I fear it may actually out-of-scope refactoring. #42 was merely fixing the existing code, and the existing code may have architectural flaws that only become clear now, but it's architecture introduced by the WSCCI architects in #1931976: Support deserialization for hal+json (needs more language handling tests) (that's the issue that introduced \Drupal\hal\Normalizer\FieldItemNormalizer::denormalize()). Therefore I think it may be better to go with the patch in #42 after all, because it doesn't change the existing architecture as much, which means there's less risk.

After all, this is merely a small blocker/bug discovered while we're working in #2135829: [PP-1] EntityResource: translations support to add test coverage for the existing hal_json format's (supposed) support for translations. So I think it's best to minimize change, and once that test coverage is in, it'll be far less risky to make significant architectural changes like the one you proposed in #51, but also the one you proposed in #47/#50.

Sorry for the comment rollercoaster :( I first tentatively agreed with you (in #49), then agreed with you even more (in #53), and now end up not agreeing at all, after doing more research. I hope all this made sense. I know it's tricky/confusing, but that's part of making kinda-working-but-mostly-broken things work again, unfortunately! I *do* appreciate your reviews on this enormously. I'm looking forward to reading your reply, perhaps you'll still convince me of #51 after all :P

@tstoeckler thanks for the reviews! @Wim Leers thanks for looking into the background.

I agree with @Wim Leers in #54. I think it makes sense to change as little as possible. There is obviously a bug in the denormalization process but from #1931976: Support deserialization for hal+json (needs more language handling tests) it seems the original intention should was to happen on the field item level. It seems we should not change this.

Custom code could also be overriding the FieldNormalizer for some small change and if they looked at the current code they would interpret that Translation does not need to be handled on this level. I think we should only change this if it is not possible to handle translations on the FieldItem level.

If we can get it working with minimal changes we can move onto [#32135829]

I've tried to plow through this issue and seem to get some of the problems, though it seems to me like a true rollercoaster and am a bit confused on how this issue should proceed? We're having a project that needs REST API in combination with translations badly and seems to be stuck on #2135829: [PP-1] EntityResource: translations support and / or this issue. As I've come to understand, currently CRUD operations on translated nodes is a no-go because the normalization process in this part is questionable? Does this still needs work? Or is this a "works as intended"?

I'd be happy to help in this matter (if I can), since we desperately need REST API to work with content translations. But at this point I'm a bit confused about the status and this hasn't been updated for a while. I'l also post a followup on #2135829: [PP-1] EntityResource: translations support, since that has been stale as well for quite some time.

Edit: After digging around some more, I noticed that there are some conceptual problems to be considered. At this point it's questionable if HAL+JSON should actually handle any POST / PATCH requests, as noted in #1931976: Support deserialization for hal+json (needs more language handling tests) which refers to #1929632: Decide how to handle POST/PATCH when using HAL. The last mentioned issue seems to be stale for years. Also, the mentioned message boards in them are really old. I'm starting to think this has hit a brick wall on whether or not HAL should support POST / PATCH?

#2135829: [PP-1] EntityResource: translations support is the general translation support issue for core REST, for all formats: json, xml and hal_json.

However, while working on that, we discovered a serious bug in the HAL normalizer that blocks progress in the aforementioned issue: that's what #2904423: Translated field denormalization creates duplicate values is for.

Thanks for the response. Alright, #2135829: [PP-1] EntityResource: translations support is for general translation support, does that mean that that issue is postponed only because of the hal implementation and therefor all other formats? Before making this into a really long discussion and going to much off-topic, would you mind discussing this with me on Slack? :-)

Oh, and yes, #1931976: Support deserialization for hal+json (needs more language handling tests) is the weird/incomplete solution that got committed but then left open for test coverage for years… and I had never even seen #1929632: Decide how to handle POST/PATCH when using HAL — how the hell did you find that!? 😮 👏

Regarding why this issue exists, see #2135829-106: [PP-1] EntityResource: translations support and preceding comments. @cburschka discovered a bug, and hence filed this issue. This issue indeed affects only the HAL implementation, but then gets in the way of uniformly testing translation support across all formats. We want to have the translation test coverage work across all formats, and therefore this bug needs to be fixed first.

Pretty easy, that issue was mentioned in #1931976: Support deserialization for hal+json (needs more language handling tests) in it's description. 😅

Alright, I now understand that this issue was created during development of #2135829: [PP-1] EntityResource: translations support and that that issue needs this fixed first to be able to test all formats. This got stalled though and I'm not entirely sure why. Is this still open on conceptual problems that have to be thought over? Or is this postponed by other issues? Or is this simply ready to be developed? If so, are any of the patches in this issue still relevant? (sorry for being dense, I'm trying to get a clear view on this 😅)

As far as my two cents are worth; tests seem to pass nicely and I've tried reproducing the original reproduction steps in the description. Without the patch I indeed see the duplicate values, after applying the patch, I do not get the duplicate values. So that seems to do the trick.

Disclaimer; I haven't looked at the contents of the patch.

I found this issue when denormalizing content_translation_source fields on nodes. The node and its translation are present in the HAL JSON data and this gets denormalized, but then the content_translation_source field will contain the default value [und] and the value from REST [nl in my case]. As the field only accepts 1 value, the REST data is discarded.

When applying patch #63 on D8.6.1 the content_translation_source field contains the correct values. But I get an error saving the denormalized entity: "Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'default_langcode' cannot be null: INSERT INTO {node_field_data} (nid, vid, type, langcode, status, title, uid, created, changed, promote, sticky, default_langcode, revision_translation_affected, content_translation_source, content_translation_outdated"

This is because patch #63 introduces the function createTranslatedEntity() which unsets all translatable field values. The default_langcode is not processed to refill these field values because it is used to determine the language to use to create the entity in its base language in \Drupal\hal\Normalizer\ContentEntityNormalizer::denormalize and is then discarded.

I solved the problem of #66 by not unsetting $data[$default_langcode_key] in the ContentEntityNormalizer. This way the REST data in the default_langcode is applied to each translation of the entity. The attached patch is based on patch #63 and only adds the removal of the usetting of the $data[$default_langcode_key] in the ContentEntityNormalizer.

For my case I have had to get a working solution for a project.

I have cross tested the patch https://www.drupal.org/files/issues/2018-07-25/2135829-120.patch from issue #2135829: [PP-1] EntityResource: translations support with #67.

Here is a reroll against 8.6.10.

Patch ported to 8.8.x version

Not sure if one report is enough to get it over the line of RTBC, but I can report that applying this patch beautifully fixed a number of issues with Default Content Deploy - issues with translation of metatags and creation/revision dates being improperly imported.

It also still applies cleanly to Drupal 9.2.

At Jarltech we can't use default_content_deploy without the patch in #73.

I set this long time issue to RTBC to get it reviewed by a core maintainer.

The patch in #72 is missing the tests from #63. We shouldn't be fixing bugs without a test that proves the bug is fixed and ensures we don't break it again.

I re-added the tests to the latest patch.

The test from #63 has been added and the tests are green again.

The hal module has moved out of Drupal Core and into a Contrib Module.
Moving this issue to the Contrib Module queue.

Attaching patch for contrib module.