Problem/Motivation

When inserting selected media there are scenarios where bundle labels cannot be generated resulting in AJAX errors and the users inability to insert their selected image. 

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /layout_builder/update/block/overrides/node.1/0/0/b7bf67d6-53e6-4229-a2fb-2649a27c1646?destination=/node/1/layout&_wrapper_format=drupal_dialog.off_canvas&ajax_form=1
StatusText: OK
ResponseText: Warning: array_map(): Argument #2 should be an array in /app/web/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php on line 826

As noted the 2nd argument to the `array_map` function on this line is not always an array which results in the error.

Steps to reproduce

Simple

  1. Do a standard install
  2. Enable media and media_library
  3. Add a new Media field to Basic page content, but do NOT select any Media types to reference.This means you cannot save the settings form, but that's ok. The field has been created at this point.
  4. Create a new Basic page and use Media Library (which should be set by default) to add Media to your new field.
  5. View the logs and note the php warning above

Original

  1. Create a custom block type with a media field with a form display of "Media Library". Note: do not limit by bundle (ie: media type)
  2. Create a new content type that uses `layout_builder`
  3. Create a new node using the content type you've created and save.
  4. Click the layout action tab to modify the layout for your node.
  5. Create a new section and add your new block type
  6. Click the add media in the add block form to upload your media.
  7. When attempting to "insert" media a console error should be generated. Note: the media entity will be created just not inserted.

Proposed resolution

The 2nd argument passed to `array_map` needs a fallback to an empty array if the field is not limited by media type.

Remaining tasks

Review provided patches.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#23 interdiff_12-23.txt937 bytesdanflanagan8
#23 3190261-23.patch1.99 KBdanflanagan8
#21 interdiff_12-21.txt973 bytesdanflanagan8
#21 3190261-21.patch2.03 KBdanflanagan8
#15 interdif_9-15.txt1.05 KBdanflanagan8
#15 3190261-15.patch2.12 KBdanflanagan8
#12 interdiff_11-12.txt763 bytesdanflanagan8
#12 3190261-12-FAIL.patch1.87 KBdanflanagan8
#11 3190261-11-FAIL.patch1.79 KBdanflanagan8
#9 3190261--9--FAIL.patch1.08 KBdanflanagan8
#4 media_library--insert-selected-results-in-ajax-error--3190261--1.patch1.02 KBdoostinharrell
#2 media_library--insert-selected-results-in-ajax-error--3190261.patch1.05 KBdoostinharrell

Issue fork drupal-3190261

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
    Support from Acquia helps fund testing for Drupal Acquia logo

    Comments

    doostinharrell created an issue. See original summary.

    doostinharrell’s picture

    doostinharrell CreditAttribution: doostinharrell at Avid New Media commented
    FileSize
    media_library--insert-selected-results-in-ajax-error--3190261.patch1.05 KB
    larowlan’s picture

    larowlan
    Location 🇦🇺🏝.au GMT+10
    CreditAttribution: larowlan at PreviousNext commented
    Status: Needs review » Needs work
    Issue tags: -media library, -Layout Builder +Bug Smash Initiative, +Needs tests

    Thanks for the detailed report

    We can use ?? here to make the solution even simpler

    Can we get a test demonstrating the issue?

    doostinharrell’s picture

    doostinharrell CreditAttribution: doostinharrell at Avid New Media commented
    FileSize
    media_library--insert-selected-results-in-ajax-error--3190261--1.patch1.02 KB
    doostinharrell’s picture

    doostinharrell CreditAttribution: doostinharrell at Avid New Media commented

    @larowlan I have uploaded an updated patch with your recommendation.

    doostinharrell’s picture

    doostinharrell CreditAttribution: doostinharrell at Avid New Media commented
    Status: Needs work » Needs review
    larowlan’s picture

    larowlan
    Location 🇦🇺🏝.au GMT+10
    CreditAttribution: larowlan at PreviousNext commented

    Thanks, we just need a test to demonstrate the bug and that it's fixed

    idebr’s picture

    idebr CreditAttribution: idebr at iO commented

    In the user interface 'Media type' is a required field when configuring a Media field. Perhaps the current code is working as intended?

    danflanagan8’s picture

    danflanagan8
    Primary language English
    Location St. Louis, US
    CreditAttribution: danflanagan8 at Nerdery commented
    FileSize
    3190261--9--FAIL.patch1.08 KB

    In the user interface 'Media type' is a required field when configuring a Media field

    This is true, but you don't have to save those field settings when creating the field, leaving the media type unrestricted. This situation is tested for in media_library using a field called 'field_null_types_media' defined in its test module. So I think we can try to use that field to expose this bug.

    I thought I'd be able to expose the bug by just inserting a media item into that field as part of EntityReferenceWidgetTest. It doesn't seem to be failing. Maybe because it's just a php warning and not a true error? Not sure. I'm attaching my attempt at a fail test just to maybe get the ball rolling. And I'm holding out hope that maybe the test runner here is configured differently from mine and will end up flagging the warning.

    For the record, I was able to reproduce the warning manually, even without layout builder. I never got anything fatal. Just a warning in the logs.

    danflanagan8’s picture

    danflanagan8
    Primary language English
    Location St. Louis, US
    CreditAttribution: danflanagan8 at Nerdery commented
    Status: Needs review » Needs work

    Well, the fail test passed, which is a failure. And I saw no evidence of warnings getting logged or anything like that. I'll change to needs work. I'm wondering what ideas other people might have.

    danflanagan8’s picture

    danflanagan8
    Primary language English
    Location St. Louis, US
    CreditAttribution: danflanagan8 at Horizontal commented
    Version: 8.9.x-dev » 9.3.x-dev
    Status: Needs work » Needs review
    FileSize
    3190261-11-FAIL.patch1.79 KB

    I stumbled across this issue again and I think I have a fail test for it.

    The difficulty here is that we are testing for a php warning (not a fatal error or exception) in an ajax callback. I've had trouble testing for warnings like this before (see https://www.drupal.org/project/drupal/issues/3106460#comment-14213609).

    I modeled the test here after something I found in core: https://git.drupalcode.org/project/drupal/-/blob/9.3.x/core/modules/syst...

    So it's ugly, but not without precedent.

    danflanagan8’s picture

    danflanagan8
    Primary language English
    Location St. Louis, US
    CreditAttribution: danflanagan8 at Horizontal commented
    FileSize
    3190261-12-FAIL.patch1.87 KB
    interdiff_11-12.txt763 bytes

    I canceled the test for the patch above because I came up with a slightly more convincing version by making it clear that only relevant php errors would be found. I like this a lot better.

    Status: Needs review » Needs work

    The last submitted patch, 12: 3190261-12-FAIL.patch, failed testing. View results

    danflanagan8’s picture

    danflanagan8
    Primary language English
    Location St. Louis, US
    CreditAttribution: danflanagan8 at Horizontal commented

    Hmmm, that was weird. The patch in #12 failed with:

    1) Drupal\Tests\media_library\FunctionalJavascript\EntityReferenceWidgetTest::testWidget
"Added one media item." not found
Failed asserting that a boolean is not empty.

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:55
/var/www/html/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTestBase.php:65
/var/www/html/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTestBase.php:370
/var/www/html/core/modules/media_library/tests/src/FunctionalJavascript/EntityReferenceWidgetTest.php:149
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:722

    I'm surprised that happened in #12 but not in #9, which was run 10 months ago against 8.9.x. Is there something different with test configuration in the last 10 months?

    I'm going to re-test the patch on #9 with D9.3.x and see if I get the same failure.

    danflanagan8’s picture

    danflanagan8
    Primary language English
    Location St. Louis, US
    CreditAttribution: danflanagan8 at Horizontal commented
    Status: Needs work » Needs review
    Issue tags: -Needs tests
    FileSize
    3190261-15.patch2.12 KB
    interdif_9-15.txt1.05 KB

    Yes, re-running #9 on 9.3.x resulted in the same error, which was the error I was hoping to trigger 10 MONTHS AGO!!! Apparently I was ahead of my time. ;)

    So here I add the fix from #2 to test against the fail test.

    Although the fix in #4 looks more elegant, it didn't fix the problem for me when I was testing manually. Apparently $element['#target_bundles'] is FALSE instead of NULL. Maybe it's a side-effect of NestedArray::getValue?

    danflanagan8’s picture

    danflanagan8
    Primary language English
    Location St. Louis, US
    CreditAttribution: danflanagan8 at Horizontal commented
    Issue summary: View changes

    I updated the IS with a "Simple" set of steps to reproduce. It doesn't involve Layout Builder, which makes it a tad easier.

    paulocs’s picture

    paulocs
    Primary language Portuguese, Brazil
    Location Belo Horizonte
    CreditAttribution: paulocs at CI&T commented
    Assigned: Unassigned » paulocs

    I'll do a review.

    paulocs’s picture

    paulocs
    Primary language Portuguese, Brazil
    Location Belo Horizonte
    CreditAttribution: paulocs at CI&T commented
    Assigned: paulocs » Unassigned
    Status: Needs review » Reviewed & tested by the community

    Patch looks good and it has tests.
    I confirm that the warning message is correctly fixed.

    Thanks @danflanagan8

    lauriii’s picture

    lauriii
    he/him
    Primary language Finnish
    Location Finland
    CreditAttribution: lauriii at Acquia commented
    Status: Reviewed & tested by the community » Needs review
    Issue tags: +Needs subsystem maintainer review

    Tagging for subsystem maintainer review to get them to weigh on #8.

    phenaproxima’s picture

    phenaproxima
    he/him
    Primary language English
    Location Massachusetts
    CreditAttribution: phenaproxima at Acquia commented
    Issue tags: -Needs subsystem maintainer review

    I agree with @danflanagan8's assessment in #9. Media Library supports the field not specifying any target bundles, and we do test for that, and therefore I think this bug fix is the correct approach.

    However, reviewing the code, I'm wondering something: in MediaLibraryWidget::formElement(), I see this line:

    '#target_bundles' => $settings['target_bundles'] ?? FALSE,

    It's not explained why #target_bundles is set to FALSE, rather than just an empty array. The rest of the code which uses this property basically expects an array, and in fact could be simplified by it. So maybe it would make sense to just change this line to $settings['target_bundles'] ?? [], and add a comment explaining why we're doing it? That way we don't have to deal with FALSE, which as far as I can tell just confuses things.

    danflanagan8’s picture

    danflanagan8
    Primary language English
    Location St. Louis, US
    CreditAttribution: danflanagan8 at Horizontal commented
    FileSize
    3190261-21.patch2.03 KB
    interdiff_12-21.txt973 bytes

    Here's a patch with that updated approach suggested by @phenaproxima. The interdiff is the fix. I'm having trouble running tests locally, so I'm not totally sure how this will turn out. It smells right to me, but we'll see.

    phenaproxima’s picture

    phenaproxima
    he/him
    Primary language English
    Location Massachusetts
    CreditAttribution: phenaproxima at Acquia commented
    Version: 9.3.x-dev » 9.4.x-dev
    danflanagan8’s picture

    danflanagan8
    Primary language English
    Location St. Louis, US
    CreditAttribution: danflanagan8 at Horizontal commented
    FileSize
    3190261-23.patch1.99 KB
    interdiff_12-23.txt937 bytes

    Re-roll of #21. Interdiff is relative to the fail test such that it shows the fix, which is the new approach suggested in #20.

    NOTE: The first commit number in the interdiff filename is wrong. It should be 9, not 12. The contents of the interdiff are correct. Sorry for the confustion.

    danflanagan8’s picture

    danflanagan8
    Primary language English
    Location St. Louis, US
    CreditAttribution: danflanagan8 at Horizontal commented

    That looks like a good result.

    Also note that the fail patch in #9 was triggered manually to re-run in 9.4.x. It applied and failed as expected. (https://www.drupal.org/pift-ci-job/2238945)

    chr.fritsch’s picture

    chr.fritsch
    he/him
    Primary language German
    Location 🇩🇪🇪🇺🌍
    CreditAttribution: chr.fritsch at Thunder commented
    Status: Needs review » Reviewed & tested by the community

    Looks good to me. Thx @danflanagan8

    phenaproxima’s picture

    phenaproxima
    he/him
    Primary language English
    Location Massachusetts
    CreditAttribution: phenaproxima at Acquia commented
    Title: Media Library Insert Selected results in AJAX Error » MediaLibraryWidget can trigger an AJAX error if all media types can be referenced

    Re-titling for clarity.

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 23: 3190261-23.patch, failed testing. View results

    Spokje’s picture

    Spokje
    Primary language Dutch
    CreditAttribution: Spokje commented
    Status: Needs work » Reviewed & tested by the community

    Back to RTBC per #25 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.

    • catch committed 458aa2c on 10.0.x 
      Issue #3190261 by danflanagan8, doostinharrell, phenaproxima, larowlan,...
    • catch committed 47d0aa0 on 9.3.x
      Issue #3190261 by danflanagan8, doostinharrell, phenaproxima, larowlan,...
    • catch committed 834c16c on 9.4.x
      Issue #3190261 by danflanagan8, doostinharrell, phenaproxima, larowlan,...
    catch’s picture

    catch
    he/him
    Primary language English
    CreditAttribution: catch at Third and Grove commented
    Version: 9.4.x-dev » 9.3.x-dev
    Status: Reviewed & tested by the community » Fixed

    Committed/pushed to 10.0.x, cherry-picked to 9.4.x and 9.3.x, thanks!

    Status: Fixed » Closed (fixed)

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