Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#28 | interdiff-2060677-28.txt | 1.06 KB | damiankloip |
#28 | 2060677-28.patch | 4.34 KB | damiankloip |
#24 | interdiff-2060677-24.txt | 1.69 KB | damiankloip |
#23 | interdiff-2060677-23.txt | 1.73 KB | damiankloip |
#23 | 2060677-23.patch | 4.15 KB | damiankloip |
Comments
Comment #1
dawehnerI am wondering whether this should better be part of entity_reference?
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedNo, 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.Comment #4
dawehnerGood point!
What do you think about checking the interface as well, so we also have potential static analyze of the code available.
Comment #5
damiankloip CreditAttribution: damiankloip commentedOk, 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?
Comment #6
damiankloip CreditAttribution: damiankloip commentedOops, removed an assertion comment that I didn't mean to.
Comment #7
damiankloip CreditAttribution: damiankloip commentedBah!!
Comment #8
damiankloip CreditAttribution: damiankloip commentedRerolled. Couple of things needed to be removed/changed too.
Comment #9
damiankloip CreditAttribution: damiankloip commented8: 2060677-8.patch queued for re-testing.
Comment #10
damiankloip CreditAttribution: damiankloip commentedRerolled.
Comment #11
damiankloip CreditAttribution: damiankloip commented10: 2060677-10.patch queued for re-testing.
Comment #13
dawehnerI wonder whether it would be helpful to put in the referenced entity type ID as well.
Comment #16
damiankloip CreditAttribution: damiankloip commented#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.
Comment #17
damiankloip CreditAttribution: damiankloip commentedNo 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.
Comment #20
damiankloip CreditAttribution: damiankloip commentedNeed to fix the test I only added a few days ago :)
Comment #21
larowlanShould we also be adding a new EntityResolverInterface here for denormalizing?
Comment #22
damiankloip CreditAttribution: damiankloip commentedWe 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?
Comment #23
damiankloip CreditAttribution: damiankloip commentedAdding the target type too.
Comment #24
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedDoh. Missed the EntitySerializationTest.
Comment #27
dawehnerIMHO even for test we should have a proper uuid. Here is one you can use, just generated
Comment #28
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedForgot about this one, added a real UUID to the unit test.
Comment #29
dawehnerThis 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.
Comment #30
Wim LeersStill applies cleanly to 8.1.
lgtm
Comment #31
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThe 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 byDrupal\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 waytarget_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?
Comment #32
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedI 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.
Comment #34
damiankloip CreditAttribution: damiankloip at Tag1 Consulting commentedComment #35
Wim LeersI see that now — sorry about that!
Comment #36
effulgentsia CreditAttribution: effulgentsia at Acquia commentedTicking credit boxes for reviewers.
Comment #38
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.1.x. Thanks!
Comment #40
Wim LeersFollow-up created: #2827164: Entity reference field normalization has target_type and target_uuid, but not used in denormalization.