Media Library widget produces "This value should not be null" error when field is required

This will need a test before we can fix it.

Self-assigning for test coverage, and adding it to the Bug Smash Initiative since this is clearly a bug.

Here's a test that is failing for me on localhost.

Unassigning from myself, since I don't know how to fix this bug. :)

Fixed the issue related to error message when media field is required.

Instead of "This value should not be null" now error message will be "@field_name field is required."

I tried these patch https://www.drupal.org/files/issues/2020-08-10/3160238-6-FAIL.patch in my local and now it is passing all test cases. So adding test cases in my patch.

Test case output:

Testing Drupal\Tests\media_library\Functional\FieldWidgetTest
Test 'Drupal\Tests\media_library\Functional\FieldWidgetTest::testEmptyValue' started
Test 'Drupal\Tests\media_library\Functional\FieldWidgetTest::testEmptyValue' ended


OK (1 test, 9 assertions)

I tried to apply the patch of comment #9 using the composer on 9.0.x core version but the patch was failing.
So I refactored the patch for 9.0.x as well.

Confirmed that the patch applies and resolves the issue for me! Thank you!

I'm not 100% certain about the proposed solution here.

I'd like to drill into it a bit further to understand why it's producing this error. The media library is a compound field widget, and I'd like to know if this is something that's been encountered by other compound widgets, either in core or contrib, as well.

Tested via #10 and above mentioned test steps. It working fine for me after adding the patch
This can be moved to RTBC

Couldn't reproduce the issue

I followed the steps:-

• Modified the default "Basic Page" content type and added an image field marked as required.
• Tried creating a new page content by filling Title field and body field and leaving media field blank (Didn't browsed and selected image file)
• Clicked Save Button
• Error message shown as " field is required"

It would be helpful, if someone could specify whether a new Page content type is created and content added to produce the issue. Adding screenshots of the issue before applying the patch would be really helpful.

#9 works to me:

Before #9

After #9

But, @phenaproxima do you want a more generic solution?

Would the image field also be one of those fields?

After applying this patch #9 on Drupal 9.1.x , it is working fine and good to go for RTBC .

This needs and Issue Summary update.

Please when making screenshots, it is usually a good idea to update the Issue Summary with that information. It will help anyone working on the issue.

The question in #13 needs to be answered. And in #15 there is a report that the problem can not be reproduced. If that is confirmed this may be a won't fix.

@anu.a_95 and @quietone I also followed the steps mentioned in #15 however I am able to reproduce the issue.

Not able to apply patch #10, So re-roll patch given.

+++ b/core/modules/media_library/tests/src/Functional/FieldWidgetTest.php
@@ -0,0 +1,65 @@
+    $this->assertSession()->pageTextNotContains('This value should not be null');

This needs to also test that the desired test is displayed, I think it will be "field_media field is required."

And lets have a fail patch as well.

Updated test to check new message for required field. Adding tests only patch.

I have verified the above issue and found that it is working fine after applying the patch #26 on drupal 9.1.x and seems to be good for RTBC. Please find the attachment for the same.

Steps to test :

1. Add media field in page content type, and mark this as required.
2. Try creating page content, fill title & body and leave media as blank.
3. Click Save button.
4. It will give you message "This value should not be null"

Expected Result:

1. It should give proper message like "Media field is required."

Actual Result:

1. It gives message "This value should not be null".

#26 works for me as well.
This can be moved to RTBC + 1

I've made a new version of this patch for core 8.9.1.

Applied both patch #25 and #30 . Patch #25 works for 9.2.x and #30 works for 8.9.x respectively. The validation message will be displaying correctly with corresponding field names after applying these patches.

Attaching screenshots below
In Drupal 9.2.x:

In Drupal 8.9.x

The patch in #31 worked form me, i tested it using D8.9.6, the message is ok now but the field does not get highlighted in red like the others.

Sorry, found a nit.

+++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
@@ -939,4 +942,34 @@ protected static function setFieldState(array $element, FormStateInterface $form
+    // Trigger error if the field is required and no media is present.
+    // Although the Form API's default validation would also catch this, the
+    // validation error message is too vague, so a more precise one is
+    // provided here.

The wrapping here isn't right, needs to be wrapped at 80 columns.

And, the issue summary needs an update. The proposed resolution needs to be completed, and the remaining tasks are out of date. And since this needs screenshots, the latest ones should be in the IS as well.

Thanks.

Here I have addressed line wrapping for both Drupal-9.2.x and Drupal-8.9.x.

Hi

I was using this patch for my project. After applying patch #36, the error message has been fixed but the field still doesnt gets highlighted. Will the field highlighting be fixed in this issue?

Thanks!

Confirming that the required indicator, "*" is still missing from the form in 8.9 but that the error message is as expected.

When I applied the 8.9 patch in #36, it also created an extraneous directory with an identical test in core/b/core/modules/media_library/tests/src/Functional/FieldWidgetTest.php.

I believe we could use a test for the required indicator, since it seems potentially fragile.

I didn't run the changes through code sniffer.

Ref #36 patch - 3160238-36_drupal-8_9.patch , @ravi.shankar , I am trying this patch on drupal version 8.9.x , it is giving
skipped patch error. ( Skipped patch 'core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php'.
).It is not getting apply through composer also. please help on this

Adding a patch that will take care of highlighting the target error media widget field. Added a small test for the same.
This patch is implemented on 9.2.x.

Thanks

Thanks for the patch, @KapilV! In the future, could you also leave a short comment explaining what you changed? That will help reviewers stay oriented and on top of the most recent changes. Thank you :)

@KapilV Please add interdiff also, It will help to understand the changes you have done. Thanks

Fixed the failed tests. Please have a look.

On my local machine, I installed Drupal version 9.2.0-dev and try to reproduce the above issue.
For me, without applying the patch I am able to see

1. "Media Fields field is required." error message and
2. Required field highlight in red color.

Note: Image field name is 'Media Fields'. For reference please check the attached screenshot.

I am able to reproduce this issue in fresh installed Drupal version 9.0.2-dev. I have successfully applied the patch #46 and it is working as expected.
Adding screenshots for reference.

I think I'd like to give this a thorough review before RTBC; I'm not 100% certain about the approach, and I'm also unclear as to why we're adding libraries.

I'm changing this issue to a merge request, since it's a lot easier to review changes on Gitlab than in a patch. Let's use that instead. If you aren't familiar with the merge request workflow, there is documentation available at https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa....

Let's stop using patches from here on out. :)

Did a quick review. I think that the main complaint I have is that we shouldn't be doing anything related to CSS, libraries, or presentation in this merge request. All of that stuff is outside the scope of the form system, which I suspect is where the problem is originating. So let's revert all of those changes, and then we'll have a tighter, more focused set of changes to iterate on. :)

I removed the CSS changes from the MR as mentioned in #54.

This reverts the functionality that a required media library field is higlighted with a red border.

EDIT: Never mind, had an idea but it turns out it won't work.

I don't think this needs the explicit subsystem maintainer review tag anymore.

A couple of outstanding requests for the MR. :)

Thanks, @JeroenT! This is definitely looking pretty good. I made a few more suggestions, plus one very easy change :)

Back to you! I think this is getting really close. I think I know why tests are failing, too, so kicking back for that to get fixed.

Only thing left is to move the test to a separate test class. Will try to do this on monday, but if someone can fix this before, feel free to do so.

I reverted the test change and moved it back to a separate functional test.

MR no longer applied, so I merged 9.2.x back into the MR.

I noticed that saving a required media field, even when the field contained a value, stopped working.

This was because $field_state['items_count'] returned the number of widgets shown, which on creating a node was 0 and when updating an existing field 1. I updated the code to count($field_state['items']).

I also moved the test back to a JS test and added the check if a node can be saved after adding a media item.

Verified and tested patch MR !607 mergeable (i.e. https://git.drupalcode.org/project/drupal/-/merge_requests/607.patch) on the drupal 9.3.x-dev version. Patch applied successfully.

Testing Steps:

1. Add media field in page content type, and mark this as required.

1. Try creating page content, fill title & body and leave media as blank.

1. Click Save button.

1. It will give you message "This value should not be null"

Expected Result:

1. It should give proper message like "Media field is required."

1. It should also highlight the field which has error.

Actual Results:

1. It gives message "Media field is required".

Observation:

1. When the error message is displayed, the field is not getting highlighted in red.

Please refer attached screenshot.
Moving this ticket to needs works.

It should also highlight the field which has an error.

An earlier version of the patch did originally do this, but I asked for those changes to be reverted because they were relatively complicated and involved the front-end, which would make it much harder to get this patch fully reviewed and committed. As far as I know, core doesn't really have a way to mark an entire fieldset as having an error, and it is not in scope of this issue to fix that. Maybe open a separate issue to address that generically?

Yes, I was thinking the same. I created #3222368: Make it possible to highlight a fieldset when containing an error. We can decide there if choose a generic approach or fix this specifically for this use case.

Welp, I'm not seeing anything objectionable here. Posted two nitpicks, but by no means should those block commit.

The only reason I'm kicking this back is because this issue has no summary, and we kinda need one in order for this to land. :)

Okay, fixed the IS. Since the nitpicks I found shouldn't block anything, I'm kicking this to RTBC. I've also given it a quick manual test and confirmed that it works as intended.

Committed/pushed to 9.3.x, thanks! Also think the new method on the widget class is innocuous enough to backport this to 9.2.x too, so have cherry-picked there.

For me, those commits seem to introduce a new problem.

I have a node type with a paragraph field. This paragraph has 2 image fields and 1 text field. The 2 image fields were not required and use the Media Library Widget.

With field_permissions, I've set this field to be editable only by certain roles while the others can view the values:
- Create own value for field field_before_after: editor
- Edit anyone's value for field field_before_after : editor
- View anyone's value for field field_before_after: anyone (anonymous + authenticated)

Up to now I had no problems. Since 9.2.3 I'm now having 2 errors saying that my 2 image fields are required, when editing a node that has a referenced paragraph already typed in by an editor.

Not sure but highly probable those commits do more than they should...

Can confirm that I also have issue mentioned in the #82 comment.

It looks like the fix caused the new issue related to Content translation.
STR
1. Add Media field to any Content Type as required.
2. Add another language /admin/config/regional/language
3. Create content of this CT
4. Uncheck the field from translation settings /admin/config/regional/content-language and select "Hide non translatable fields on translation forms" for the CT
5. Add another translation to #3 node
6. Get error "Field_name is required."

The new issue has been created https://www.drupal.org/project/drupal/issues/3226791

I can confirm that the patch also applies on 10.0.8 and resolves the issue. Thank you.