Currently, HAL outputs UUIDs and URIs for reference fields. However, the "simple" formats (e.g., json and xml), only output a direct representation of an entity. Since for performance, ER fields internally use just a 'target_id' (the local serial id), that's all that's included in the serialized output of those fields.

However, a common use case for serialization is to migrate/deploy an entity from one site to another (e.g., stage -> prod). Outputting UUIDs for ER fields would help that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue tags: +Needs tests
@@ -2,6 +2,10 @@ services:
+  serializer.normalizer.entity_reference_item:
+    class: Drupal\serialization\Normalizer\EntityReferenceItemNormalizer

I am wondering whether this should better be part of entity_reference?

Status: Needs review » Needs work

The last submitted patch, serialization-uuid.patch, failed testing.

effulgentsia’s picture

I am wondering whether this should better be part of entity_reference?

No, because the ER module is just for adding configurable fields. Base fields, like $node->uid, are instances of Drupal\Core\Entity\Plugin\DataType\EntityReferenceItem, which is the class targeted by this patch.

dawehner’s picture

Good point!

@@ -0,0 +1,39 @@
+    if ($target_entity = $object->entity) {
+      $attributes['target_uuid'] = $target_entity->uuid();
+    }

What do you think about checking the interface as well, so we also have potential static analyze of the code available.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.57 KB
5.01 KB

Ok, it's been a while, so rerolled with some tests. I've just modified EntitySerializationTest for now, as we pretty much just need to add a user that we can reference, the EntityReferenceItem->entity call will then return something.

I have plans to open up some new issues dedicated to actually unit testing our normalizers etc..

@dawehner: In your last comment do you mean checking that what's returned implement EntityInterface?

damiankloip’s picture

Issue tags: +Needs tests
FileSize
931 bytes
4.49 KB

Oops, removed an assertion comment that I didn't mean to.

damiankloip’s picture

Issue tags: -Needs tests

Bah!!

damiankloip’s picture

Issue summary: View changes
FileSize
4.19 KB
2.5 KB

Rerolled. Couple of things needed to be removed/changed too.

damiankloip’s picture

8: 2060677-8.patch queued for re-testing.

damiankloip’s picture

FileSize
4.43 KB

Rerolled.

damiankloip’s picture

10: 2060677-10.patch queued for re-testing.

damiankloip queued 10: 2060677-10.patch for re-testing.

dawehner’s picture

+++ b/core/modules/serialization/src/Normalizer/EntityReferenceItemNormalizer.php
@@ -0,0 +1,38 @@
+    // Convert the object to an array as for any field item (complex data).
+    // For entity reference items, that includes the 'target_id' property.
+    $attributes = parent::normalize($object, $format, $context);
+
+    // Additionally, add the target entity's uuid.
+    if ($target_entity = $object->entity) {
+      $attributes['target_uuid'] = $target_entity->uuid();
+    }
+
+    return $attributes;

I wonder whether it would be helpful to put in the referenced entity type ID as well.

damiankloip queued 10: 2060677-10.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 10: 2060677-10.patch, failed testing.

damiankloip’s picture

#2124677: Expose URI in file fields when serializing an object now makes this easier once more, and the EntityReferenceItemNormalizer is already added.

RE #13, I feel like anything consuming this should know the type or be using another type/system? Not sure if we should add that or not.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3 KB

No interdiff, just creating a new patch as most of what's needed is already in (for tests) and what was there before is different with the url addition and the EntityReferenceFieldItemNormalizer.

Status: Needs review » Needs work

The last submitted patch, 17: 2060677-17.patch, failed testing.

The last submitted patch, 17: 2060677-17.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.97 KB
997 bytes

Need to fix the test I only added a few days ago :)

larowlan’s picture

Should we also be adding a new EntityResolverInterface here for denormalizing?

damiankloip’s picture

We have the right tools already I think, we just need serialization module to actually use them. I think we should do that in a follow up to this though, and just keep this for adding this to the denormalized data. Sound ok?

damiankloip’s picture

Adding the target type too.

damiankloip’s picture

Doh. Missed the EntitySerializationTest.

The last submitted patch, 23: 2060677-23.patch, failed testing.

The last submitted patch, 23: 2060677-23.patch, failed testing.

dawehner’s picture

+++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
@@ -78,6 +78,12 @@ public function testNormalize() {
+    $entity->uuid()
+      ->willReturn('UUID')

IMHO even for test we should have a proper uuid. Here is one you can use, just generated

B73D697D-4F1D-4B90-90CC-B2A3EE90F486
damiankloip’s picture

Forgot about this one, added a real UUID to the unit test.

dawehner’s picture

This issue can be dramatically helpful for people using actually REST, but I somehow doubt that this task can land before 8.1
IMHO there is at least no reason for this to be a RC target, but feel free to disagree. Maybe this kind of issue can be committed for a patch release.

Wim Leers’s picture

Title: Add target_uuid to json and xml output of entity reference fields » Add target_type, target_uuid to serialized output of entity reference fields
Version: 8.0.x-dev » 8.1.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: +API-First Initiative

Still applies cleanly to 8.1.

lgtm

effulgentsia’s picture

Title: Add target_type, target_uuid to serialized output of entity reference fields » Add target_type, target_uuid to serialized output of entity reference fields in non-HAL formats
Status: Reviewed & tested by the community » Needs review

The patch looks good to me, but before committing it, I'm curious what folks here think of the following.

Because this patch changes Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer only, it does not affect the output of HAL responses, since that is controlled by Drupal\hal\Normalizer\EntityReferenceItemNormalizer. The issue's original title (prior to #30) explicitly called that out. However, with #30's broadening of the issue title, I'd like to double-check what we want to do with HAL.

For references to content entities, HAL already provides the UUID of the reference as uuid and the type of the reference as _links -> type -> href. Though note that this type is conveyed as a URI, not as a type string the way target_type is in this patch for the non-HAL formats.

But for references to config entities (e.g., the type field for node entities), HAL currently outputs neither the type of that (e.g., node_type) nor its UUID.

For now, retitling this issue to explicitly clarify that it's not changing what is output in HAL. If you all think this is ok for the scope of this issue, please re-RTBC it. Also, comment on whether you think we should open a follow-up to discuss whether we want to add something to HAL related to this. Or else, maybe we should retitle the issue back to #30's, and then set this to Needs work to add the same output to HAL?

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community

I would say we shouldn't get into hal here. In my mind this issue was always about regular normalization having this stuff.

The issue retitling was confusing from that point of view, maybe. Thanks for the update to that, and definitely clarifies this issues purpose. The scope was not broadened in any way. This normalizer intentionally does not have a specific format it supports, so it acts as the default.

Setting back to RTBC as before, based on that.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 2060677-28.patch, failed testing.

damiankloip’s picture

Status: Needs work » Reviewed & tested by the community
Wim Leers’s picture

The issue retitling was confusing from that point of view, maybe.

I see that now — sorry about that!

effulgentsia’s picture

Ticking credit boxes for reviewers.

  • effulgentsia committed ada4e57 on 8.1.x
    Issue #2060677 by damiankloip, dawehner, Wim Leers, larowlan: Add...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Pushed to 8.1.x. Thanks!

Status: Fixed » Closed (fixed)

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