#2060677: Add target_type, target_uuid to serialized output of entity reference fields in non-HAL formats added target_type
and target_uuid
to the normalization. But didn't add denormalization support. It was intended to become a follow-up, but that follow-up was never filed. Until now.
Should we also be adding a new EntityResolverInterface here for denormalizing?
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?
Comment | File | Size | Author |
---|---|---|---|
#58 | interdiff-2827164-58.txt | 4.55 KB | damiankloip |
#58 | 2827164-58.patch | 11.5 KB | damiankloip |
#53 | interdif.txt | 2.96 KB | Pavan B S |
#53 | 2827164-54.patch | 13.3 KB | Pavan B S |
#48 | 2827164-48.patch | 11.43 KB | tedbow |
Comments
Comment #2
tedbowRight 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
Comment #3
tedbowduh! meant to change the title not tag. Also relating issue
Comment #4
Wim Leers#2827218: Denormalization on field items is never called: add FieldNormalizer + FieldItemNormalizer with denormalize() methods landed, this is now unblocked!
Comment #5
jibranComment #6
damiankloip CreditAttribution: damiankloip at Acquia commentedJust 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?
Comment #7
jibranWe can if we set the default value to NULL.
Comment #8
damiankloip CreditAttribution: damiankloip at Acquia commentedI 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.
Comment #9
Wim LeersFYI: service constructors do not need to maintain BC per https://www.drupal.org/core/d8-bc-policy.
Comment #10
damiankloip CreditAttribution: damiankloip at Acquia commentedCool. 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...?).
Comment #11
tedbowCan 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
Comment #12
tedbowComment #15
damiankloip CreditAttribution: damiankloip at Acquia commentedI 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()).
Comment #16
damiankloip CreditAttribution: damiankloip at Acquia commentedThis 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).
Let's check for 'target_type' too.
We don't really need this else. This can just be the default return statement for the function.
Comment #17
Wim LeersLet's remove the default of
NULL
.More importantly:
serialization.services.yml
has not been updated to inject this service.Let's get rid of this.
Comment #18
tedbow#16.1 fixed
#16.2 fixed
#17.1 update service definition and change constructor
#17.2 removed
Added test coverage also
Comment #19
tedbowComment #21
damiankloip CreditAttribution: damiankloip at Acquia commentedFrom 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?
Comment #22
damiankloip CreditAttribution: damiankloip at Acquia commentednit: needs newline
TBH, I think it would be better if this was just split into two test methods instead of using the data provider.
testDenormalizeWithTypeAndUuid
andtestDenormalizeWithId
or something? This is mainly just my preference though :)Comment #23
Wim Leers#21: this is what https://www.drupal.org/core/d8-bc-policy says:
Comment #24
tedbow#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.
Comment #25
Wim LeersAFAICT
$this->normalizer
is unnecessary/unused?I'm missing test coverage for three edge cases:
I know those can't work (although, why can't the second one work?), but then we should have test coverage documenting their error behavior.
Hm, no
->reveal()
here. Why does this work then?Comment #27
tedbow#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
Because \Drupal\serialization\Normalizer\FieldItemNormalizer::denormalize checks
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.
Comment #29
tedbowOk. 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:
So this is coming from \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::testPatch line 781
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
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.
Comment #30
tedbowSo 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.
Comment #32
damiankloip CreditAttribution: damiankloip at Acquia commentedThat 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:
Comment #33
tedbowHere 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.
Comment #35
tedbowThis 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.
Comment #37
dawehnerIMHO 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 ...
Comment #38
damiankloip CreditAttribution: damiankloip at Acquia commentedI'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.
Kind of a nit, but we could check there is a target type set before getting the target type from the field definition.
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?
I think maybe we should always get this first before checking for the UUID. Then just amend
$data['target_id']
?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.
Comment #39
Wim LeersI'm sorry, but #35 + #29 don't yet convince me that this change is correct.
Whatever the full explanation is, we should also put that in an inline comment here!
From #29:
This is correct.
I don't fully understand this yet, but I can see how that is related. Can you explain this a bit more?
Comment #40
tedbow@Wim Leers re
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
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.
Comment #41
damiankloip CreditAttribution: damiankloip at Acquia commentedSo 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?
Comment #42
Wim LeersGaahh, that means that
this is a consequence of the bug described in #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, and it's just getting in the way here. But yes, then that change makes sense. Not making that change requires fixing #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior, and we should definitely not block this on that. It still behaves effectively the same as before, as long as you don't send
target_uuid
etc.Hm… but that's the thing, AFAICT we're not sending
target_uuid
etc?EntityResourceTestBase::testPatch()
does this:i.e. it serializes every field, and subsequently removes every PATCH-protected field after having verified that sending a PATCH-protected field in a PATCH request results in the expected 403 response.
But the patch in this issue does not change the
normalize()
method, only thedenormalize()
method is changed. And if we're still sending onlytarget_id
, then shouldn't this still work?I'm probably missing something. Sorry for holding this up.
Comment #43
tedbow@Wim Leers thanks for the context about #2824851
re
We aren't changing normalize() but without the patch...
With new patch:
Comment #44
tedbowThis 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
It seems then we should remove values we know normalize provides but aren't field values.
This relates to @damiankloip question in #41
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
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
Comment #45
damiankloip CreditAttribution: damiankloip at Acquia commentedRelated 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.
Doesn't the latest patch mean we can restore this again?
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.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?
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.
Comment #46
tedbow1. 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.
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
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
Comment #47
damiankloip CreditAttribution: damiankloip at Acquia commentedThat 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.
Comment #48
tedbow@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.
Comment #49
damiankloip CreditAttribution: damiankloip at Acquia commentedAwesome, this is RTBC now as far as I'm concerned.
Comment #52
alexpottIs it worth expanding this if to be like this?
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.
Comment #53
Pavan B S CreditAttribution: Pavan B S at Valuebound commentedline exceeding 80 characters
line exceeding 80 characters
There are some minor error 2827164-48.patch of "line exceeding more than 80 characters" Applying patch , please review
Comment #54
alexpottOut-of-scope changes - I guess coming from another patch.
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
Comment #55
Wim LeersNW for all points in #54.
Comment #56
Wim Leers#52:
Expanding it is not necessary because
\Drupal\serialization\Normalizer\EntityReferenceFieldItemNormalizer
has:i.e. it will already only run for entity reference fields.
Comment #57
damiankloip CreditAttribution: damiankloip at Acquia commentedYes, +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.
Comment #58
damiankloip CreditAttribution: damiankloip at Acquia commentedReverted the changes from the patch in #53 and changed the exception annotations to use
setExcpectedException
instead.Comment #59
Wim LeersPerfect, thanks!
Comment #61
Wim LeersCI fail, retesting.
Comment #62
alexpottCommitted 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.
Comment #64
alexpottComment #65
Wim Leers@alexpott++
Comment #66
Wim LeersThis unblocked #2844946: Add DynamicEntityReferenceItemNormalizer for serialization module.