Implement forward-compatible REST API search controller by felixarntz · Pull Request #7894 · WordPress/gutenberg

felixarntz · 2018-07-11T15:57:45Z

Description

This PR is a new take on what was previously worked on in #6489. It was decided to go with a completely different approach (particularly explained in #6489 (comment)), and it didn't make sense to adjust that PR for that. The new implementation is a fully forward-compatible approach that theoretically allows for searching any WordPress content and is not tied to only posts like the original PR was. However, since only post search is a priority for Gutenberg, that object type (post) is the only one we need.

The REST controller uses gutenberg/v1 as its namespace at the moment. This should change when it gets merged into core.

This addresses #2084.

Types of changes

WP_REST_Search_Controller is a read-only controller that contains a single route which is a collection of arbitrary search results.
Each search result consists of the following properties:
- id
- title
- url
- type (i.e. 'post', 'term', 'comment', 'user', etc.)
- subtype (i.e. a post type, a taxonomy, a comment type, etc.)
The main goal of the controller is to provide an interface to searching any content for a search term, therefore the request param search is most important here. However, the controller also works without it, simply listing all content that is available.
Other request parameters that can be used are:
- page and per_page, as used by almost all core REST controllers
- type: A single string identifying whether to search posts, terms, comments, ... The default value is 'post' since that is the most likely type people want to search and the only one this PR adds support for. Other types like 'term' and 'comment' could be implemented after the initial core merge. In some point far away in the future, it may be possible to search cross-content, which is also considered here. The parameter only supports a string now, but it can easily be adjusted to support an array if that becomes possible.
- subtype: One or more strings (i.e. an array) identifying the subtypes (post types, taxonomies, ...) to search. The default value is set to 'any', which essentially ensures that all subtypes of the type specified via type are searched.
Individual types are handled via separate implementations of a new base class WP_REST_Object_Search_Handler, which makes sense because all types work very differently in terms of querying and preparing the output in a uniform format that matches the above search result properties. It also allows for the logic in the search controller itself to be very straightforward.
As only posts are supported for now, only that implementation called WP_REST_Post_Search_Handler is part of the PR.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

…ress content, only supporting posts for now.

felixarntz · 2018-07-11T15:58:39Z

I've tested the controller manually by making some requests and it appears to work well. I'll work on unit tests after an initial review to ensure it's on the right track.

danielbachhuber

Looks like a good start. Left a couple of comments with the time I have to look now.

I'm not overly enthusiastic about the abstraction, but I'm not opposed either. What would make it much more compelling is a (plugin?) implementation of the abstraction, to prove that it's useful.

danielbachhuber · 2018-07-11T17:39:55Z

lib/class-wp-rest-post-search-handler.php

+		}
+
+		// Get the public post statuses as only those should be searched.
+		$post_statuses = array_values( get_post_stati( array(


For parity with core, we should simply use post_status=>publish. We can discuss custom statuses at greater depth with #3144

I'm still not fully convinced why as the REST API handles post statuses like that in core already, but I'm okay simplifying it.

I'm still not fully convinced why as the REST API handles post statuses like that in core already, but I'm okay simplifying it.

Oh. If that's the case, I'm fine with it as-is.

For now, I decided to follow your suggestion and only use publish for now. The only problem that brings is that the controller won't be able to search attachments at this point because they typically use inherit. Is it acceptable not having that in the first iteration? Pinging @pento for an opinion on this too.

Because searching for attachments will always return an empty array for now (which is unexpected), I explicitly removed support for type=>post and subtype=attachment. Once we add support, we can allow that again.

danielbachhuber · 2018-07-11T17:40:34Z

lib/class-wp-rest-post-search-handler.php

+			'fields'              => 'ids',
+		);
+
+		// If a search term is given, add it and order by relevance.


Is there a reason we default to ID and then switch to relevance?

When search is not given, it is impossible to sort by relevance, so we need to have some default.

When search is not given, it is impossible to sort by relevance, so we need to have some default.

Makes sense. It'd be great to have this clarified in the comment.

danielbachhuber · 2018-07-11T17:43:04Z

lib/class-wp-rest-post-search-handler.php

+	 *               an array of found IDs and `WP_REST_Object_Search_Handler::RESULT_TOTAL` containing the
+	 *               total count for the matching search results.
+	 */
+	public function search_items( WP_REST_Request $request ) {


Core doesn't typically type cast.

True, but I don't see why this cannot change. To be fair, there are a few areas in core where type hints are present, and them being available (where possible in PHP 5.2) ensures that the parameter is valid.

I'm not strongly opposed to changing it, but only if there's other arguments/opinions against it.

There are a handful of uses in Core, I have no problem with adding this one.

(Run ack --php 'ack --php '(?:^|\s)function [^(]+\([^)]*?(\S+[^(,] \$[^ ),]+)'' on Core for examples.)

felixarntz · 2018-07-11T18:39:13Z

@danielbachhuber

I'm not overly enthusiastic about the abstraction, but I'm not opposed either. What would make it much more compelling is a (plugin?) implementation of the abstraction, to prove that it's useful.

Are you saying you would like to see a demo plugin where that abstraction is leveraged? I could do that, but in that case I think it would make more sense to implement a search handler for terms as a proof-of-concept because that is something we could actually use in core at some point. Well, I could still implement that as a plugin.

danielbachhuber · 2018-07-11T19:06:52Z

Are you saying you would like to see a demo plugin where that abstraction is leveraged?

Yep.

danielbachhuber · 2018-07-11T19:08:26Z

@felixarntz Let's go ahead and get tests + Gutenberg integration in place for this.

felixarntz · 2018-07-11T20:42:17Z

@danielbachhuber I'll implement the feedback and work on tests + integration on Friday.

pento

Thanks for tackling this, @felixarntz.

I have similar feelings as @danielbachhuber about the abstraction. I see how it will be potentially valuable in the future, as more search handlers are implemented, but it does feel a little premature.

I would certainly want to see several other search handlers implemented before it could be merged into Core. If you're okay with ensuring that happens, I'm fine with leaving it.

pento · 2018-07-13T02:29:39Z

lib/class-wp-rest-post-search-handler.php

+	 *               an array of found IDs and `WP_REST_Object_Search_Handler::RESULT_TOTAL` containing the
+	 *               total count for the matching search results.
+	 */
+	public function search_items( WP_REST_Request $request ) {


There are a handful of uses in Core, I have no problem with adding this one.

(Run ack --php 'ack --php '(?:^|\s)function [^(]+\([^)]*?(\S+[^(,] \$[^ ),]+)'' on Core for examples.)

pento · 2018-07-13T02:36:30Z

lib/class-wp-rest-object-search-handler.php

+ *
+ * @since 3.3.0
+ */
+abstract class WP_REST_Object_Search_Handler {


To match the naming scheme of other abstract classes in the REST API WP_REST_Meta_Fields, WP_REST_Controller, the class name should just be WP_REST_Search_Handler, the implementing classes would follow the naming scheme you uses for WP_REST_Post_Search_Handler.

Makes sense to me!

felixarntz · 2018-07-13T12:46:19Z

@pento

I have similar feelings as @danielbachhuber about the abstraction. I see how it will be potentially valuable in the future, as more search handlers are implemented, but it does feel a little premature.

I would certainly want to see several other search handlers implemented before it could be merged into Core. If you're okay with ensuring that happens, I'm fine with leaving it.

I will ensure that then. :) Originally my thought was it would be fine to add them later, but I agree implementing them before merge ensures the abstraction is solid enough. I'll implement the terms integration as a plugin for now, but once we get to a core merge, I can move that over into the actual patch for the whole thing.

…rch term is given.

…ject search handlers into search controller, including a filter hook.

…or now because they are not support at this point.

felixarntz · 2018-07-13T15:42:10Z

I have added tests for the search controller now, including a very simple WP_REST_Dummy_Search_Handler implementation that is loaded for tests to ensure such a custom integration works.

I've also adjusted the UrlInput React component to use the new endpoint, as previously suggested in the original PR.

An additional plugin adding term support to the REST search controller is available at https://github.com/felixarntz/gutenberg-rest-term-search-handler. Simply download and activate it together with this Gutenberg PR to see it in action.

felixarntz · 2018-07-13T15:53:49Z

From my POV this now has everything we discussed so far. I have two open questions we need to answer before merge though:

I currently sort results by ID descending by default. My trail of thought behind this is that every type of content has some kind of ID, and the ID being higher typically means it's newer. Sure, for posts we have a date, but not all types of content have one, that's why I chose the ID for the sort criteria, in order to have a similar foundation for all search handlers. Is that a good solution for now, or are there any concerns?
The controller does not support any kind of content at the moment, however many object types have content that goes beyond a title (e.g. posts have post_excerpt, terms have description), and having that content available would be valuable for several use-cases (for example if using the controller for the posts search in the frontend of a WordPress site). Should we include that for every result or not? My concern is if we don't do it now, but decide to do it later, we need to highlight that change so that developers can adapt their code to it.

pento · 2018-07-16T02:14:42Z

I currently sort results by ID descending by default.

All of the WP_*Query classes implement default sort orders, why do we need to explicitly set it at all?

In the case of WP_Query, if orderby isn't specified, it will sort by post_date, unless s is set, in which case it will sort by relevance.

In the case of WP_Term_Query, it will default to sorting by name, which I suspect would be a useful default in search results, too, terms are generally not time-sensitive data.

The controller does not support any kind of content at the moment

I don't think that's necessary for now. If we decide to do it later, I suspect it will take two forms:

Some sort of generic field that each type would need to implement separately, so clients have a content lump they can use without processing.
The full object of the found item, so clients can perform specific actions by type.

I don't think it's worth the effort of planning for and adding either of these until we have an actual use case in Core for them.

felixarntz · 2018-07-16T13:01:09Z

Agreed. Will update the PR accordingly.

pento

Let's party! 🎉

pento

Wait, I got ahead of myself.

I got too excited. Also, GitHub UI is weird.

pento · 2018-07-17T05:39:43Z

@karmatosed, could you please provide design feedback on this? Classic shows a bit more information in the search results than Gutenberg does, do you think Gutenberg needs to show more information?

Classic Editor

Gutenberg

karmatosed · 2018-07-17T08:48:31Z

I don't mind adding that in if there is a use case for knowing that information. I do wonder how useful a date is though. I can see the use of seeing if something is a page.

pento · 2018-07-20T00:48:49Z

It seems like the use case is being able to differentiate results.

For posts, it shows the post date, for all other post types, it shows the post type. The date is probably useful for sites that do regular posts on a particular topic, but they keep the same title every time. (Eg, "Monthly Review Roundup", or something like that.) Being able to link to the post that happened most recently would be useful.

felixarntz · 2018-07-23T13:26:22Z

@pento If we need the date here (which I agree would be useful), that would be a very specific thing because not all content types in WordPress have dates (for example terms).

In that case I would suggest querying the REST API search controller with embed=1 to get the embedded post object for those extra details and display them, something which we could do in a separate follow-up PR, as it would be a separate enhancement IMO.

…h-controller

pento · 2018-07-24T04:20:54Z

Yah, let's do that in a follow up, so we can get this shipped.

pento

🎉

Implement forward-compatible REST API search controller for any WordP…

3ce07b8

…ress content, only supporting posts for now.

felixarntz requested a review from danielbachhuber July 11, 2018 15:57

felixarntz mentioned this pull request Jul 11, 2018

Implement WP_REST_Search_Controller class #6489

Closed

4 tasks

danielbachhuber reviewed Jul 11, 2018

View reviewed changes

danielbachhuber added [Status] In Progress Tracking issues with work in progress Core REST API Task Task for Core REST API efforts labels Jul 11, 2018

pento reviewed Jul 13, 2018

View reviewed changes

Felix Arntz added 8 commits July 13, 2018 14:52

Search only post with status 'publish' in the REST search controller.

57294fc

Clarify posts orderby parameter is set to 'relevance' only when a sea…

9c9efa0

…rch term is given.

Rename WP_REST_Object_Search_Handler to WP_REST_Search_Handler and in…

c1b206a

…ject search handlers into search controller, including a filter hook.

Remove unnecessary instantiation.

015fed3

Explicitly forbid access to attachments in REST post search handler f…

1f1bc79

…or now because they are not support at this point.

Add tests for REST search controller.

12859be

Add tests for the REST search controller and its integrations.

016d8ba

Adjust UrlInput component to use the new REST search controller.

22baf55

chrisvanpatten mentioned this pull request Jul 14, 2018

Add pages to suggestions for UrlInput #7951

Closed

Rely on WP_Query orderby default values in WP REST post search handler.

d8baf24

pento previously approved these changes Jul 17, 2018

View reviewed changes

pento removed the [Status] In Progress Tracking issues with work in progress label Jul 17, 2018

pento added this to the 3.3 milestone Jul 17, 2018

pento reviewed Jul 17, 2018

View reviewed changes

pento added the Needs Design Feedback Needs general design feedback. label Jul 17, 2018

aduth modified the milestones: 3.3, 3.4 Jul 18, 2018

Merge remote-tracking branch 'upstream/master' into add/wp-rest-searc…

903ec79

…h-controller

chrisvanpatten mentioned this pull request Jul 23, 2018

"Add a link" only offers posts whereas current WP Editor offers posts and pages #8152

Closed

pento approved these changes Jul 24, 2018

View reviewed changes

pento removed the Needs Design Feedback Needs general design feedback. label Jul 24, 2018

pento merged commit eb27a09 into WordPress:master Jul 24, 2018

This was referenced Jul 30, 2018

Fix the UrlInput component after the search API introduction #8302

Merged

CSS and add Link Issue #8419

Closed

designsimply mentioned this pull request Aug 20, 2018

Hyperlink are not searching for CPT content #9131

Closed

designsimply mentioned this pull request Sep 17, 2018

Suggestion for internal links #9932

Closed

Conversation

felixarntz commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Checklist:

Uh oh!

felixarntz commented Jul 11, 2018

Uh oh!

danielbachhuber left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danielbachhuber commented Jul 11, 2018

Uh oh!

danielbachhuber commented Jul 11, 2018

Uh oh!

felixarntz commented Jul 11, 2018

Uh oh!

pento left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz commented Jul 13, 2018

Uh oh!

felixarntz commented Jul 13, 2018

Uh oh!

felixarntz commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pento commented Jul 16, 2018

Uh oh!

felixarntz commented Jul 16, 2018

Uh oh!

pento left a comment

Choose a reason for hiding this comment

Uh oh!

pento left a comment

Choose a reason for hiding this comment

Uh oh!

pento commented Jul 17, 2018

Uh oh!

karmatosed commented Jul 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pento commented Jul 20, 2018

Uh oh!

felixarntz commented Jul 23, 2018

Uh oh!

pento commented Jul 24, 2018

Uh oh!

felixarntz commented Jul 11, 2018 •

edited

Loading

felixarntz commented Jul 11, 2018 •

edited

Loading

felixarntz commented Jul 13, 2018 •

edited

Loading

karmatosed commented Jul 17, 2018 •

edited

Loading