Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 22 months ago

#39092 closed enhancement (fixed)

REST API: Add support for filename search in media endpoint

Reported by: jblz's profile jblz Owned by: jnylen0's profile 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 8 years ago.
39092-test.diff (1.1 KB) - added by tyxla 8 years ago.
Add test for searching media by filename in the media REST API endpoint
39092.2.diff (4.2 KB) - added by jblz 8 years ago.
Remove extra meta clause, move filter application, incorporate test
39092.2.2.diff (4.2 KB) - added by jblz 8 years ago.
Remove extra meta clause, move filter application, incorporate test
39092.3.diff (4.2 KB) - added by jblz 8 years ago.
fix docblock / comment
39092.4.diff (4.5 KB) - added by jnylen0 8 years ago.
More robust test (check post ID)

Download all attachments as: .zip

Change History (24)

@jblz
8 years ago

#1 @jblz
8 years ago

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

#2 @jnylen0
8 years 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
8 years 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
8 years 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
8 years 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
8 years 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
8 years 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
8 years 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
8 years ago

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

#9 @tyxla
8 years 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.


8 years ago

@jblz
8 years ago

Remove extra meta clause, move filter application, incorporate test

#11 @jblz
8 years 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 8 years ago by jblz (previous) (diff)

@jblz
8 years ago

Remove extra meta clause, move filter application, incorporate test

@jblz
8 years ago

fix docblock / comment

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


8 years ago

@jnylen0
8 years ago

More robust test (check post ID)

#13 @jnylen0
8 years 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
8 years ago

  • Keywords commit added

#15 @jnylen0
8 years 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
8 years 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
8 years 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.

#18 @nicohood
22 months ago

Hey guys, I looked through the code, but it is still not clear to me if the search will search with wildcards, substrings, etc or only work for exact matches?

Lets say I have:
blue-car.jpg
red-car.jpg
car.jpg

Will it find 1 or 3 pictures if I search for "car.jpg"?

Note: See TracTickets for help on using tickets.