WordPress.org

Make WordPress Core

#39092 closed enhancement (fixed)

REST API: Add support for filename search in media endpoint

Reported by: jblz Owned by: jnylen0
Milestone: 4.7.1 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests commit fixed-major
Focuses: rest-api Cc:

Description

In [38625], the functionality to search for attachments by filename was added via the _filter_query_attachment_filenames function and exposed via the query-attachments AJAX action.

This applies the same filter function in the same manner in the REST API /media endpoint.

It was necessary to move _filter_query_attachment_filenames from wp-admin/post.php to wp-includes/post.php as the function did not exist in the context of a REST API call to that endpoint.

To test:

  • Change the Title of an attachment to something other than a filename.
  • Attempt to search for the filename via the media?search=... endpoint -- notice it doesn't surface the intended result
  • Apply patch
  • Repeat the search call -- it should surface the intended result

Attachments (6)

39092.diff (3.2 KB) - added by jblz 11 months ago.
39092-test.diff (1.1 KB) - added by tyxla 11 months ago.
Add test for searching media by filename in the media REST API endpoint
39092.2.diff (4.2 KB) - added by jblz 11 months ago.
Remove extra meta clause, move filter application, incorporate test
39092.2.2.diff (4.2 KB) - added by jblz 11 months ago.
Remove extra meta clause, move filter application, incorporate test
39092.3.diff (4.2 KB) - added by jblz 11 months ago.
fix docblock / comment
39092.4.diff (4.5 KB) - added by jnylen0 11 months ago.
More robust test (check post ID)

Download all attachments as: .zip

Change History (23)

@jblz
11 months ago

#1 @jblz
11 months ago

  • Keywords has-patch needs-unit-tests needs-testing added

#2 @jnylen0
11 months ago

There is some discussion on the ticket that introduced this feature for the media library (#22774) about its intended purpose and scope. For example (ticket:22744#comment:32):

visitors likely don't know or care about file names anyway

I could argue this one either way. It's worth considering whether to allow every API user (including unauthenticated) to search filenames. However, we already expose the filenames in the response of /media, and media search in the API should support the same features as the search in the wp-admin media library.

@joemcgill @swissspidy since you worked on adding filename search to the media library, what do you think about supporting this in the API also?

#3 follow-up: @jnylen0
11 months ago

Review comments on 39092.diff:

Normally we would remove this filter after using it, but _filter_query_attachment_filenames removes itself so we are good there.

There's an extra SQL clause added in 39092.diff. OR sq1.meta_key = '_wp_attachment_image_alt' ) -- this could be a good enhancement also, but it needs to be a separate ticket.

Finally we need to only run this code for the attachment post type. The new add_filter call should go in WP_REST_Attachments_Controller->prepare_items_query instead.

#4 follow-up: @joemcgill
11 months ago

I think including attachment filenames in the REST API is a good idea, as I assume at some point we'll likely want to use the REST API to drive search experiences for media rather than relying on the admin-ajax handlers. Thanks for the suggestion and patch @jblz.

A few comments on 39092.diff:

First, I agree with @jnylen0 that adding alt to search should be handled in a separate ticket. Conveniently, that ticket already exists, see: #39004.

Also, It looks like by adding the filter to WP_REST_Posts_Controller::get_items(), the extra JOINs would get added to all search queries and not just attachment search queries. Is that correct? While the code looks identical to the other places where the filter is added, we're only adding that filter within functions that only apply to attachment searches within the admin. I'm not sure if adding some sort of admin context to the REST endpoints makes sense here, but limiting this so the extra joins don't affect regular search queries would be good.

#5 in reply to: ↑ 4 @jnylen0
11 months ago

Replying to joemcgill:

It looks like by adding the filter to WP_REST_Posts_Controller::get_items(), the extra JOINs would get added to all search queries and not just attachment search queries.

Yep, we shouldn't do that, see my note above about moving the filter to WP_REST_Attachments_Controller.

@joemcgill in your opinion is this good enough, or should we only search filenames if context=edit (requires authentication)?

#6 @joemcgill
11 months ago

I think it would be nice if this wasn't limited by context. In [38625], we were being intentionally conservative—partially because targeting only search queries for attachments isn't as clean through regular search queries on the front end. Adding the filter inside the WP_REST_Attachments_Controller sounds like a good plan to me.

#7 @jnylen0
11 months ago

  • Keywords needs-refresh added
  • Milestone changed from Awaiting Review to 4.7.1

Sounds reasonable to me. Let's get this in 4.7.1 after the above comments are addressed, then.

#8 in reply to: ↑ 3 @jblz
11 months ago

Replying to jnylen0:

There's an extra SQL clause added in 39092.diff. OR sq1.meta_key = '_wp_attachment_image_alt' ) -- this could be a good enhancement also, but it needs to be a separate ticket.

Ya, whoops! I was testing out the #39004 patch & missed reverting that bit before making this patch. Good catch!

Thanks for taking a look & for linking to that ticket, @joemcgill. Much appreciated.

Finally we need to only run this code for the attachment post type. The new add_filter call should go in WP_REST_Attachments_Controller->prepare_items_query instead.

That makes sense to me. I'll see about an updated patch with some tests :)

@tyxla
11 months ago

Add test for searching media by filename in the media REST API endpoint

#9 @tyxla
11 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

39092-test.diff adds a test for searching media by filename in the media REST API endpoint.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


11 months ago

@jblz
11 months ago

Remove extra meta clause, move filter application, incorporate test

#11 @jblz
11 months ago

  • Keywords needs-refresh removed

Refreshed the patch in 39092.3.diff:

  • Removes the extra _wp_attachment_image_alt meta clause
  • Moves the filter application to class-wp-rest-attachments-controller.php so it only applies to media searches
  • Incorporates the test case from 39092-test.diff. Thanks very much, @tyxla!
Last edited 11 months ago by jblz (previous) (diff)

@jblz
11 months ago

Remove extra meta clause, move filter application, incorporate test

@jblz
11 months ago

fix docblock / comment

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


11 months ago

@jnylen0
11 months ago

More robust test (check post ID)

#13 @jnylen0
11 months ago

  • Keywords needs-testing removed
  • Owner set to jnylen0
  • Status changed from new to accepted

This looks good & tests out well for me. Very minor change in 39092.4.diff to verify the correct attachment post ID rather than just the content type.

#14 @pento
10 months ago

  • Keywords commit added

#15 @jnylen0
10 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 39598:

REST API: Add support for filename search in media endpoint.

In [38625], the functionality to search for attachments by filename was added
via the posts_clauses filter and the _filter_query_attachment_filenames()
function. This moves _filter_query_attachment_filenames() from
wp-admin/includes/post.php to wp-includes/post.php so that it can be
applied in the same manner in the REST API media endpoint.

Props jblz, tyxla.
Fixes #39092.

#16 @jnylen0
10 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Needs to be ported to the 4.7 branch.

#17 @pento
10 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 39629:

REST API: Add support for filename search in media endpoint.

In [38625], the functionality to search for attachments by filename was added via the posts_clauses filter and the _filter_query_attachment_filenames() function. This moves _filter_query_attachment_filenames() from wp-admin/includes/post.php to wp-includes/post.php so that it can be applied in the same manner in the REST API media endpoint.

Merge of [39598] to the 4.7 branch.

Props jblz, tyxla.
Fixes #39092.

Note: See TracTickets for help on using tickets.