FieldItemList::equals() doesn't correctly compare textfield values with leading zeros

Wrong patch.

Patch from comment #10 is wrong.

Here is updated patch.

Proposed the new comparison mechanism.
Added new test cases.

Very sorry, missed when moved changes.

Hi,

Unfortunately, the patch #14 doesn't fix the original issue ( change of field value "123523" == "0123523").
I have added several tests in FieldItemListTest.php to cover the case and several others that are important for core/tests/Drupal/KernelTests/Core/Entity/ContentEntityChangedTest.php

And modified arrayEquals given in the patch #14.
I still don't like it (arrayEquals) there but currently it does comparison for according to expectations.

Please review.

Could someone review the issue?

I don't think including all sorts of type-specific checks in FieldItemList is a good solution, to be honest.

I think a better solution would be to add an override of the ::equals() method to StringItemBase to account for the problem described in the issue summary.

If we want to go for a more generic solution and more accurately cover more types of fields, we could add an ::equals() method to FieldItemInterface with a base implementation in FieldItemBase that just checks ==. Then field types could override the equality check without providing a list class.

>I think a better solution would be to add an override of the ::equals() method to StringItemBase to account for the problem described in the issue summary...

I don't agree here. The issue is not only about comparing 2 single values.
Otherwise, could you provide some details to illustrate your idea?

The issue is not only about comparing 2 single values.

Well, I'm not sure what it's about then. Per the issue summary this is about comparing (for example) "123523" and "0123523", no? Sorry, if I misunderstood something. Maybe you can clarify.

> If we want to go for a more generic solution and more accurately cover more types of fields, we could add an ::equals() method to FieldItemInterface with a base implementation in FieldItemBase that just checks ==. Then field types could override the equality check without providing a list class.

Sounds interesting. See #2988309: Ensure that all field types return TRUE on equals() for the same values and additional related issues there where it would be easier for specific field types if they could just override the by-item equals() implementation without having to duplicate the part of the list class that compares the amount of items.

Would need to check how that affects performance though.

The patch in this issue did not apply cleanly to 8.6.x, so here is a reroll.

FWIW, it fixes an issue we had with storing 0-prefixed strings, so we will probably be using this patch until a more generic solution is in core.

Faced the same issue. We have a Paragraph with a field Block (plugin) type. And we use the Facets Block module to be used in this field. It provides a list of checkboxes with all available facets.
So, when the User tries to edit the existing paragraph and change the list of facets he can't save it. He receives a message about successful saving, but actually, changes are not applied.
The patch from comment #31 solved the issue.

I've tested too the patch from #31 in the current latest Drupal 9 version, 9.1.4.
It merges fine and fixes the issues of modifying the field with leading zeros.

IMO, this should be RTBC

Hi @isholgueras

Patch #31, Failed in CI and it is not working in Drupal 9.1. Getting this error message.

Checking patch core/lib/Drupal/Core/Field/FieldItemList.php...
warning: core/lib/Drupal/Core/Field/FieldItemList.php has type 100755, expected 100644
Hunk #1 succeeded at 408 (offset 11 lines).
Checking patch core/tests/Drupal/Tests/Core/Field/FieldItemListTest.php...
warning: core/tests/Drupal/Tests/Core/Field/FieldItemListTest.php has type 100755, expected 100644
error: while searching for:
// not exist ('3').
$datasets[] = [TRUE, $field_item_h, $field_item_i];

return $datasets;
}

error: patch failed: core/tests/Drupal/Tests/Core/Field/FieldItemListTest.php:135
error: core/tests/Drupal/Tests/Core/Field/FieldItemListTest.php: patch does not apply

So NW

Reroll patch given for 9.2.x, Please have a look.

The $this->arrayEquals($value1, $value2); method is iterating and comparing the arrays key value, so I have updated tests where the method is tested for false input.

Applied patch #41 on 9.2.x and it works fine.The field value is not changing after applying this patch.

After patch:

RTBC +1

Patch #41 works fine and fix another use case.
If there is a custom field with a serialized property, it wasn't updated because the unserialized array was wrongly considered as equal to the old array value. That's because of the php implicit conversion of types.

In my case, patch #2 also worked (using php7.4) because identity operator (===) works fine with nested arrays so, I'm just curious about why it was discarded to develop more complex solutions.

Re-rolled #41 for 9.4.

Hi guys,

The problem still exist on drupal 9.3.9....
I want to be able to update a text field "1" to "01"... But the only solution I founded was to do it on 2 steps :
- First : change "1" to anything other than only 1 with 0 ( example : "A" or "343" )
- Secund : finally change the previous update to "01" and then, I finally get the update "1" to "01"

Can anybody help me on this please?

Patch purpose for Drupal 9.3.x:

Thanks for all the work on this everyone! I ran into the problem described in the IS, and this patch fixed it right up for me on 9.4.4.

From #41:

The $this->arrayEquals($value1, $value2); method is iterating and comparing the arrays key value, so I have updated tests where the method is tested for false input.

Changing the outcome of an existing test from FALSE to TRUE makes me nervous. Can this get some discussion? Do we really want that? Is it a regression? Is it an enhancement?

I don't like how $field_item_j is declared with two different values in the test. That should be updated for clarity.

+++ b/core/tests/Drupal/Tests/Core/Field/FieldItemListTest.php
@@ -135,6 +135,96 @@ public function providerTestEquals() {
+    /** @var \Drupal\Core\Field\FieldItemBase  $field_item_j */
+    $field_item_j = $this->getMockForAbstractClass('Drupal\Core\Field\FieldItemBase', [], '', FALSE);
+    $field_item_j->setValue(['+2']);

+++ b/core/tests/Drupal/Tests/Core/Field/FieldItemListTest.php
@@ -135,6 +135,96 @@ public function providerTestEquals() {
     /** @var \Drupal\Core\Field\FieldItemBase  $field_item_j */
     $field_item_j = $this->getMockForAbstractClass('Drupal\Core\Field\FieldItemBase', [], '', FALSE);
     $field_item_j->setValue(['0' => 1]);

I think the same goes for $field_item_m. That's confusing to declare it twice with different values.

I'm updating the target branch too. I think 10.1 is the bug target currently.

I'm going to respond to myself. From #52:

Changing the outcome of an existing test from FALSE to TRUE makes me nervous. Can this get some discussion? Do we really want that? Is it a regression? Is it an enhancement?

I believe this change was needed due to a bug in the new arrayEquals method. The method is not symmetric. What I mean is that the test is asserting$datasets[] = [TRUE, $field_item_j, $field_item_l];. But if I reverse the two arguments and assert $datasets[] = [TRUE, $field_item_l, $field_item_j];, then that fails. I think this method needs to be symmetric.

I think the easiest way to do that is to start the arrayEquals method with a comparison of the lengths of the input arrays.

if count($array1) !== count($array2) {
  return FALSE;
}

As far as I can tell, though, this is bug would only be exposed through edge cases. At least I hope so, because I'm using the patch from #50!

I think this is Major, as per:

Cause user input to be lost, but do not delete or corrupt existing data.

Looking at steps to reproduce, this affects also a simple use-case of changing a textfield value. Tested this on 11.x-dev using steps to reproduce from IS and it seems to be still an issue.

I have converted patch from #50 to MR. Also implemented suggestions from #51 (changed variable names) and #52 (reverted the change to the current tests). Moving to Needs review.

We can consider if #25 will be a viable approach, but if it can possibly impact performance, maybe we can consider fixing this first (given the severity) and then working on a more robust solution in a follow-up? I will leave this decision to core committers. Thanks!

Tests are green: https://git.drupalcode.org/project/drupal/-/pipelines/148484
Test only job failed as expected: https://git.drupalcode.org/project/drupal/-/jobs/1344634

Hiding all patches from summary.

https://git.drupalcode.org/issue/drupal-2925972/-/jobs/1351125 shows the extensive test coverage.

Wondering though if array_diff_assoc() could be leveraged anywhere here?

Think I'm being nitpicky, this does appear to improve things and if it can be improved maybe that should be a follow up.

For me this is a critical bug. Your data is not being saved. That's data loss. I think we need to turn the logic of the method around so that we're testing for equallity rather than inequality. And return FALSE by default. I think this will be more robust and easier to reason about.

Excellent work on extending the unit tests... I do wonder if we've got all the cases covered.