-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Attachments REST API: Add support for filtering attachments by multiple media types #10054
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attachments REST API: Add support for filtering attachments by multiple media types #10054
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
andrewserong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing well for me and generally looks like a good idea, especially for a DataViews-powered media library that would need this in order to be able to show multiple media types at the same time.
I left a comment, but I'm wondering if we could consolidate the behaviour to the existing media_type arg, but set it to allow either a string or an array, rather than adding the extra arg?
src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php
Outdated
Show resolved
Hide resolved
Strongly suggest the same. No need for a new param. |
andrewserong
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing great for me, I also tested manually within a JS context by running the following from the console and the responses all looked correct:
wp.data.select( 'core' ).getEntityRecords( 'postType', 'attachment', { media_type: 'image' } );
wp.data.select( 'core' ).getEntityRecords( 'postType', 'attachment', { media_type: ['image', 'audio'] } );
LGTM, but I also understand if you'd like to get an approving review from folks more experienced with the REST API!
|
I completely missed https://core.trac.wordpress.org/ticket/63668 and #9211 Looks like #9211 handles I'll ping over there to see if work can carry on in that PR. If, for any reason, it cannot, I'll bring the changes over here and assign props to @himanshupathak95 |
| 'type' => 'string', | ||
| 'enum' => array_keys( $media_types ), | ||
| 'description' => __( 'Limit result set to attachments of a particular media type or media types.' ), | ||
| 'oneOf' => array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array may already accept strings so oneOf and string type may not be needed? Not sure about this, please check or maybe @TimothyBJacobs can confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @adamsilverstein 🙇🏻
I think #9211 might be the one to follow since it was up first and takes care of mime_types too.
I'll update #9211 as required if the author can't.
I forgot to close this one, sorry!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I can't push to that branch so I'll move it over here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array may already accept strings so oneOf and string type may not be needed?
The type has changed to 'type' => array( 'string', 'array' ), for simplification. Is that okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array may already accept strings so oneOf and string type may not be needed?
The type has changed to
'type' => array( 'string', 'array' ),for simplification. Is that okay?
right, and it may be possible to simplify further as type => 'array' because I believe array will also accept a string and convert it to a single element array. Not certain about this (its from memory), the unit tests should be able to confirm if I am right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, that works great. It looks like rest_sanitize_array calls wp_parse_list, which always processes comma-separated strings into arrays during sanitization.
So this will coerce the value to an array. I'll update.
Thanks for nudging me in that direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep exactly, just type of array is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming @TimothyBJacobs - I know you pointed this out to me recently on another ticket, but appreciate the confirmation and taking the time to review here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the help on this one, folks! 🙇🏻
18e0b5b to
ea0c42c
Compare
|
I've based this PR on @himanshupathak95's work over in #9211 I think it's ready for another look - the base functionality hasn't changed, except for the additional feature of being able to filter by multiple mime types AND multiple media types or a combination of both. |
src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php
Outdated
Show resolved
Hide resolved
|
This is still testing well for me 👍 |
|
This is still looking good to me after the latest changes 👍 |
|
Thanks @andrewserong! I'll let it settle a day or two then merge in it. |
…types. This update introduces a new `media_types` parameter to the WP REST Attachments Controller, allowing users to filter attachment queries by an array of media types. Additionally, the existing `media_type` parameter remains functional, ensuring backward compatibility. `post_mime_type` already supports multiple mime types.
…ket reference 64046.
…types in the WP REST Attachments Controller. Updated related tests to verify functionality and ensure backward compatibility.
…ents Controller to support array values for filtering. Updated tests to validate functionality and ensure compatibility with existing features. Pulling over work from WordPress#9211 Props to @himanshupathak95
…troller to enforce array input. Updated tests to validate new behavior and ensure proper error responses for invalid parameters.
…on from schema definition. Updated related tests and fixtures to reflect this change.
5ee634b to
c43d2dc
Compare
|
Committed in r60917 |
This patch carries on from #9211
Additionally, this patch add further tests to check that requests containing both
media_typeandmime_typevalues return as expected.Trac ticket: https://core.trac.wordpress.org/ticket/63668
Props to @himanshupathak95
Testing
npm run test:php -- --filter WP_Test_REST_Attachments_ControllerIn the block editor, some example core data calls to the REST API: