Entity reference field normalization has target_type and target_uuid, but not used in denormalization

Right now denormalization will not happen for FieldItems for non-hal normalization

Created this #2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods
Has patch that fixes that.

Postponing this issue on that 1

duh! meant to change the title not tag. Also relating issue

Just to get the ball rolling, something like this is a start and should work OK. I don't think we can inject the entity manager into the class constructor as it'll be a BC break?

I don't think we can inject the entity manager into the class constructor as it'll be a BC break?

We can if we set the default value to NULL.

I guess that could work, as there is currently NO constructor. So any classes extending this could be doing their own thing. Just means we need to check for the existence of the entity manager before doing anything, or returning NULL otherwise. I don't think we can throw exceptions there as that would also break peoples code.

Cool. I was mainly thinking if people are extending this class. So from that standpoint a required property could break in extending classes (unless we check for the entity manager and just make this a NULL op if it's not there...?).

+++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
@@ -39,4 +40,11 @@ public function normalize($field_item, $format = NULL, array $context = []) {
+    return \Drupal::entityManager()->loadEntityByUuid($data['target_type'], $data['target_uuid']);

Can we guarantee that $data['target_uuid'] will be set? If it is custom construct REST request it likely won't be. Think we should default to parent constructValue() if not.

Also is entityManager is deprecated correct? Should we use EntityRepository here?

Here is patch that does those 2

Can we guarantee that $data['target_uuid'] will be set?

I think we can because we should assume that the same Normalizer class is normalizing the data. I originally had this check in an overridden normalize() method, then realised we might not need it. It doesn't hurt, that's for sure. If we are going to add this check in, we should also check for the target_type being present. We should also move these checks into the denormalize method. Checking these items are present. then calling the parent method to do the work (which will in turn call constructValue()).

This looks good! I guess we just need some test coverage now.

Yes, entity repository is correct here I think. That initial patch was lazy, more just testing the actual denormalization worked locally (which it did).

1. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
   @@ -35,8 +53,30 @@ public function normalize($field_item, $format = NULL, array $context = []) {
   +    if (!empty($data['target_uuid'])) {
   

   Let's check for 'target_type' too.

2. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
   @@ -35,8 +53,30 @@ public function normalize($field_item, $format = NULL, array $context = []) {
   +    else {
   

   We don't really need this else. This can just be the default return statement for the function.

#16.1 fixed
#16.2 fixed

#17.1 update service definition and change constructor
#17.2 removed

Added test coverage also

From the comments in #17 we are getting rid of all the things we put in place to maintain BC for this class, no? Or is changing service definitions in a breaking way ok? Otherwise if anyone is extending this class, their code will be broken?

1. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
   @@ -36,12 +40,19 @@ class EntityReferenceFieldItemNormalizerTest extends UnitTestCase {
   +  /**
   

   nit: needs newline

2. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
   @@ -121,4 +140,51 @@ public function testNormalizeWithNoEntity() {
   +  /**
   +   * @covers ::denormalize
   +   * @dataProvider providerTestDenormalize
   +   */
   +  public function testDenormalize($data) {
   

   TBH, I think it would be better if this was just split into two test methods instead of using the data provider. testDenormalizeWithTypeAndUuid and testDenormalizeWithId or something? This is mainly just my preference though :)

#22.1 fixed
#22.2 Got rid of dataProvider and made 2 test methods. Added assertDenormalize() because the last 4 lines of the test methods were the same.

Also noticed I could reuse $this->fieldItem from setup() instead of having local vars $field_item.

All this allows testDenormalizeWithId() to be very simple.

#25.1
$this->normalizer = new EntityReferenceFieldItemNormalizer($this->entityRepository->reveal());
In setup() is neccessary because all of the other existing test methods(before this issue) use it. Just had to update the lines for the new constructor.

re: $normalizer = new EntityReferenceFieldItemNormalizer($this->entityRepository->reveal());
I wasn't using $this->normalizer in this case because I think I had a misunderstanding \Prophecy\Prophecy\ObjectProphecy::reveal().

I thought of more like clone so any expectations set on $this->entityRepository mock object after
$this->normalizer = new EntityReferenceFieldItemNormalizer($this->entityRepository->reveal());
in setUp() would not affect the object return from $this->entityRepository->reveal().

This is not the case I guess. I can use $this->normalizer in assertDenormalize() and the expectations still hold.
I have done this. Let me know if I am misunderstanding this.

#25.3

Hm, no ->reveal() here. Why does this work then?

Because \Drupal\serialization\Normalizer\FieldItemNormalizer::denormalize checks

if ($context['target_instance']->getParent() == NULL) {
      throw new InvalidArgumentException('The field item passed in via $context[\'target_instance\'] must have a parent set.');
    }

and \Prophecy\Prophecy\ObjectProphecy is also != to null. Since we are using a mock EntityReferenceItem the setValue() doesn't actually fail if there is no parent. in the real \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::setValue() it does rely on $this->parent.

I have added the reveal() call thought because it makes more sense.

#25.2
Not addressed in this patch but some thoughts....

Before #2060677: Add target_type, target_uuid to serialized output of entity reference fields in non-HAL formats which is this issue is follow up of we supported just 'target_id'.
But since we added target_uuid and target_type to normalize() it made sense to use it for denormalize() which allows it work when the actual entity id of the target has changed.
We still need to support just target_id because previous code for REST could have been written to POST/create entities which only used target id. So we should not break that.

We could support requests with just target_uuid because we could look up the target_type from the field definition. This would probably be nice for developers making POST requests where the entity was NOT previously normalized with this class but should we do it here or in a follow up.

Ok. here is patch that should fix some of the tests. But I think it might need a test update. Here is what I have found for this error:

1) Drupal\Tests\rest\Functional\EntityResource\Node\NodeJsonAnonTest::testPatch
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"Access denied on updating field \u0027uid\u0027."}
+{"message":"Access denied on updating field \u0027created\u0027."}

So this is coming from \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testPatch line 781

// DX: 403 when sending PATCH request with read-only fields.
    // First send all fields (the "maximum normalization"). Assert the expected
    // error message for the first PATCH-protected field. Remove that field from
    // the normalization, send another request, assert the next PATCH-protected
    // field error message. And so on.
    $max_normalization = $this->getNormalizedPatchEntity() + $this->serializer->normalize($this->entity, static::$format);
    for ($i = 0; $i < count(static::$patchProtectedFieldNames); $i++) {
      $max_normalization = $this->removeFieldsFromNormalization($max_normalization, array_slice(static::$patchProtectedFieldNames, 0, $i));
      $request_options[RequestOptions::BODY] = $this->serializer->serialize($max_normalization, static::$format);
      $response = $this->request('PATCH', $url, $request_options);
      $this->assertResourceErrorResponse(403, "Access denied on updating field '" . static::$patchProtectedFieldNames[$i] . "'.", $response);
    }

So basically the test is expecting that for all patchProtectedFieldNames including uid, it is expecting an error saying the field can't be updated.

the error is coming from \Drupal\rest\Plugin\rest\resource\EntityResource::patch(). If we look at that function

// Unchanged values for entity keys don't need access checking.
        if ($original_entity->get($field_name)->getValue() === $entity->get($field_name)->getValue()) {
          continue;
        }

Basically uid should not get an error message because an entity key and should be unchanged.
The reason this is not happening without the patch is that target_uuid, target_type and url are taken into account during denormalization. So the the fields values above are never the same.

So should remove uid from \Drupal\Tests\rest\Functional\EntityResource\Node\NodeResourceTestBase::$patchProtectedFieldNames because it not protected in the sense that you can send it a patch request and not get an error if it is the same.

+++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
@@ -61,7 +61,8 @@ public function normalize($field_item, $format = NULL, array $context = []) {
-      return $this->entityRepository->loadEntityByUuid($data['target_type'], $data['target_uuid']);
+      $entity = $this->entityRepository->loadEntityByUuid($data['target_type'], $data['target_uuid']);
+      $data['target_id'] = $entity->id();

So this isn't really fix for the problem outlined above in #29 but I do like it better.

It seems weird that in constructValue() we would sometime return and entity object and sometimes return an array.

I guess this only works because \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::setValue() takes this into account.

I think it would be better if we right before the return statement here we did.

$data = ['target_id' => $data['target_id']]
Because really the extra keys are just for use to be able to figure what the correct/current target id should be.

That sounds OK to me too (and I like the change to just add the ID to the return value for conistency). I think we should just do:

/**
 * {@inheritdoc}
 */
protected function constructValue($data, $context) {
  if (!empty($data['target_uuid']) && !empty($data['target_type'])) {
    $entity = $this->entityRepository->loadEntityByUuid($data['target_type'], $data['target_uuid']);

    return ['target_id' => $entity->id()];
  }

  return parent::constructValue($data, $context);
}

Here is a patch that does a few things from what @damiankloip suggested in #32 and some another that chatted with @damiankloip and chatted about on IRC.
1. #32 suggestion
2. If $data['target_type'] is not provided get it from $field_item->getFieldDefinition()->getSetting('target_type')
3. If $data['target_type'] is provide make sure it is correct and throw an error if not

For 2 & 3 I will try to some up what @damiankloip and I discussed.

a. It makes sense to leave target_type in normalization because it is useful metadata. For instance if you request entity A which has field field_reference that references entity B then the target_type will allow you know the entity type of B without knowing the field configuration(or having to request it) so you can make another request to load entity B.
b. If we leave target_type in normalize() because of reason above then developers will send it back or also use it for new POST requests. So we even though we can figure out the type without target_type being sent we should check to make sure it is right or developers might get confused and assume they can change it.
c. we should NOT require target_type because we can easily figure out from the field definition.

This patch removes uid from \Drupal\Tests\rest\Functional\EntityResource\Node\NodeResourceTestBase::$patchProtectedFieldNames

this is because if unchanged uid and other entity keys will not throw an error. For more info see #29

#33 failed for this reason plus a random test failure.

+++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
@@ -35,8 +54,24 @@ public function normalize($field_item, $format = NULL, array $context = []) {
+      $entity = $this->entityRepository->loadEntityByUuid($target_type, $data['target_uuid']);
+      return ['target_id' => $entity->id()];

IMHO here is a source of a potential bug: This call can easily fail: for example by someone passing in a wrong UUID. In this case, there is a call to a null object ...

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
   @@ -23,7 +23,6 @@
   -    'uid',
   

   I'm not sure about this change, I guess Wim can give us a definitive answer on this. I remember seeing similar fails on some other issue that were down to casting. You reasoning does make sense to me though.

2. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
   @@ -35,8 +54,24 @@ public function normalize($field_item, $format = NULL, array $context = []) {
   +      if (!empty($data['target_type']) && $target_type !== $data['target_type']) {
   

   Kind of a nit, but we could check there is a target type set before getting the target type from the field definition.

3. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
   @@ -35,8 +54,24 @@ public function normalize($field_item, $format = NULL, array $context = []) {
   +      $entity = $this->entityRepository->loadEntityByUuid($target_type, $data['target_uuid']);
   

   Yes, was going to say this too, Daniel is right, we really need to check we have loaded an entity first. Question is, do we just leave it (return nothing) OR return some kind of exception so we can trigger a 422 response?

4. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
   @@ -35,8 +54,24 @@ public function normalize($field_item, $format = NULL, array $context = []) {
   +    return parent::constructValue($data, $context);
   

   I think maybe we should always get this first before checking for the UUID. Then just amend $data['target_id']?

5. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
   @@ -121,4 +152,83 @@ public function testNormalizeWithNoEntity() {
   +  public function testDenormalizeWithTypeAndUuid() {
   

   Related to above comments, I guess we could also do with a test case to cover the instance that no entity is loaded from the provided UUID.

@Wim Leers re

I don't fully understand this yet, but I can see how that is related. Can you explain this a bit more?

This patch just has some code so show why uid is different in \Drupal\rest\Plugin\rest\resource\EntityResource::patch and therefore have in uid patchProtectedFieldNames works now.
Basical when comparing the original field value and the patch entity the difference is

Array
(
    [orig] => Array
        (
            [0] => Array
                (
                    [target_id] => 0
                )

        )

    [entity] => Array
        (
            [0] => Array
                (
                    [target_id] => 0
                    [target_type] => user
                    [target_uuid] => 4812cd5c-5a84-47de-a80c-f716d294fbc4
                    [url] => /d8_3_rest/user/0
                )

        )

)

since target_id is the same this will denormalize to the same thing. but you get the error about protected field because the values are not equal.

So in a nutshell, the loaded entity field only has 'value', as that's all we store for an entity reference but the posted data is generated from the normalized data, so it contains the other keys too.

So should we do something like check the keys present in the loaded field values match only in these cases?

@Wim Leers thanks for the context about #2824851

re

But the patch in this issue does not change the normalize() method, only the denormalize() method is changed. And if we're still sending only target_id, then shouldn't this still work?

I'm probably missing something. Sorry for holding this up.

We aren't changing normalize() but without the patch...

1. \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testPatch is sending a request with a normalized Node which contains the extra target_uuid and target_type for uid(this is the same with/without the patch)
2. Without this patch the effectively is no special denormalization for entity reference fields.
3. So the EntityReferenceFieldItem will be denormalized with \Drupal\serialization\Normalizer\FieldItemNormalizer::constructValue which makes no changes to the incoming $data
4. Because $data is unchanged it will not exactly match the current fieldItem value as shown in #40(it will have other keys besides target_id)
5. Because the incoming value will not exactly match original field value \Drupal\rest\Plugin\rest\resource\EntityResource::patch will throw an error which the unpatched test expects

With new patch:

1. \Drupal\serialization\Normalizer\FieldItemNormalizer::constructValue is no longer used but instead \Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer::constructValue is called which removes target_uuid and target_type for uid and just leaves target_id
2. Because the incoming value will exactly match original field value \Drupal\rest\Plugin\rest\resource\EntityResource::patch will not throw an error so uid has to be removed from \Drupal\Tests\rest\Functional\EntityResource\Node\NodeResourceTestBase::$patchProtectedFieldNames

This patch addresses @damiankloip's review in #38
1. see #39 to #43
2. fixed
3. Checking $entity exists and throwing a InvalidArgumentException if not. Haven't updated Rest to handle this yet.
4. If we always call parent::constructValue() first and just amend then we will have the columns from normalize() returned from construct value which we know are not valid field columns. The doc for \Drupal\serialization\Normalizer\FieldItemNormalizer::constructValue says

Build the field item value using the incoming data.

It seems then we should remove values we know normalize provides but aren't field values.
This relates to @damiankloip question in #41

So should we do something like check the keys present in the loaded field values match only in these cases?

This seems like larger problem than this issue. We could that generically \Drupal\serialization\Normalizer\FieldItemNormalizer::denormalize
By changing
$field_item->setValue($this->constructValue($data, $context));
To something like

$value = $this->constructValue($data, $context);
// Check if is_array b/c maybe not? Like in an earlier version of this patch where we returned the entity.
if (is_array($value)) {
 $value = $this->removeNonFieldColums($value);
}
$field_item->setValue($value);

But would there be cases where you would want to pass in non- field column value to $fieldItem->setValue() with some metadata?
5. added test

Related to #44, I think it's fine if the constructValue() method always just returns the 'target_id' only for ER fields? If not (DER maybe?) yeah, we need to think about it. But seems like DER should just extend this instead if it is an issue.

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
   @@ -23,7 +23,6 @@
   -    'uid',
   

   Doesn't the latest patch mean we can restore this again?

2. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
   @@ -2,12 +2,15 @@
   +use Doctrine\Common\Proxy\Exception\InvalidArgumentException;
   

   This is not the correct InvalidArgumentException class, you meant \Symfony\Component\Serializer\Exception\InvalidArgumentException I think. Same goes for the expected exception in the new test coverage.

3. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
   @@ -35,8 +55,31 @@ public function normalize($field_item, $format = NULL, array $context = []) {
   +    if (!empty($data['target_uuid'])) {
   

   It would make more sense if this exception logic lived in denormalize but alas I think this makes it trickier as we still need to check for the existence of the target_uuid property to load by UUID. In other words - this is fine :)

   Related to this; this would mean that an empty target_uuid would just skip this code. Do you think we should throw an error if this is present at all in the data?

4. +++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
   @@ -35,8 +55,31 @@ public function normalize($field_item, $format = NULL, array $context = []) {
   +        throw new InvalidArgumentException(sprintf('"%s" is not a valid UUID.', $data['target_uuid']));
   

   Do you think "No entity found with UUID" or similar would be more helpful? Because using the word valid makes me think it is not correctly formatted.

1. No because the patch will still result in the denormalized field value having the single element array with the key 'target_id' which will match exactly the current field value. Therefore since this an entity key field there will no error given and uid should not be in \Drupal\Tests\rest\Functional\EntityResource\Node\NodeResourceTestBase::$patchProtectedFieldNames
2. Whoops, I should be more careful with PHPStorm auto-completes. fixed
3.

Related to this; this would mean that an empty target_uuid would just skip this code. Do you think we should throw an error if this is present at all in the data?

If "empty target_uuid" then they should have target_id. I am not sure how much hand-holding we should do here to check things because most field we are just leaving $data untouched in denormalization. This issue is really just to make we handle target_uuid if it is there because normalize() puts in there. I don't think we should work about $data if that is not the case.
4. Yes that was not a great error message. Change the message to

No "test_type" entity found with UUID "unique-but-none-non-existent" for field "field_reference".

Hopefully this will help because an entity will likely have multiple entity reference fields pointing to different type.

I also noticed the change I made for #38.2 caused it not to work if target_type is not provided.
So I fixed that and created the new test method \Drupal\Tests\serialization\Unit\Normalizer\EntityReferenceFieldItemNormalizerTest::testDenormalizeWithUuidWithoutType

That new exception message is much better, thanks!

Regarding #3, I just mean that in the event that 'target_uuid' is present but just an empty string or NULL (or 0) we should not fallback to the previous behaviour, we should throw an error. If there is a target_uuid key present, we should not allow the previous behaviour. IMO.

@damiankloip thanks for the clarification I read too quickly.

Yes that makes sense. Updated the patch. Throwing an error if target_uuid isset but empty.
Added test.

Awesome, this is RTBC now as far as I'm concerned.

+++ b/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php
@@ -35,8 +55,32 @@ public function normalize($field_item, $format = NULL, array $context = []) {
+    if (isset($data['target_uuid'])) {
+      /** @var \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem $field_item */
+      $field_item = $context['target_instance'];

Is it worth expanding this if to be like this?

if (isset($data['target_uuid']) && $context['target_instance'] instanceof EntityReferenceItem) {

This would mean that if someone has a property of target_uuid on a field and it is not an entity reference we wouldn't do any of this entity reference stuff - which seems correct. Setting to needs review to get thoughts on that. I might be wrong.

The patch has nice test coverage of the different exceptions - nice one.

1. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
   @@ -121,4 +152,152 @@ public function testNormalizeWithNoEntity() {
   +   * @expectedExceptionMessage The field "field_reference" property "target_type" must be set to "test_type" or omitted.
   

   line exceeding 80 characters

2. +++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
   @@ -121,4 +152,152 @@ public function testNormalizeWithNoEntity() {
   +   * @expectedExceptionMessage If provided "target_uuid" cannot be empty for field "test_type".
   

   line exceeding 80 characters

There are some minor error 2827164-48.patch of "line exceeding more than 80 characters" Applying patch , please review

+++ b/core/lib/Drupal/Core/Plugin/ContextAwarePluginAssignmentTrait.php
@@ -48,7 +48,7 @@ protected function addContextAssignmentElement(ContextAwarePluginInterface $plug
-      if (count($options) > 1 || !$definition->isRequired()) {
+      if (count($options) > 1 || (count($options) == 1 && !$definition->isRequired())) {

+++ b/core/modules/block/src/Tests/BlockUiTest.php
@@ -249,6 +249,13 @@ public function testContextAwareBlocks() {
+    // Test context mapping form element is not visible if there are no valid
+    // context options for the block (the test_context_aware_no_valid_context_options
+    // block has one context defined which is not available for it on the
+    // Block Layout interface).
+    $this->drupalGet('admin/structure/block/add/test_context_aware_no_valid_context_options/classy');
+    $this->assertNoField('edit-settings-context-mapping-email', 'The mapping selector is missing as expected.');

Out-of-scope changes - I guess coming from another patch.

+++ b/core/modules/serialization/tests/src/Unit/Normalizer/EntityReferenceFieldItemNormalizerTest.php
@@ -121,4 +152,154 @@ public function testNormalizeWithNoEntity() {
+   * @expectedExceptionMessage The field "field_reference" property
+   *   "target_type" must be set to "test_type" or omitted.
...
+   * @expectedExceptionMessage No "test_type" entity found with UUID
+   *   "unique-but-none-non-existent" for field "field_reference".

Actually splitting these lines makes things hard because it becomes more difficult to search the code base for the message. This is an annotation for PHPUnit. That said we're moving away from the annotation because of https://thephp.cc/news/2016/02/questioning-phpunit-best-practices so we could remove these annotations and replace with something like

    $this->setExpectedException(
      InvalidArgumentException::class,
      'No "test_type" entity found with UUID "unique-but-none-non-existent" for field "field_reference".'
    );

Yes, +1 to #56 here I think. We could add it, but in theory it would be pretty much redundant. FieldableEntityNormalizerTrait will pass the item to denormalization which as Wim mentioned, will be used when determining the normalizer. It is possible people could pass some data for denormalization themselves and mess this up. That's the only instance this could come in to play.

Reverted the changes from the patch in #53 and changed the exception annotations to use setExcpectedException instead.

Committed 2cd2ec6 and pushed to 8.4.x. Thanks!

@Pavan B S thank you for trying to fix issues in this patch. I've not credited you because the work done proved more disruptive to the issue - when fixing coding standards on issues it is vital that you try not to upload patches with out-of-scope changes.

@dawehner I've creditted you for #37 - spotting things like this that make our code more robust is exactly what reviewer credit is for.