Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
The change in #3160238: Media Library widget produces "This value should not be null" error when field is required makes it impossible to save entity translations that reference Media items, if the Media reference field is hidden and untranslatable.
Steps to reproduce
- Create a
testcontent type with a required Media reference field. - Add a second language to the site.
- Enable content translation for the
testcontent type.- Disable translation of the Media reference field.
- Select the "Hide non translatable fields on translation forms" option.
- Create a new
testnode and attach a Media item to it. - Attempt to translate the page into a second language.
Expected result
The untranslatable Media reference field is hidden from the node translation form, and the node translation can be saved successfully.
Actual result
The untranslatable Media reference field is hidden from the node translation form, and the error "Media field is required." appears when trying to save the node translation. The node translation cannot be saved.
Proposed resolution
Only execute the custom media library widget validator if the field is accessible on the form.
| Comment | File | Size | Author |
|---|---|---|---|
| #39 | 3226791-39-test-only.patch | 4.36 KB | ameymudras |
| #35 | after_patch.png | 202.82 KB | gaurav-mathur |
| #35 | before_patch.png | 167.8 KB | gaurav-mathur |
Issue fork drupal-3226791
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:
changes
Comments
Comment #2
bgreco CreditAttribution: bgreco commentedComment #3
cilefen CreditAttribution: cilefen at Institute for Advanced Study commentedComment #4
dwebpoint CreditAttribution: dwebpoint at EPAM Systems commentedGot the same issue on Drupal 9.2.4.
Comment #5
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot for Vardot commentedTested Patch #2
Working well, Thank you Brad.
Comment #6
sergei_brill CreditAttribution: sergei_brill as a volunteer commentedThis fix needs tests.
Comment #7
sergei_brill CreditAttribution: sergei_brill as a volunteer commentedSince #3160238: Media Library widget produces "This value should not be null" error when field is required adds significant drawback which is more critical that what it fixes (not working translation form is worse than incorrect error message, IMHO), I suggest to revert commit from #3160238: Media Library widget produces "This value should not be null" error when field is required and start new issue/reopen #3160238: Media Library widget produces "This value should not be null" error when field is required.
Put major priority according to https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-...
Comment #8
efrainhI had the same issue on Drupal 9.2.6, I tested this patch and it's working. Thanks @bgreco.
Changing status to Reviewed & tested by the community.
Comment #9
szeidler CreditAttribution: szeidler at Ramsalt Lab commentedThanks for the patch. It's fixing the bug introduced in the other issue. It's quite painful since it easily can happen for translated content with a untranslatable media field, which is a quite common use case.
Comment #10
phenaproximaGreat catch on this bug, and nice fix!
However, this will absolutely need an automated test before it can be committed. Self-assigning to get that done, and re-targeting this against 9.3.x (it can be backported to 9.2.x at committer discretion). I'm also escalating the severity of this bug, since I agree that this is quite a painful problem for anyone who wants to use media in a multilingual site.
Let's see what I can do...
Comment #12
phenaproximaOK! I wrote a test which fails without the patch, and passes with it. Let's see if testbot agrees. In the meantime, I'm hiding the patch file in favor of a merge request.
Comment #13
seanBSetting it back to needs work since the fail commit didn't seem to fail.
Comment #15
larowlan💄This issue was today's Bug Smash Initiative daily triage issue
Removed some unrelated composer changes (related to tuf, so guessing automated updates) from the MR, rebased and force-pushed
Comment #16
kbrodej CreditAttribution: kbrodej at Agiledrop - Your Trusted Drupal Teammates commentedTested this on 9.2.9 version. Works flawlessly.
Comment #18
mqannehTested on 9.2.9, it works and fixed the issue.
Comment #19
quietone CreditAttribution: quietone at PreviousNext commentedI applied the patch and tested on Drupal 9.4.x, standard install, using the steps given in the IS. I confirmed the problem exists without the patch and that the patch fixes it. The test looks good to me and locally fails without the fix.
The only thing missing here is a failing test.
Comment #20
wwedding CreditAttribution: wwedding at Sticky Co commentedI'm in a hotfix situation so I don't have time to run the test myself, but what is the issue with the current test in #14?
Comment #21
phenaproximaThe problem is that we need a test which fails if the bug fix isn't there, which would prove that the bug fix actually fixes the bug. I tried to write one, but it didn't fail for some reason, so apparently it's not testing the faulty code path.
Comment #23
aschiwi CreditAttribution: aschiwi at undpaul commentedThe patch in comment #2 solves the problem for us, whereas the MR does not apply.
Comment #25
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedThe PR wasn't getting applied, I did rebase the PR against 9.5.x but I am unable to switch the PR base to 9.5.x
Comment #26
anup.singh CreditAttribution: anup.singh as a volunteer and for Faichi Solutions Pvt Ltd commentedMerge requests started breaking the composer, so adding a patch that works with 9.5.x
Comment #27
anup.singh CreditAttribution: anup.singh as a volunteer and for Faichi Solutions Pvt Ltd commentedComment #29
sjerdoSince the target branch of merge request 1312 cannot be changed to 9.5.x, I restored the MR to the commits for 9.3.x and created a new branch + merge request for 9.5.x
Comment #31
DeepaliJ CreditAttribution: DeepaliJ at Salsa Digital commentedComment #33
DeepaliJ CreditAttribution: DeepaliJ at Salsa Digital commentedComment #34
Manibharathi E R CreditAttribution: Manibharathi E R at Srijan | A Material+ Company for Srijan | A Material+ Company commentedPatch #26 Applied and Tested successfully on Drupal 9.5.x.
Before Patch:
After Patch:
Comment #35
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedApplied patch #26 working fine on Drupal 9.5.x
refer to the screenshot
Comment #36
Nick Dewitte CreditAttribution: Nick Dewitte commentedWe've ran into a similar issue with the layout builder, where submitting a layout builder behaviors form triggered the same "[field_name] field is required" error.
Applied the changes from MR 2759 to our 9.4 install, and this resolved the issue.
Comment #37
smustgrave CreditAttribution: smustgrave at Mobomo commentedMR should be updated.
Test-only patch would be useful as well.
This ticket only needed 1 set of screenshots being #34 so those should be added to the issue summary.
Comment #38
a.dmitriiev CreditAttribution: a.dmitriiev as a volunteer and at 1xINTERNET commentedI can confirm that the changes from MR 2759 for Drupal 9.4.8 installation resolved the issue.
Comment #39
ameymudras CreditAttribution: ameymudras at Salsa Digital commentedComment #41
Sharique CreditAttribution: Sharique as a volunteer and for Drupal India Association commentedI tested this PR D10, it works fine.