Existing field items should not be validated when adding another item in widget for unlimited cardinality field

Drupal 9.4.0-alpha1 → was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.3.0-rc1 → was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.2.0-alpha1 → will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Drupal 9.1.0-alpha1 → will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule → and the Allowed changes during the Drupal 9 release cycle → .

Drupal 8.9.0-beta1 → was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule → and the Allowed changes during the Drupal 8 and 9 release cycles → .

Can confirm that issue exists when initially field is not filled.

Drupal 9.5.0-beta2 → and Drupal 10.0.0-beta2 → were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

Can confirm this issue is still happening in 9.5.x

Can confirm this issue still happening in 10.1.
I tried to fix this issue but I couldn't.

I will share what I understand about the issue.

It happens the first time because when we do the request we override the HTML of the field. On WidgetBase.php:235 the wrapper id is overriding with the same id + Html::getUniqueId.
Replacing the html content on the DOM the ajax instance lost the reference to the content, so when commandQueue execute that last command insert to add the messages the element doesn't exist anymore. That's why in the first click on "add more" button we didn't see the error message, only the input appear with the error style.

A quick fix could be removing on WidgetBase:253 the function Html::getUniqueId. I'm not sure if in this scenario we need to return HTML with a new id considering that we are replacing the old element. But it's difficult to guarantee that this will not generate side effects.

But in the next few days, I'll try to do it and run the unit test to check that.

Hey guys, I could fix the error message not appearing adding a fallback on Drupal.commands.insert function. The fallback check if the wrapper isn't present on the dom, if not the fallback looks for the element id and try to find the wrapper based on the element id.

Also, I tested with fields with multiple trees as paragraphs for example, and also it works.

Appears to be CI failures in #13

Also @reenaraghavan don't see a change?

Fixing eslint errors.

Steps followed:-

1. Created a link field in a content type with cardinality unlimited and marked it as required field.
2. Create a node for the same content type.
3. Clicked 'Add another item' without filling data in the first field. It blocked 'Add another item' by highlighting the field, but there was no status message.
4. Filled the link field with a value, it was successful .
5. Remove value from the first link field and try to 'Add another item' again.
6. With the above trigger and two fields, the error message is shown and the first link field is highlighted.
7. Applied the patch #17 and repeated steps 1 to 3. This time the error message is shown correctly.
Tested it on Drupal version 10.1.x and I have added the before and after screenshots for reference.

Looks like good catch from
@joaopauloc.dev .

+1 RTBC

Should we update this issue to tested and reviewed by community?
What do you think @nayana-ramakrishnan and @smustgrave ?

So testing is done via #18. So any follow up would just be duplicated effort.

A code review would still need to happen.

And good rule of thumb is with bugs they will almost always need a test case to show the issue. Once there’s a test it should be uploaded as a test only patch, with the full patch too. So we can see the red/green tests

Hey @smustgrave, I added the test unit to show the issue and after that I applied the patch that fix it.
What next? I search for a tag like Needs code review but couldn't found.

I was just pointing out I didn’t review the code earlier. But when doing a review it’s suppose to be one of the steps before marking RTBC

Testing MR 3330

Running the test locally without the fix I got an error which is good and show the error.

Behat\Mink\Exception\ElementNotFoundException : Element matching xpath "//div[starts-with(@id, "field-unlimited-add-more-wrapper")]/div[starts-with(@class, "messages-list")]" not found.

Tested following the steps in the IS. Attached screenshot to IS.

Good job!

What is being fixed here? The issue summary consists of steps to reproduce and a screenshot. There should be a statement of what the fix is and the screenshot should state if it is a before or after screenshot. In fact, as a UI issue there should be before and after screenshots available from the Issue Summary. I am tagging this for an Issue Summary update.

A reminder to everyone that keeping the Issue Summary up to date and complete is something anyone working on an issue should contribute to.

Hey @quietone Issue summary updated, please let me know if needs other details.

Issue summary changes look good.
Shows the issue where the error is not showing and is now showing.

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

We need a change record here to detail the new fallbackWrapper ajax setting. Also we need to update the Ajax API documentation in core.api.php.

Before we do that it'd be great to get some feedback from a frontend maintainer as to whether this is the right way to go.

Fantastic work identifying the underlying cause of the issue. I think the current proposed solution resembles what I'd like to ultimately see, but I don't think it's quite there yet. As the variable names suggest, it's providing a user-configurable fallback for when an expected selector isn't available, in this case due to an ID being unique-ified. This is difficult to test reliably and seems like it's addressing a symptom and not the underlying problem.

A few things come to mind that may help steer this towards an approach I'd FEFM-approve:

• The concept of data-drupal-selector already exists to create a stable selector since id can change on AJAX rebuilds.
• In the #ajax setup for adding an item in \Drupal\Core\Field\WidgetBase::formMultipleElements, no method is specified and as a result the default method with default settings is used. Specifically implementing a method may offer the customization needed without having to create new apis - wether it is the arguments sent to the commands or the order they are executed.
• I notice that the approach does this
  

   const $wrapper = response.selector
          ? $(response.selector)
          : $(ajax.wrapper);

  has been changed to let $wrapper, so that the wrapper can be changed if certain conditions are happening. I also noticed response.selector is always null when the reported problem happens. When it isn't null, it becomes the wrapper. Getting the necessary selector into response.selector uses existing APIs to address the issue in a predictable manner. If this requires adding a more evergreen selector to the markup, that would be fine FE: $elements['#prefix'] = '<div id="' . $wrapper_id . '" data-drupal-selector="SOME UNCHANGING SELECTOR'...

.

Thanks for the comments @bnjmnm. I'll try to adjust the approach to use the data-drupal-selector instead of using the current approach proposed.

A new approach to fix the issue was used.
Following @bnjmnm comments, I fixed using the current API.
I updated the Widget.php on addMoreAjax function to return a new response with an ajax commands defining the selector and returning the same structure returned by the AjaxRenderer service renderResponse.
There is a space to improve the fix by refactoring the renderResponse, but could be too much and dangerous since the method is an implementation of MainContentRendererInterface. But, this method could accept other parameters to have different behavior based on parameters, like using replace command with selector instead of always just Insert and Prepend items without selector.

Removing tags Needs change record and Needs documentation updates as we don't have any API changes anymore.

joaopauloc.dev → closed merge request !3330

The Patch first-error-message-not-appear-3076054-17.patch by joaopauloc.dev is working fine. After applying patch error message is showing on field when no value is entred. Everything is working fine as required.

I wonder if a similar solution to the changes in latest MR (4717), but with a simpler diff could suffice to address the issue?

Something that looks like below worked for me (fix only patch attached in case anyone else wants to test)

diff --git a/core/lib/Drupal/Core/Field/WidgetBase.php b/core/lib/Drupal/Core/Field/WidgetBase.php
index 7b0fa54961..93ee2b93a5 100644
--- a/core/lib/Drupal/Core/Field/WidgetBase.php
+++ b/core/lib/Drupal/Core/Field/WidgetBase.php
@@ -268,6 +268,12 @@ protected function formMultipleElements(FieldItemListInterface $items, array &$f
       if ($is_unlimited_not_programmed) {
         $elements['#prefix'] = '<div id="' . $wrapper_id . '">';
         $elements['#suffix'] = '</div>';
+        // AJAX processing will replace the wrapper element when another item is
+        // added. The new content wrapper will have a different DOM ID, so it
+        // is necessary to use this new ID as the selector to prepend status
+        // messages markup to.
+        // @see \Drupal\Core\Render\MainContent\AjaxRenderer::renderResponse()
+        $elements['#ajax_status_messages_selector'] = "#$wrapper_id";

         $elements['add_more'] = [
           '#type' => 'submit',
diff --git a/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
index 189a7c489d..0f6b66d002 100644
--- a/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
+++ b/core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php
@@ -73,7 +73,8 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
     $status_messages = ['#type' => 'status_messages'];
     $output = $this->renderer->renderRoot($status_messages);
     if (!empty($output)) {
-      $response->addCommand(new PrependCommand(NULL, $output));
+      // Use selector specified for the status messages to prepend to if set.
+      $response->addCommand(new PrependCommand($main_content['#ajax_status_messages_selector'] ?? NULL, $output));
     }
     return $response;
   }

Curious if anyone could see if this is still an issue? Followed the steps in the issue summary and I get the expected status message without the patch or MR.

@smustgrave: I have confirmed the issue still exists. I followed the steps in the IS and reproduced as described. Some added details I did:

1. Installed new site with standard profile on latest 11.x
2. Added new plain text field to Article content type. Set cardinality to unlimited and made the field required. Did not set a default value
3. Went to /node/add/article to create a new article
4. Left the required text field empty and clicked Add new item
5. Confirm that additional text field is not added, that there is a error state on the original text field, and no status message appears

Rest is as IS describes.

I've updated patch from #40 to include tests from MR 4747 and attached as new patch.

Will see if someone else can replicate in the mean time.

btw if we do go with a new solution issue summary will have to be updated.

Thanks for the patch @godotislate, nice solution, good job.
I could reproduce the issue.

Running the patch with only the test to see tests failing.

The last submitted patch, 44: 3076054-43-only-test.patch, failed testing. View results →

If we are going if the new solution could that be documented in the issue summary please.

Thanks for verifying, not sure why it didn't appear for me but good work everyone!

Added commits per patch in #40 🐛 Existing field items should not be validated when adding another item in widget for unlimited cardinality field Needs work to new MR 5043 based on commits from MR 4717.

Updated IS.

Thanks, will still need frontend framework manager sign off but think this is ready for their review.

alexpott → closed merge request !4717

Hiding all the files so only the current MR is showing,

Discussed briefly with @nod_ and we agreed that the solution doesn't feel correct yet. Hopefully @nod_ will comment further, but it feels odd that we're adding something new to fix this.

The last commit of the MR changes the approach, when there is a different approach it's better to create a new Merge request for the new approach to avoid loosing work.

The previous approach worked and was more in line with what we're comfortable with so I reverted the last commit and pushed it to the latest of the branch.

If the other approach wants to be worked on please create a new MR on this issue so we don't overwrite each other's work

@nod_
Thanks for the feedback.

FYI, MR 5043 https://git.drupalcode.org/project/drupal/-/merge_requests/5043 was created as a new MR with the new approach, just with additional commits for the new approach added to keep the history of the work done in MR 4717.

MR 4717 https://git.drupalcode.org/project/drupal/-/merge_requests/4717 was the original MR (now closed) with the approach you prefer. If that approach is preferred, I don't think there's a reason to open another PR to continue work on the other approach in 5043, unless there's a way forward to make that one preferred.

I didn't see the different MRs and that you actually did open a new request, sorry about that.

And yes the previous approach was preferred so let's refine that one! thanks!

@nod_

Rebased MR against latest 11.x and pushed commit to address review comment about escaping quotes.

Have outstanding question about the suggested switch to MessageCommand instead of PrependCommand.

Pushed commit to MR to use MessageCommand instead of PrependCommand and rebased.

After seeing it working, switching the position of the error message is not good. Added a code suggestion to make sure the message shows up above the field.

Added commit to apply suggestion so that messages appear above the field

I'm wondering if the currently proposed solution is right. Why are we validating the items when users add new new values? When new values are added, the user is not actually submitting anything yet, so shouldn't we allow adding new values until they are actually submitting the form? This is at least the approach we took in 📌 List key|label entry field is textarea, which doesn't give guidance towards the expected input Fixed .

@lauriii: I agree that adding new items should be allowed regardless of whether the existing items pass validation. Created new MR 5176 with this solution.

That said, I originally arrived in this issue from 🐛 Link-widget throws exception when rebuilding a form with an invalid uri Fixed . And in the case of an unlimited cardinality link field, I think it'd be better for the user to be informed or warned about invalid URIs after they press "Add more", instead of after trying to save. Especially since this kind of validation also isn't done by the browser like required field validation is. But presenting warnings like might be something that can or should be implemented in specific widgets and not in WidgetBase.

I also looked at the commit history, and validation on the field on add more click was added way back in #370537: Allow suppress of form errors (hack to make "more" buttons work properly) → when #limit_validation_errors was introduced, but I didn't see any discussion about whether that validation should be done at all.

Removing the FEFM review tag given that the approach has changed since #31. Adding usability review as a tag since the proposal is to change the behavior.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2023-11-03 Needs work . That issue has a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @AaronMcHale, @anmolgoyal74, @benjifisher, @ckrina, @rkoller, and @simohell.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

It seems that there are two competing proposals for this issue:

1. Keep the current validation, and add en error message. (See the "Expected result" in the issue summary, under Problem/Motivation.)
2. Validate when the form is submitted, not when another item is added, so that the user can create several blank fields and then fill them in. (See the Proposed Solution in the issue summary and MR 5176.)

The Usability team agrees with Comments #63, #65: the second approach seems more user-friendly. I have not looked at the code, but I think the idea is to validate when the form is submitted, and save only the non-empty values.

Although we like the approach, we want to point out one drawback. There can be different expectations when some of the added items are left blank. If I save the page with one blank item, then edit again, it looks as though the fields have been rearranged, with the empty one moved to the end.

An advantage of the second approach is consistency with the interface for adding options to a List field, as pointed out in #63. I am adding the issue mentioned there as a related issue.

We think a little more work is needed here:

1. Update the title and the Problem/Motivation sections to match the proposed solution. I am adding the tag for an issue summary update.
2. For consistency with the UI for a List field, change the focus to the new, blank field after using the "Add another item" button.

If it does not make sense to do (2) as part of this issue, then it can be done in a follow-up issue.

Updated issue title and summary per #68 🐛 Existing field items should not be validated when adding another item in widget for unlimited cardinality field Needs work . Investigating whether the focus can be set on the new field item is TBD.

Rebased MR against latest 11.x and added commit to set the focus on the new field item.

Copied assertHasFocusByAttribute() from core/modules/options/tests/src/FunctionalJavascript/OptionsFieldUITest.php to core/modules/field/tests/src/FunctionalJavascript/MultipleValueWidgetTest.php, which is not DRY, but both instances are waiting on ✨ Add focus assertions to JavaScript tests Needs work , so this seemed to be the most straightforward way to proceed for now.

@godotislate there are 2 MRs against 11.x could one be closed please

godotislate → closed merge request !5043

Previous MR with validation messages closed.

Thanks! Ran the test-only feature and got

There was 1 failure:
1) Drupal\Tests\field\FunctionalJavascript\MultipleValueWidgetTest::testFieldMultipleValueWidgetAddMoreNoValidation
Successfully added another item.
Failed asserting that a NULL is not empty.
/builds/issue/drupal-3076054/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/builds/issue/drupal-3076054/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/builds/issue/drupal-3076054/core/modules/field/tests/src/FunctionalJavascript/MultipleValueWidgetTest.php:189
/builds/issue/drupal-3076054/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 2, Assertions: 33, Failures: 1.

Tested manually on a standard profile install
Confirmed following the steps in the issue summary that I can't add a new field till I fill in the first one
Applied the MR
Confirmed I can add multiple fields without error

I didn't fill in the first filed
Added a new one
Filled in the 2nd
Tried to save but got an error that the first must be filled in, which I would expect
Filling in the first allowed me to save
Order was saved correctly

Crediting UX team and others who've changed the shape/direction of the patch/MR

Can we get the issue summary updated to remove the screenshots that relate to the old approach?

Thanks

Removed screenshots from IS and pushed back to RTBC.

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions.

Very nice progress on this, thanks everyone!

This is tagged for an issue summary update but according to #77 that has been done so I am removing the tag. Can everyone remember to keep the tags up to date, thanks.

This has had a Usability review and approval by front end managers. Usually, such an issue will have before and after screenshots linked to from the Issue Summary to demonstrate the problem and fix to reviewers/committers etc.

I applied the diff and tested manually. I used a standard install on Drupal 11.x with a 'Number' field. I found that there is a difference in behavior when the first item is invalid and when it is not. When the first field is valid and there are other empty items added, then on 'Save' the node is saved and the blank items are removed. When the first field is empty and there are also other empty items, then on 'Save', I get an error 'Please enter a number'. I can not save until the first field is valid while the other empty fields can remain empty. I think this needs work for this point. Others may decide it is best in a followup. Either way, I found it unexpected and jarring.

I did not look at the MR at all.

So, just the one item to look at.

Updated the IS with before and after screenshots.

I can confirm the behavior observed in #78 🐛 Existing field items should not be validated when adding another item in widget for unlimited cardinality field Needs work .

1. Install Drupal with standard
2. Add a required Number field with unlimited cardinality to Article content type
3. Create a new Article
4. Press "Add another item" in the number field, with the existing field blank
5. Enter a number value in the new second field, leave first field blank
6. Press Save
7. In Chrome, HTML5 validation says the first field is required
8. If you disable browser validation by adding the "novalidate" attribute to the form, after trying to save, you are presented an error status message that the first number field is required

I think this might out of scope for this issue and suitable for a follow up. Currently, for any multiple cardinality field, if the field is set to required, the form element for the first delta has '#required' set to TRUE. Will ping usability team in Slack for feedback.

We discussed this issue at 📌 Drupal Usability Meeting 2023-12-29 Active . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, and @worldlinemine.

We agreed that the current MR makes an improvement, and it is all right to fix the remaining problem (pointed out in Comments #78 and #80) in a follow-up issue.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

Created 📌 Field widgets for required field with multiple cardinality makes first field item required Active as follow up. Moving back to RTBC.

Issue credits

Looking great, left a suggestion on the MR that simplifies things further, in my local testing both manual testing and the automated test still pass with my suggestion.

Made the suggested change and rebased.

godotislate → changed the visibility of the branch 11.x to hidden.

Feedback appears to have been addressed.

Committed to 11.x
As there's only internal changes here (except for tests) AND this is a bug, backported to 10.2.x

larowlan → closed merge request !5176

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