Problem/Motivation

The HAL and core Entity Reference field item normalizers will discard data passed to constructValue if an entity is referenced by its UUID. The biggest impact of this is that normalizing then denormalizing an entity with an Image field will discard the alt, title, width, and height properties.

Proposed resolution

We should accept valid properties in data passed to constructValue for these normalizers, so that users can normalize and denormalize the same structure without data loss.

Remaining tasks

Review the patch.

User interface changes

None.

API changes

None.

Data model changes

None.

Original issue summary

While working on the test coverage for #1927648-494: Allow creation of file entities from binary data via REST requests.3, I discovered that denormalizing file fields in HAL explicitly omits any values I send for a @FieldType=file field.

\Drupal\hal\Normalizer\EntityReferenceItemNormalizer inherits \Drupal\hal\Normalizer\FieldItemNormalizer::denormalize(), which does $field_item->setValue($this->constructValue($data, $context));. $data here does contain 'description' => 'something'.

Then we go into constructValue() … which EntityReferenceItemNormalizer overrides to this:

  protected function constructValue($data, $context) {
    $field_item = $context['target_instance'];
    $field_definition = $field_item->getFieldDefinition();
    $target_type = $field_definition->getSetting('target_type');
    $id = $this->entityResolver->resolve($this, $data, $target_type);
    if (isset($id)) {
      return ['target_id' => $id];
    }
    return NULL;
  }

… in other words: if the entity that the data is referencing exists, then we will set one property and one property only: target_id. Which means that the display and description that FileItem extends EntityReferenceItem is adding will never get set. Because EntityReferenceItemNormalizer doesn't work for any subclasses. Same for ImageItem extends FileItem extends ReferenceItem: its alt, title, width and height properties are impossible to set.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Berdir’s picture

Kind of expected that, that's why I mentioned we should test that :)

That's why file_entity has http://cgit.drupalcode.org/file_entity/tree/src/Normalizer/FileItemNorma... for a long time, but it's fairly ugly.

Berdir’s picture

That said, I don't think this needs to block testing in the other issue, we simply can't test setting/getting the description yet.

Wim Leers’s picture

That said, I don't think this needs to block testing in the other issue, we simply can't test setting/getting the description yet.

Agreed, and that's what I did there! :)

+++ b/core/modules/rest/tests/src/Functional/FileUploadResourceTestBase.php
@@ -0,0 +1,637 @@
+    // @todo Remove this early return in https://www.drupal.org/project/drupal/issues/2935738.
+    if (static::$format === 'hal_json') {
+      return;
+    }
+    $this->assertSame([[
+      'target_id' => '1',
+      'display' => NULL,
+      'description' => "The most fascinating file ever!",
+    ]], EntityTest::load(2)->get('field_rest_file_test')->getValue());

.

samuel.mortenson’s picture

I ran into something similar when just trying to normalize and denormalize an existing entity, but with the "json" format.

If you install the Standard profile, then create an Article with an image uploaded and alt text filled in, you should be able to replicate this locally (I'm using Drush's PHP console for this):

$node = node_load(1);
[...]
$serializer = \Drupal::service('serializer');
[...]
$node->field_image->alt
=> "Alternative text you filled in"
$node->field_image->target_id
=> "1"
$serializer->denormalize($serializer->normalize($node, 'json'), get_class($node))->field_image->alt
=> null
$serializer->denormalize($serializer->normalize($node, 'json'), get_class($node))->field_image->target_id
=> "1"
Wim Leers’s picture

Hm, so perhaps this is not limited to HAL's entity reference field normalizer.

The IS says it's caused by hal's constructValue() override:

 protected function constructValue($data, $context) {
    $field_item = $context['target_instance'];
    $field_definition = $field_item->getFieldDefinition();
    $target_type = $field_definition->getSetting('target_type');
    $id = $this->entityResolver->resolve($this, $data, $target_type);
    if (isset($id)) {
      return ['target_id' => $id];
    }
    return NULL;
  }

And serialization's looks different:

  protected function constructValue($data, $context) {
    if (isset($data['target_uuid'])) {
      /** @var \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem $field_item */
      $field_item = $context['target_instance'];
      if (empty($data['target_uuid'])) {
        throw new InvalidArgumentException(sprintf('If provided "target_uuid" cannot be empty for field "%s".', $data['target_type'], $data['target_uuid'], $field_item->getName()));
      }
      $target_type = $field_item->getFieldDefinition()->getSetting('target_type');
      if (!empty($data['target_type']) && $target_type !== $data['target_type']) {
        throw new UnexpectedValueException(sprintf('The field "%s" property "target_type" must be set to "%s" or omitted.', $field_item->getFieldDefinition()->getName(), $target_type));
      }
      if ($entity = $this->entityRepository->loadEntityByUuid($target_type, $data['target_uuid'])) {
        return ['target_id' => $entity->id()];
      }
      else {
        // Unable to load entity by uuid.
        throw new InvalidArgumentException(sprintf('No "%s" entity found with UUID "%s" for field "%s".', $data['target_type'], $data['target_uuid'], $field_item->getName()));
      }
    }
    return parent::constructValue($data, $context);
  }

In the example above, you would be sending target_uuid. Could you unset($normalized['field_image']['target_uuid']); and try again?

samuel.mortenson’s picture

@wim-leers Normalizing the Node, then unsetting target_uuid, then denormalizing properly set the field_image's alt text. Nice!

Wim Leers’s picture

Title: HAL's EntityReferenceFieldItemNormalizer::denormalize() only keeps 'target_id' property, discards all other received properties » Entity reference field subclasses (such as file and image fields) lose the non-default properties upon denormalization, in both hal_json and json
Component: hal.module » serialization.module
Issue tags: +Needs issue summary update

Yay indeed, but that means both have bugs, yet in different ways… 🤦‍♂️

I think it makes sense to fix both in the same/single issue, because half the effort is the test coverage, and the test coverage can be shared. The only thing that's going to be different is the fixes!

kpv’s picture

For text fields you can set both
{ "field_text": {"value": "Dramallama"}, }
and
{ "field_text": [{"value": "Dramallama"}], }
when setting data for a request.

For file fields you should always pass it as an array, e.g.:
{ "field_file": [{"target_id": 42}], }

(Found when implementing #1927648-546: Allow creation of file entities from binary data via REST requests)

Wim Leers’s picture

#9: yep, there's lots of weirdness in there :( Thanks for explicitly reporting another example!

samuel.mortenson’s picture

Status: Active » Needs review
FileSize
1.56 KB

This may be overly simplistic but I found that merging in the data retained alt text (and other column) values. Would like to see if approach makes sense before writing tests.

Status: Needs review » Needs work

The last submitted patch, 11: 2935738-11.patch, failed testing. View results

samuel.mortenson’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
2.75 KB

Added tests, made it so that only properties defined in the field item are set.

Edit: Should say that I fixed tests, I did not add new tests yet.

samuel.mortenson’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
FileSize
4.54 KB
2.63 KB

Added test coverage, removed nitpick. Also updated the issue summary.

Berdir’s picture

Status: Needs review » Needs work

We also have a @todo in \Drupal\Tests\rest\Functional\FileUploadResourceTestBase::testPostFileUpload() waiting for this to be resolved. If this indeed is enough to fix it, then we should be able to remove that todo and complete the test to make sure we can deal with a file/image field for both normalization and denormalization now.

samuel.mortenson’s picture

Version: 8.4.x-dev » 8.6.x-dev
Status: Needs work » Needs review
FileSize
5.36 KB
681 bytes

@Berdir I removed the todo and the early return, and it looks like tests pass! Let's see what the test bot thinks.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Great, we have proof that the properties that class FileItem extends EntityReferenceItem add work as expected! We have strict integration test coverage thanks to #1927648: Allow creation of file entities from binary data via REST requests, which is also the issue that originally triggered the creation of this issue.

Since that works, we can reasonably assume that class ImageItem extends FileItem also works.

For once, an API-First normalization issue is simple! Let's do this! 🤘

alexpott’s picture

Crediting @Berdir and @Wim Leers for reviews.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed a46c461 and pushed to 8.6.x. Thanks!

  • alexpott committed a46c461 on 8.6.x
    Issue #2935738 by samuel.mortenson, Wim Leers, Berdir: Entity reference...
Wim Leers’s picture

🎉

samuel.mortenson’s picture

Wow! Thanks everyone.

Status: Fixed » Closed (fixed)

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