Contrib.social

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

Open on Drupal.org →
Created on 24 November 2017, about 7 years ago
Updated 25 April 2024, 8 months ago

Problem/Motivation

After updating entity text field value with leading zeros or plus, the changes are not saved.
The issue was found in Profile entity.

Steps to reproduce:

  1. Add a new "Text (plain)" field to a profile entity on admin/config/people/accounts/fields
  2. Create a new user with the field value "123523" and save
  3. Edit the user and change the value to "0123523"

Expected result:
The field is updated

Actual result:
The field is not updated.

The issue is caused by PHP strings comparison. Currently we check values like $value1 == $value2 which for "123523" == "0123523" will return true.

Proposed resolution

Change comparison to consider types of compared values.
We cannot change comparison to $value1 === $value2. That can lead to incorrect comparison of database values (strings) and current values (can be int).

We need to check strings strictly but save the current comparison of int and string.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

🐛 Bug report
Status

Needs work

Version

11.0 🔥

Component
Field  →

Last updated 1 day ago

Created by

🇺🇦Ukraine skrasulevskiy

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 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 8 months ago
  • Comment 8 months ago →
    🇸🇰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

  • Comment 8 months ago →
    🇸🇰Slovakia poker10

    Hiding all patches from summary.

  • First commit to issue fork.
  • Pipeline finished with Success
    8 months ago
    Total: 1047s
    #149191
  • Comment 8 months ago →
    🇺🇸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 8 months ago
  • Comment 8 months ago →
    🇺🇸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 8 months ago
  • Comment 8 months ago →
    🇬🇧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.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024