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

Comments

DannyBoing created an issue. See original summary.

almunnings’s picture

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

    $opener_parameters = array_filter($opener_parameters, function ($value) {
      return !empty($value);
    });

Seems to fix it up.
So it's would appear to be the entity_id not getting hashed.

almunnings’s picture

I'm guessing the source of the issue is actually MediaLibraryWidget.php:

    $state = MediaLibraryState::create('media_library.opener.field_widget', $allowed_media_type_ids, $selected_type_id, $remaining, [
      'field_widget_id' => $field_widget_id,
      'entity_type_id' => $entity->getEntityTypeId(),
      'bundle' => $entity->bundle(),
      'field_name' => $field_name,
      // The entity ID needs to be a string to ensure that the media library
      // state generates its tamper-proof hash in a consistent way.
      'entity_id' => (string) $entity->id(),
    ]);

On a new entity, entity_id is empty string? Empty params dont get rendered into URLs for pagination?

seanB’s picture

Version: 8.7.4 » 8.8.x-dev
Status: Active » Needs review
Issue tags: +Media Initiative
FileSize
7.18 KB
8.92 KB

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

seanB’s picture

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

The last submitted patch, 4: 3067041-4-fail.patch, failed testing. View results

The last submitted patch, 5: 3067041-5-fail.patch, failed testing. View results

The last submitted patch, 4: 3067041-4.patch, failed testing. View results

almunnings’s picture

#5 working fine for me.

phenaproxima’s picture

Status: Needs review » Needs work

This seems pretty good to me!

  1. +++ b/core/modules/media_library/src/Plugin/Field/FieldWidget/MediaLibraryWidget.php
    @@ -464,15 +464,19 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +    if ($entity->id()) {
    

    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 use isset($entity->id()), since the ID can never be null.

  2. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -877,6 +817,128 @@ public function testWidget() {
    +    // Visit a node create page.
    +    $this->drupalGet('node/add/basic_page');
    

    Nit: I don't think we need the comment. :)

  3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
    @@ -877,6 +817,128 @@ public function testWidget() {
    +    $checkboxes = $page->findAll('css', '.media-library-view .js-click-to-select-checkbox input');
    +    $this->assertCount(24, $checkboxes);
    

    These two lines could just be $assert_session->elementsCount().

dhirendra.mishra’s picture

Assigned: Unassigned » dhirendra.mishra
dhirendra.mishra’s picture

Assigned: dhirendra.mishra » Unassigned
Status: Needs work » Needs review
FileSize
12.54 KB
3.24 KB

Please find patch.

seanB’s picture

danthorne’s picture

#13 seems to have fixed the issue :)

phenaproxima’s picture

Title: Media library pagination ajax error » Media library pagination is broken due to invalid entity_id parameter
Status: Needs review » Reviewed & tested by the community

Re-titling and RTBCing. Thanks!

phenaproxima’s picture

Issue tags: +backport

Also, I think this patch should be backported to 8.7.x since the same bug likely exists there.

handspiker’s picture

Backported patch

webchick’s picture

Status: Reviewed & tested by the community » Fixed

This 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:

+    // Only add the entity ID when we actually have one. The entity ID needs to
+    // be a string to ensure that the media library state generates its
+    // tamper-proof hash in a consistent way.
+    if (!$entity->isNew()) {
+      $opener_parameters['entity_id'] = (string) $entity->id();
+    }

...

+    // Assert the pager works as expected.
+    $assert_session->elementTextContains('css', '.media-library-view .pager__item.is-active', 'Page 1');
+    $assert_session->elementsCount('css', '.media-library-view .js-click-to-select-checkbox input', 24);
+    $page->clickLink('Next page');
+    $assert_session->assertWaitOnAjaxRequest();
+    $assert_session->elementTextContains('css', '.media-library-view .pager__item.is-active', 'Page 2');
+    $assert_session->elementsCount('css', '.media-library-view .js-click-to-select-checkbox input', 1);
+    $page->clickLink('Previous page');
+    $assert_session->assertWaitOnAjaxRequest();
+    $assert_session->elementTextContains('css', '.media-library-view .pager__item.is-active', 'Page 1');
+    $assert_session->elementsCount('css', '.media-library-view .js-click-to-select-checkbox input', 24);

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!

  • webchick committed dfbde86 on 8.8.x
    Issue #3067041 by seanB, dhirendra.mishra, Al Munnings, handspiker,...

  • webchick committed fc7c309 on 8.7.x
    Issue #3067041 by seanB, dhirendra.mishra, Al Munnings, handspiker,...

Status: Fixed » Closed (fixed)

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