Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Issue tags: +Needs tests

This must expand the test coverage of \Drupal\Tests\Core\Field\FieldItemListTest::testEquals(), but that is currently hardcoding the following:

    $field_storage_definition = $this->getMock('Drupal\Core\Field\FieldStorageDefinitionInterface');
    $field_storage_definition->expects($this->any())
      ->method('getColumns')
      ->willReturn([0 => '0', 1 => '1']);

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.

Wim Leers’s picture

Status: Active » Needs review
FileSize
1.3 KB

This is the change that @cburschka is proposing. Let's see how it fares.

Status: Needs review » Needs work

The last submitted patch, 4: 2906600-4-cburschka.patch, failed testing. View results

amateescu’s picture

+++ b/core/lib/Drupal/Core/Field/FieldItemList.php
@@ -394,8 +393,15 @@ public function equals(FieldItemListInterface $list_to_compare) {
+      $columns = $this->getFieldDefinition()->getFieldStorageDefinition()->getPropertyDefinitions();
...
+      $columns = $this->getFieldDefinition()->getFieldStorageDefinition()->getColumns();

getPropertyDefinitions() returns something very different than getColumns() (an array of data definition objects vs. an array of SQL schema column names), so how can the following code still function correctly?

tstoeckler’s picture

Re #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 reference fields have an entity property which would then be compared. Especially because FieldItemList::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.
  • Map fields will always be considered equal regardless of their values due to #2887105: MapItem violates FieldItemInterface::schema()'s contract - breaks widgets. Note that even though I consider that a bug I don't think it makes sense to block this (or anything) on that as @Berdir does not consider it a bug, so regardless where we collectively come down on this I don't think it will be fixed anytime soon.
Wim Leers’s picture

Note 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 on PathItem, but it may be a super exceptional case that FieldItemList::equals() shouldn't cater for? I don't know. I defer to you, the Entity/Field API experts :)

There are two problems (possibly more) with that, though:

  1. No, because only the entity property on entity reference fields is computed, but any entity reference field is not computed! So FieldItemList::equals() updated with the changes in #4 would not cause any different behavior for FieldItemList::equals() AFAICT.
  2. IDK what to say about that other than that it scares me :)
tstoeckler’s picture

Re #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.

Wim Leers’s picture

Pinged @Berdir in IRC, told him you said we need his input :)

cburschka’s picture

Regarding #7:

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".

Entity reference fields have an entity property which would then be compared. Especially because FieldItemList::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.

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.

Wim Leers’s picture

#7:

  1. Map fields will always be considered equal regardless of their values due to #2887105: MapItem violates FieldItemInterface::schema()'s contract - breaks widgets. Note that even though I consider that a bug I don't think it makes sense to block this (or anything) on that as @Berdir does not consider it a bug, so regardless where we collectively come down on this I don't think it will be fixed anytime soon.

Related: #2895532: MapData cannot be serialized . 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?

Wim Leers’s picture

@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.

amateescu’s picture

Here'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.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Field/FieldItemList.php
    @@ -378,7 +379,6 @@ protected function defaultValueWidget(FormStateInterface $form_state) {
    -    $columns = $this->getFieldDefinition()->getFieldStorageDefinition()->getColumns();
    
    @@ -396,9 +396,13 @@ public function equals(FieldItemListInterface $list_to_compare) {
    +    $property_definitions = $this->getFieldDefinition()->getFieldStorageDefinition()->getPropertyDefinitions();
    +    $non_computed_properties = array_filter($property_definitions, function (DataDefinitionInterface $property) {
    +      return !$property->isComputed();
    +    });
    ...
    -        $value = array_intersect_key($value, $columns);
    +        $value = array_intersect_key($value, $non_computed_properties);
    

    So 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.

    👍

  2. +++ b/core/modules/path/tests/src/Kernel/PathItemTest.php
    @@ -171,6 +171,24 @@ public function testPathItem() {
    +    // Change the alias for the second node to a different one and try again.
    +    $second_node->get('path')->alias = '/foobar';
    +    $this->assertFalse($node->get('path')->equals($second_node->get('path')));
    

    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:

    • the two other non-computed properties (pid and langcode)?
    • a non-existent property foobar, which should be set to a random value, which should simply ignored by ::equals()?
Wim Leers’s picture

amateescu’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
8.96 KB
6.06 KB

Ok, here's the longer version of the answer to #7.1 and #9:

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.

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 the schema() 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 of getValue()), which is also what MapItem::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.

amateescu’s picture

+++ b/core/lib/Drupal/Core/Field/MapFieldItemList.php
@@ -0,0 +1,34 @@
+    return $value1 == $value2;

Note 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.

Wim Leers’s picture

Wow, 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 😁👏

tstoeckler’s picture

Thanks 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:

+++ b/core/lib/Drupal/Core/Field/MapFieldItemList.php
@@ -0,0 +1,34 @@
+    $count1 = count($this);
+    $count2 = count($list_to_compare);
+    if ($count1 === 0 && $count2 === 0) {
+      // Both are empty we can safely assume that it did not change.
+      return TRUE;
+    }
+    if ($count1 !== $count2) {
+      // One of them is empty but not the other one so the value changed.
+      return FALSE;
+    }
+
+    // The map field type does not have any property defined (because they are
+    // dynamic), so the only way to evaluate the equality for it is to rely
+    // solely on its values.
+    $value1 = $this->getValue();
+    $value2 = $list_to_compare->getValue();

Since this is copied verbatim from FieldItemList we could theoretically have a

protected function compareValues($value1,
 $value2)

that we call out from equals() so that MapFieldItemList 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.

amateescu’s picture

@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!

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Fair enough. Let's do this.

Wim Leers’s picture

🎉

Wim Leers’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/lib/Drupal/Core/Field/MapFieldItemList.php
@@ -0,0 +1,34 @@
+    $count2 = count($list_to_compare);
+    if ($count1 === 0 && $count2 === 0) {
+      // Both are empty we can safely assume that it did not change.
+      return TRUE;
+    }
+    if ($count1 !== $count2) {
+      // One of them is empty but not the other one so the value changed.
+      return FALSE;
+    }

The 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?

amateescu’s picture

Yup, 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.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed 3f20ff5 on 8.5.x
    Issue #2906600 by amateescu, Wim Leers, tstoeckler, cburschka:...

  • catch committed c687094 on 8.4.x
    Issue #2906600 by amateescu, Wim Leers, tstoeckler, cburschka:...
Wim Leers’s picture

Status: Fixed » Closed (fixed)

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