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

  1. Create a test content type with a required Media reference field.
  2. Add a second language to the site.
  3. Enable content translation for the test content type.
    1. Disable translation of the Media reference field.
    2. Select the "Hide non translatable fields on translation forms" option.
  4. Create a new test node and attach a Media item to it.
  5. 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.

CommentFileSizeAuthor
#39 3226791-39-test-only.patch4.36 KBameymudras
#35 after_patch.png202.82 KBgaurav-mathur
#35 before_patch.png167.8 KBgaurav-mathur
#34 After-Patch-3226791.png441.47 KBManibharathi E R
#34 before-Patch-3226791.png270.92 KBManibharathi E R
#26 3226791.patch5.24 KBanup.singh
#2 media_required_validation-3226791-2.patch899 bytesbgreco

Issue fork drupal-3226791

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

bgreco created an issue. See original summary.

bgreco’s picture

bgreco commented
StatusFileSize
new media_required_validation-3226791-2.patch899 bytes
cilefen’s picture

cilefen commented
Status: Active » Needs review
dwebpoint’s picture

dwebpoint commented
Version: 9.3.x-dev » 9.2.x-dev

Got the same issue on Drupal 9.2.4.

rajab natshah’s picture

rajab natshah
he/him
commented

Tested Patch #2
Working well, Thank you Brad.

sergei_brill’s picture

sergei_brill commented
Status: Needs review » Needs work

This fix needs tests.

sergei_brill’s picture

sergei_brill commented
Status: Needs work » Needs review

Since #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-...

efrainh’s picture

efrainh
Primary language Spanish
commented
Status: Needs review » Reviewed & tested by the community

I 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.

szeidler’s picture

szeidler commented

Thanks 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.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Version: 9.2.x-dev » 9.3.x-dev
Assigned: Unassigned » phenaproxima
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Great 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...

phenaproxima opened merge request !1312

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Assigned: phenaproxima » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests

OK! 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.

seanb’s picture

seanb
Primary language Dutch
Location Netherlands
commented
Status: Needs review » Needs work

Setting it back to needs work since the fail commit didn't seem to fail.

larowlan made their first commit to this issue’s fork.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review
Issue tags: +Bug Smash Initiative

💄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

kbrodej’s picture

kbrodej commented

Tested this on 9.2.9 version. Works flawlessly.

Version: 9.3.x-dev » 9.4.x-dev

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.

mqanneh’s picture

mqanneh
he/him
Primary language English
Location Amman
commented

Tested on 9.2.9, it works and fixed the issue.

quietone’s picture

quietone commented
Status: Needs review » Needs work

I 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.

wwedding’s picture

wwedding commented

I'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?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

The 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.

Version: 9.4.x-dev » 9.5.x-dev

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.

aschiwi’s picture

aschiwi commented

The patch in comment #2 solves the problem for us, whereas the MR does not apply.

ameymudras made their first commit to this issue’s fork.

ameymudras’s picture

ameymudras commented

The 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

anup.singh’s picture

anup.singh commented
StatusFileSize
new 3226791.patch5.24 KB

Merge requests started breaking the composer, so adding a patch that works with 9.5.x

anup.singh’s picture

anup.singh commented
Status: Needs work » Needs review

sjerdo made their first commit to this issue’s fork.

sjerdo’s picture

sjerdo
Primary language Dutch
Location Haarlem
commented

Since 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

sjerdo opened merge request !2759

deepalij’s picture

deepalij commented
Assigned: Unassigned » deepalij

Version: 9.5.x-dev » 10.1.x-dev

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.

deepalij’s picture

deepalij commented
Version: 10.1.x-dev » 9.5.x-dev
Assigned: deepalij » Unassigned
Manibharathi E R’s picture

Manibharathi E R commented
StatusFileSize
new before-Patch-3226791.png270.92 KB
new After-Patch-3226791.png441.47 KB

Patch #26 Applied and Tested successfully on Drupal 9.5.x.

Before Patch:
Before-Patch

After Patch:
After-patch

gaurav-mathur’s picture

gaurav-mathur commented
StatusFileSize
new before_patch.png167.8 KB
new after_patch.png202.82 KB

Applied patch #26 working fine on Drupal 9.5.x
refer to the screenshot

nick dewitte’s picture

nick dewitte commented

We'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.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

MR 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.

a.dmitriiev’s picture

a.dmitriiev commented

I can confirm that the changes from MR 2759 for Drupal 9.4.8 installation resolved the issue.

ameymudras’s picture

ameymudras commented
Status: Needs work » Needs review
StatusFileSize
new 3226791-39-test-only.patch4.36 KB

Status: Needs review » Needs work

The last submitted patch, 39: 3226791-39-test-only.patch, failed testing. View results

sharique’s picture

sharique commented
Status: Needs work » Reviewed & tested by the community

I tested this PR D10, it works fine.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: 3226791-39-test-only.patch, failed testing. View results

Version: 9.5.x-dev » 11.x-dev

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. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

sjerdo’s picture

sjerdo
Primary language Dutch
Location Haarlem
commented
Status: Needs work » Closed (duplicate)
Related issues: +#3267401: Media library is showing 'is required message' while the user has no access to the field

This is fixed in #3267401

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

sjerdo’s picture

sjerdo
Primary language Dutch
Location Haarlem
commented
Version: 11.x-dev » 9.5.x-dev

sjerdo closed merge request !2759