Problem/Motivation
Discovered in #2824851-110: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior. See details in #2824851-117: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#26 | interdiff-26.txt | 577 bytes | amateescu |
#26 | 2906600-26.patch | 8.96 KB | amateescu |
#17 | interdiff.txt | 6.06 KB | amateescu |
#17 | 2906600-17.patch | 8.96 KB | amateescu |
#14 | 2906600-14.patch | 2.9 KB | amateescu |
Comments
Comment #2
Wim LeersComment #3
Wim LeersThis must expand the test coverage of
\Drupal\Tests\Core\Field\FieldItemListTest::testEquals()
, but that is currently hardcoding the following:This will need to be made dependent on the fields it receives from
\Drupal\Tests\Core\Field\FieldItemListTest::providerTestEquals()
, to allow for a computed field to be passed.Comment #4
Wim LeersThis is the change that @cburschka is proposing. Let's see how it fares.
Comment #6
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedgetPropertyDefinitions()
returns something very different thangetColumns()
(an array of data definition objects vs. an array of SQL schema column names), so how can the following code still function correctly?Comment #7
tstoecklerRe #6: That works because we're really only interested in the array keys, i.e. the property/column names.
It's still rather confusing/unintuitive, though, in my opinion. Since the specified columns must be a subset of the property definitions we could consider to simply always compare the list of property names instead of the column names. That would make the code more readable and also in my opinion more correct as it would no longer be tied to the storage-specific implementation detail of "columns".
There are two problems (possibly more) with that, though:
entity
property which would then be compared. Especially becauseFieldItemList::equals()
uses weak comparison (==) this can not only lead to unexpected results if e.g. different clones of the same entity are being compared, but also infinite recursion. To mitigate this we could avoid computed properties, but that seems a bit strange for a fix for bug that occured because we were ignoring computed fields, i.e. it seems we are just stuffing the same problem one level further down in the typed data stack.Comment #8
Wim LeersNote that this was discovered particularly in relation to
PathItem
, which is kind of a special case: it's a field with a custom storage, which required it to be marked as computed in #2539634: PathItem::delete() never runs because the path field type is a computed field in disguise. I think the patch by @cburschka was completely based onPathItem
, but it may be a super exceptional case thatFieldItemList::equals()
shouldn't cater for? I don't know. I defer to you, the Entity/Field API experts :)entity
property on entity reference fields is computed, but any entity reference field is not computed! SoFieldItemList::equals()
updated with the changes in #4 would not cause any different behavior forFieldItemList::equals()
AFAICT.Comment #9
tstoecklerRe #8: Right I was not referring to #4 but rather to my proposal in #7. However, thinking about it now, there really is only a difference in degree, not in principle, as we can never know whether fields of a specific type will be marked as computed for a certain entity type. Entity Backreference, for example, implements a computed entity reference field, so it could possibly introduce the problem described in #7.1. I don't know of any modules doing computed map fields (which would then trigger #7.2) but it's certainly not unthinkable.
Not sure, would be awesome to get @Berdir's input on this.
Comment #10
Wim LeersPinged @Berdir in IRC, told him you said we need his input :)
Comment #11
cburschkaRegarding #7:
This part comes back to #2392845: Add a trait to standardize handling of computed item lists, ultimately.
Each field presumably has a set of "comparison" keys, whose equality is equivalent to the equality of the respective field items. If we assume these are the same as the storage columns, it breaks for PathItem. But just as you say, the property definitions are too broad - they include stuff that shouldn't be compared, like the entity object.
Maybe one of the following two then:
1. Override PathFieldItemList::equals(), and do the same for any other computed field. This makes sense if PathItem is a rare exception, and each computed field needs its own comparison logic. For robustness, it might even make sense to enforce this by throwing an exception if a computed field class doesn't override FieldItemList::equals().
2. Create a ::getComparisonKeys() method somewhere in the data definition inheritance chain (not sure where) which defaults to the columns for normal fields, but must be overridden by computed fields. Same as above, but with most of the actual comparison logic from FieldItemList being reused.
Comment #12
Wim Leers#7:
Related: #2895532: @DataType=map cannot be normalized, affects @FieldType=link, @FieldType=map. I think that means that if #2895532 lands, we'll have a problem similar to #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior?
Comment #13
Wim Leers@tstoeckler pointed out that #2916967: FieldItemList::equals() is broken for field types without a schema is essentially a duplicate of this issue. We're probably going to merge that issue into this one, with @amateescu's new patch.
Comment #14
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHere's my patch from #2916967: FieldItemList::equals() is broken for field types without a schema which essentially does what @tstoeckler said in #7, it makes the code always use the property names and also skip computed properties.
I'll have to think about the MapItem problem later and fix the unit tests as well.
Comment #15
Wim LeersSo this is switching from column names to non-computed property names.
AFAICT this is the reasoning: computed properties aren't stored, therefore don't have columns. Therefore we should only look at non-computed properties. And of course, we only need to look at stored properties.
👍
This is only testing that if one of the three non-computed properties is changed, it works as expected.
Is this sufficient test coverage?
Shouldn't we also test this for:
pid
andlangcode
)?foobar
, which should be set to a random value, which should simply ignored by::equals()
?Comment #16
Wim Leers#2916967: FieldItemList::equals() is broken for field types without a schema was closed as a duplicate of this. Let's also merge the issue metadata.
Comment #17
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedOk, here's the longer version of the answer to #7.1 and #9:
The latest patch already implements the equality check only for non-computed properties, so we're good regarding the 'entity' property of the entity reference field, for example.
As for kicking the same problem down the road (typed data stack), that's not the case because there is no lower level in typed data that is actively responsible for storing "things" in a persistent way. I.e. there's no concept of "property storage" as an equivalent to "field storage".
The purpose of
FieldItemList::equals()
is to check the equality for something that is actually stored somewhere, that's why it currently uses the columns returned by theschema()
method to figure out what needs to be discarded from the equality check.A more straightforward answer to #9 is that all fields, computed or not, have some properties defined, so
FieldItemList::equals()
should work just fine with the current patch.Re #7.2: As you already pointed out, one notable exception to the above is
MapItem
which is the only field type without any properties defined.But supporting the equality check for the Map field type is really easy, we just need to write a specialized item list class for it where we override the parent
equals()
method and make it check only its values (i.e. the output ofgetValue()
), which is also whatMapItem::toArray()
does.This is implemented in the patch attached.
Re #15:
1. Yup, that's exactly the reasoning :)
2. I've extended the unit test for
FieldItemList::equals()
itself with a case where a computed property has different values and keeps the equality true, and another case with a non-existing property which also keeps the equality true.Comment #18
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNote that I didn't use a strict check here for the same reason that the parent method does not use a strict check as the final return value.
That reason is explained in #2475237: Method FieldItemList::equals() uses type safe comparison limiting usefulness.
Comment #19
Wim LeersWow, that all sounds very convincing and solid :) Can’t wait to read @tstoeckler’s reply!
/me cheering from the sidelines for the Entity and Field API maintainers 😁👏
Comment #20
tstoecklerThanks for your elaborate response, very hard to argue with that. Even though I did have some reservations the fact that my comment and your patch independently arrived at a similar solution is also kind of a good sign.
Patch itself looks good and the test coverage is very nicely written and precise (and sufficient!).
I just had one thought:
Since this is copied verbatim from
FieldItemList
we could theoretically have athat we call out from
equals()
so thatMapFieldItemList
can overwrite the array_walk stuff more directly.If you feel that increases the complexity of field item lists, I don't feel strongly, though. Since overwriting
equals
for lists is somewhat of an edge case I'm also fine with leaving the patch as is.Comment #21
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented@tstoeckler, my general rule of thumb for when to create a helper method is when some non-trivial code is repeated at least three times, which is not case here :) So if you don't feel that strongly about it, I would prefer to not increase the complexity in
FieldItemList
.And thanks for the swift review!
Comment #22
tstoecklerFair enough. Let's do this.
Comment #23
Wim Leers🎉
Comment #24
Wim LeersThis blocks #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior.
Comment #25
catchThe second check isn't actually testing that one is empty, it's testing that they have different numbers of items in them - is it just the comment that's wrong?
Comment #26
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedYup, just the comment is wrong :) It was inherited from
\Drupal\Core\Field\FieldItemList::equals()
but that's not an excuse for not correcting it here.Comment #27
catchThanks, back to RTBC.
Comment #28
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #31
Wim LeersYay, one less blocker for #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior!