Skip to content

Conversation

@danielbachhuber
Copy link
Member

@danielbachhuber danielbachhuber commented Nov 9, 2018

Description

Switches wp.media.query to use posts_per_page=-1 and fetch all attachments when opening a gallery in the Media Library.

See #10873 (comment)

How has this been tested?

  1. Create a Gallery Block and click Edit:

image

  1. Inspect the Network Tab and verify that posts_per_page=-1 was used in the request and that the request was successful:

image

@danielbachhuber danielbachhuber added [Type] Bug An existing feature does not function as intended [Feature] Block API API that allows to express the block paradigm. [Feature] Media Anything that impacts the experience of managing media REST API Interaction Related to REST API labels Nov 9, 2018
@danielbachhuber danielbachhuber added this to the 4.4 milestone Nov 9, 2018
@adamsilverstein
Copy link
Member

Why are we using unbounded queues vs pagination?

@danielbachhuber
Copy link
Member Author

Why are we using unbounded queues vs pagination?

Isn't this what core currently does for wp.media.query ?

@joemcgill
Copy link
Member

I just double checked, and can confirm that in the classic editor we are including posts_per_page=-1 when the query-attachments action is called to get data for a gallery. In this case, it seems to make sense, because we are pairing that with a post__in value containing an array of all image IDs in the gallery.

  1. I don't see anything in this PR that limits this to only apply to galleries, but I think it's a reasonable expectation that we would fetch all of the attachment data needed by any block that is saving a set of attachment IDs.
  2. wp.media.query actually uses perPage as the property instead of per_page, which gets mapped to posts_per_page here. So good catch that it was wrong. Changing it to posts_per_page directly should work fine, however.

orderby: 'post__in',
post__in: ids,
per_page: 100,
posts_per_page: -1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in my comment, wp.media.query() normally uses perPage instead of posts_per_page, but either should work and I slightly prefer the fact that this is consistent with the WP_Query args that are more familiar.

TL;DR – LGTM.

@danielbachhuber danielbachhuber merged commit 29a2b7f into master Nov 9, 2018
@danielbachhuber danielbachhuber deleted the 10873-fetch-attachments branch November 9, 2018 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Block API API that allows to express the block paradigm. [Feature] Media Anything that impacts the experience of managing media REST API Interaction Related to REST API [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants