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.
Since upgrading to 8.7.4, when I go to add a media item to a node and try to paginate to the next page I get an ajax error:
An AJAX HTTP error occurred.
HTTP Result Code: 400
Debugging information follows.
Path: /views/ajax?ajax_form=1&_wrapper_format=drupal_ajax&media_library_opener_id=media_library.opener.field_widget&media_library_allowed_types%5Bimage%5D=image&media_library_selected_type=image&media_library_remaining=1&media_library_opener_parameters%5Bfield_widget_id%5D=field_hero_image&media_library_opener_parameters%5Bentity_type_id%5D=node&media_library_opener_parameters%5Bbundle%5D=page&media_library_opener_parameters%5Bfield_name%5D=field_hero_image&media_library_opener_parameters%5Bentity_id%5D=&hash=0eKv-R7SmGpQCmRSe09EekuC2Mk3pSsaeG1Q1BULICk
StatusText: Bad Request
Comment | File | Size | Author |
---|---|---|---|
#17 | 3067041-17.patch | 11.75 KB | handspiker |
#13 | 3067041-13.patch | 11.73 KB | seanB |
#13 | interdiff-5-13.txt | 2.97 KB | seanB |
#12 | 5-12_interdiff.txt | 3.24 KB | dhirendra.mishra |
#12 | 3067041-12.patch | 12.54 KB | dhirendra.mishra |
Comments
Comment #2
almunningsIm just hitting this issue now.
After a look, it appears to be something to do with entity_id in the request URL being empty on new nodes. The issue doesnt occur on existing nodes.
I did a quick patch in MediaLibraryState...
Seems to fix it up.
So it's would appear to be the entity_id not getting hashed.
Comment #3
almunningsI'm guessing the source of the issue is actually MediaLibraryWidget.php:
On a new entity, entity_id is empty string? Empty params dont get rendered into URLs for pagination?
Comment #4
seanBAttached is a fail patch to prove we have an actual issue, and a patch containing a fix for it in the media library widget.
It might be good to move the tests for the media library views components (like the grid/table links, pager, filters etc) to a separate test, that way we don't have to add all those extra media items in the setup.
Comment #5
seanBCreating all those extra items in setup breaks existing tests, and since testWidget was already testing way too much I think it would be best to move the views related stuff to a separate test. Here is a new fail patch and patch containing the fix.
Comment #9
almunnings#5 working fine for me.
Comment #10
phenaproximaThis seems pretty good to me!
I think this should be
!$entity->isNew()
. Otherwise, we are assuming things about the ID. Granted, it's a safe assumption, but still -- we can usually be certain that only a non-new entity has an ID. Or, alternatively, we could useisset($entity->id())
, since the ID can never be null.Nit: I don't think we need the comment. :)
These two lines could just be $assert_session->elementsCount().
Comment #11
dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commentedComment #12
dhirendra.mishra CreditAttribution: dhirendra.mishra at Valuebound for Valuebound commentedPlease find patch.
Comment #13
seanBFixed #10.
Comment #14
danthorne#13 seems to have fixed the issue :)
Comment #15
phenaproximaRe-titling and RTBCing. Thanks!
Comment #16
phenaproximaAlso, I think this patch should be backported to 8.7.x since the same bug likely exists there.
Comment #17
handspikerBackported patch
Comment #18
webchickThis was a pretty tricky patch to review because of all of the test refactoring. :\ But @phenaproxima and I reviewed the diffs side by side and looks good.
So the "meat" of the patch is mainly:
...
I got a little confused because the comment above the animals array says "We want to have more than 24 items to trigger a pager in the widget view." but there are only 21 animals there (I always count my animals :D). @phenaproxima pointed out that this is because the setUp() method adds an additional 8, but then why would there only be 1 on the following page? The answer is because we're filtering by bundle = type_one up above. Whew!
Ok, so TL;DR, looks good, and let's get this backported to Drupal 8.7 as well since it's a valid bug fix.
Committed and pushed #13 to 8.8.x and cherry-picked to 8.7.x. Thanks!