- First commit to issue fork.
- Merge request !7530Issue #2925972: FieldItemList::equals() doesn't correctly compare textfield values with leading zeros β (Open) created by poker10
- Status changed to Needs review
5 months ago 7:26pm 16 April 2024 - πΈπ°Slovakia poker10
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 - First commit to issue fork.
- πΊπΈUnited States smustgrave
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?
- Status changed to RTBC
5 months ago 1:39pm 24 April 2024 - πΊπΈUnited States smustgrave
Think I'm being nitpicky, this does appear to improve things and if it can be improved maybe that should be a follow up.
- Status changed to Needs work
5 months ago 11:15am 25 April 2024 - π¬π§United Kingdom alexpott πͺπΊπ
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.