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
Steps to reproduce:
- Install Media Library module
- Create a media reference field selecting the Reference > Media option in the field type list
- Submit the field storage settings form
Expected result:
The field settings form is shown without errors
Actual result:
The following notice is shown:
Notice: Undefined index: target_bundles in Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget->getAllowedMediaTypeIdsSorted() (line 155...
Proposed resolution
Add check if the variable is exist.
Comment | File | Size | Author |
---|---|---|---|
#43 | 3055516-43.patch | 2.95 KB | oknate |
#43 | 3055516-43--FAIL.patch | 1.96 KB | oknate |
#43 | 3055516--interdiff-36-43.txt | 1004 bytes | oknate |
#36 | 3055516--interdiff-30-36.txt | 2.25 KB | oknate |
#36 | 3055516-36.patch | 2.95 KB | oknate |
Comments
Comment #2
Skymen CreditAttribution: Skymen commentedComment #3
Skymen CreditAttribution: Skymen commentedComment #4
Skymen CreditAttribution: Skymen commentedHere is the correct patch file.
Comment #5
Skymen CreditAttribution: Skymen commentedComment #6
BrightBoldSolved the problem for me, thanks!
Comment #8
yogeshmpawarAll looks good so setting back to RTBC! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest).
Comment #10
Skymen CreditAttribution: Skymen commentedComment #11
Wim LeersBut …
target_bundles
is a required setting:So this shouldn't be necessary. How did you manage to set this up without configuring allowed target bundles?
Comment #12
tstoecklerAdded steps to reproduce. Although the previous description was pretty brief, it was actually correct, this happens when creating the field. Expanded the IS a bit though, to make this more clear. Re #11, that's why this can occur, even though the field is required, because the very first you hit this form the field settings have not yet been saved.
We should add a test, though, before this can go in.
Comment #13
Wim Leers🤯 That sounds like a serious bug in Field UI, not Media? That would also explain many bug reports in JSON:API, to be honest, where JSON:API chokes on this required information not being set.
Comment #14
seanBCan we remove the extra () around the isset() and put this on 1 line? Or better yet, in PHP7 we can also do this:
$allowed_media_type_ids = $handler_settings['target_bundles'] ?? NULL;
Comment #15
phenaproximaFrom a backend perspective, this seems quite minor. Media Library itself handles the various values of target_bundles quite gracefully as of #2989503: Add tests to prove that the media library widget works when target_bundles is NULL and #3033653: InvalidArgumentException when adding reference field without Media type, but I guess we missed an isset() check somewhere. If it's easy, go ahead and add a small test (if we're lucky, it could be a one-line change, asserting that the notice is not there when creating a new field).
Comment #16
Chris Matthews CreditAttribution: Chris Matthews commentedInterestingly, when adding a new entity reference field...
If you select Reference > Media then this notice occurs
However, if you select Reference > Other... > Type of item to reference > Media, then this notice does not occur
Comment #17
rgpublicPatch works for me and is badly needed IMHO. Otherwise everything is inundated with these stupid error messages.
#14 is right, of course. Modern PHP exists and should be used. I've adapted the patch accordingly.
Comment #18
anmolgoyal74 CreditAttribution: anmolgoyal74 commentedComment #19
seanBThe patch looks good and I can confirm it fixes the issue. We now only need a test to prove that no error messages are shown when adding media fields.
Comment #20
oknateI'll see if I can write a test this morning.
Comment #21
oknateHere's test coverage.
Comment #22
oknateComment #23
oknateRemoving an extraneous permission.
Comment #25
seanBThanks, looking good to me. One minor thing though.
We already have test coverage for this in
MediaLibraryTest::testWidget()
.I'm not against moving that coverage to the new
testFieldUiIntegration
method, but that other test also asserts the configured order is actually used in the widget, so that might be more complete.I personally feel it's best to remove the duplicate test coverage for now, since it is not actually related to the bug we are trying to solve. We could have a follow-up to move some of the test coverage in testWidget, but I have no strong preference regarding that.
Comment #26
oknateOh bummer. I didn't see that test. I'll see if I can combine them and add the test coverage needed here to that one.
Comment #27
oknateHmm, looking. It looks like i can just drop the second part of my test.
Comment #28
phenaproximaLet's make the changes discussed above and add a new FAIL patch to move this along.
Comment #29
dasginganinjaAttached are the removals mentioned by seanB.
I generated these from a fresh 8.8.x branch.
Edit: I must have had some issue with creating these. I'll generate new patches.
Comment #30
dasginganinjaI goofed hard on the patch in #29. I removed those from the file list.
Here are new patches to
* update FAIL from 21 with the 23 interdiff and comments from seanB
* update Success from 23 patch with comments from seanB
Comment #31
oknateComment #33
phenaproximaI found nitpicks, but nothing that should block RTBC. Feel free to fix these if they're not too annoying!
It'd be nice to split these bits up with a blank line, since the setting up and logging in is like the "prologue" of the test, and the real meat of it starts with the drupalGet().
This assertion really...engages me. (See what I did there? Wrong captain, I know. :) 👍🤓
I would change this, I think, to assert that the partial text of the error is not showing up. So, something like:
It's a shame we need to use assertWaitOnAjaxRequest(), which is not a particularly reliable method. However, there's no particular condition or element for us to wait for, so this is fine for now.
I know @seanB asked for this, but I don't think we should use the
??
operator, since that means this patch will not be backportable to Drupal 8.7 (which, IMHO, it absolutely should be). So, ugly as it is, let's revert this to theisset() ? $foo : $bar
syntax instead.Comment #35
phenaproximaFail patch failed. :)
Comment #36
oknateAdding new fail patch to demonstrate this change will fail without bugfix:
1. ✅ Capitalized AJAX.
2. 👍 I thought you’d appreciate that. I don’t see “Picard” in the code base yet. That’ll be in the next generation of patches.
3. updated to
$assert_session->pageTextNotContains('Undefined index: target_bundles’);
4. I know you don't care for these vague assertions, we could test if the form_build_id changes, but that seems silly.
Comment #37
seanBMeh, but excellent point.
Rgarding #33.3
I think since
$assert_session->elementNotExists('css', '.messages--error');
will also catch other error in the future, that might be a bit more robust?Comment #38
oknateRe:
I think phenaproxima is right. What if we need to test error messages on the form, and it makes this test fail?
I guess we could change the test at that point. For now this would be more robust coverage that the form saves without error.
Comment #39
seanBI think the normal path in this test should never produce an error, but I have no strong opinion on it. If we feel more comfortable with the more specific assert I could definitely live with it.
Comment #40
phenaproximaI agree with @seanB's rationale. However, the reason we should still change the assertion is because, if the error message area's CSS selector is ever changed to something besides
.messages--error
, this test will silently cease to test the regression. Using.messages--error
gives us an accidental, unintentional dependency on front end-specific code. So in the name of specificity and future-proofing, we should change the assertion.Comment #43
oknateOops. Should have been isset(), not !empty(), as phenaproxima said in #33.
Comment #44
phenaproximaI'm happy with this. RTBC when it's green.
Comment #45
seanB+1 from me!
Comment #47
catchThe double 'when' is a bit hard to read, will add a comma after 'setting' on commit.
Committed c6a7ce5 and pushed to 8.8.x. Thanks!
Comment #49
catchComment #50
phenaproximaI'm nominating the patch to be backported. I believe it also exists on 8.7.x, and although it's not "serious", per se, it's a big scary WTF for users.
Comment #51
Martijn de WitApplied this patch to 8.7.7 after getting the nice red error :p
Works fine for 8.7.7 as wel. Tests also seems to be green. I don't know what needs to be done for a backport?
Comment #52
phenaproximaWe don't need to do anything to backport this. Tests passed/failed on 8.7.x exactly as expected, so I don't think we're blocked.
8.7.x has been in a commit freeze for a couple of days, I think, so a committer will probably backport this to that branch when the freeze is done.
Comment #53
xjmThis seems fine for backport. (Note that "backport" isn't a tag we use or track.) Patch-eligible issues should just always be set to the production branch (currently 8.7.x). Separate tests can still be run against 8.8.x and 8.7.x if needed.
Comment #54
xjmCherry-picked to 8.7.x. Thanks!