Problem/Motivation

The Field UI attaches a change event listener to every form element inside field storage config edit form to update the field config form based on changes made to the storage settings. However, if user submits the form while the change event is still being processed, this leads to a race condition where incomplete or invalid data may be saved.

For example, creating field with a negative value in the Allowed number of values, and directly clicking save button on the form (without unfocusing the field) then the form is successfully saved instead of showing the validation error.

In some cases, such as the list field, this could lead into losing changes made to the options.

Steps to reproduce

  1. Navigate to the add field page of the of any content type.
  2. Select the date type as filed.
  3. Manually enter any negative value in Allowed number of values in the form.
  4. Save the form.

Proposed resolution

The quick fix for this would be to disable the submit button while the AJAX is processing. This would not be the ideal UX but would address the critical data loss issue. We could open a follow-up issue to submit the form without first triggering the change event.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 Screenshot 2023-10-30 at 9.09.38 AM.png118.24 KBlauriii

Issue fork drupal-3397594

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Utkarsh_33 created an issue. See original summary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Active » Postponed (maintainer needs more info)
StatusFileSize
new Screenshot 2023-10-30 at 9.09.38 AM.png118.24 KB

I'm wondering if there's a step missing from the steps to reproduce? Tried to follow the steps but it looks like validation is preventing this from happening. Also looked at the code and didn't find anything risky in the field type.

utkarsh_33’s picture

utkarsh_33 commented
Status: Postponed (maintainer needs more info) » Needs work

So if we enter any negative value in the given field( which updates the message shown using Ajax) and save the form directly without waiting for the Ajax response then it saves the form even after the message is shown up. I'll write a test to demonstrate this.

Utkarsh_33 opened merge request !5184

utkarsh_33’s picture

utkarsh_33 commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Rebased to run test-only feature

1) Drupal\Tests\datetime\FunctionalJavascript\AllowedValuesForDateFieldTypeTest::testAllowedValuesFormValidation
Behat\Mink\Exception\ResponseTextException: The text "Limit must be higher than or equal to 1." was not found anywhere in the text of the current page.
/builds/issue/drupal-3397594/vendor/behat/mink/src/WebAssert.php:811
/builds/issue/drupal-3397594/vendor/behat/mink/src/WebAssert.php:262
/builds/issue/drupal-3397594/core/modules/datetime/tests/src/FunctionalJavascript/AllowedValuesForDateFieldTypeTest.php:66
/builds/issue/drupal-3397594/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
ERRORS!
Tests: 1, Assertions: 4, Errors: 1.

Which is good.

Manually testing I can confirm the issue following the steps in the issue summary.
Applying the MR I was unable to advance the form and got the correct error message.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Priority: Normal » Critical
Status: Reviewed & tested by the community » Needs work

Discussed this issue with @lauriii. This issue can cause a problem for list fields where you enter some values in the allowed values and then press submit and they are not saved as you've pressed the submit before the ajax has run. So this causes data loss - hence making it critical.

I'm not sure about the fix as form.js is loaded on every form.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Title: Form validation is broken for the date type field. » Race condition on AJAX change event and form submission
Issue summary: View changes
Issue tags: +Needs tests
utkarsh_33’s picture

utkarsh_33 commented
Status: Needs work » Needs review
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +Needs followup

Thanks @Utkarsh_33! I looked into the APIs AJAX system provides and couldn't figure out a better way to do this in the Field UI module. Maybe this is something the AJAX system should handle so that we wouldn't have to override and listen to the AJAX events? Can we file a follow-up to investigate a better solution. This seems fine since it addresses a critical bug.

catch’s picture

catch
he/him
Primary language English
commented

Double click prevention was added in #1705618: Double click prevention on form submission in 2014. There are open issues like #3251249: Should double-click prevention return early when isDefaultPrevented?. I have no idea if that prevention is supposed to work in that situation and is broken, or never worked for it though.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

This is different from the double click scenario. This is when you have two events triggered at the same time. This can happen with blur/change event listeners when users clicks a button that triggers the click event. What the MR does is it marks the submit button (in Field UI specifically) disabled while the other AJAX request is triggering to avoid the race condition caused by triggering multiple events at once.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

I think this is good for now - it's a specific fix caused by the issue of have field storage and field config on the same form. I think there is a possible follow up to think about whethere we should prevent all form submission whilst we're ajaxing.

Committed and pushed 27f9fa664c0 to 11.x and 07830391275 to 10.2.x. Thanks!

  • alexpott committed 27f9fa66 on 11.x 
    Issue #3397594 by Utkarsh_33, lauriii: Race condition on AJAX change...

  • alexpott committed 07830391 on 10.2.x 
    Issue #3397594 by Utkarsh_33, lauriii: Race condition on AJAX change...
utkarsh_33’s picture

utkarsh_33 commented
Issue tags: -Needs Review Queue Initiative, -Needs followup

I have opened a follow-up for finding a better(more generic) solution this problem in Find a generic way to resolve race condition on AJAX change event and form submission.

Status: Fixed » Closed (fixed)

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