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

Implement `WP_REST_Search_Controller` class #6489

Closed
wants to merge 27 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@felixarntz
Member

felixarntz commented Apr 29, 2018

Description

This PR aims at introducing a WP_REST_Search_Controller class to allow searching across multiple post types via the REST API. This will allow powering the link modal and other related ideas where results must not be restricted to a single post type.

While this PR should be implemented and evaluated here, the final implementation will then be merged back into core, with the gutenberg/v1 namespace changed to wp/v2. See https://core.trac.wordpress.org/ticket/39965 for the related ticket.

This addresses #2084.

How has this been tested?

For now I've only performed a few API calls to test things work as expected. I plan on writing unit tests later this week.

Types of changes

  • WP_REST_Search_Controller is introduced, and its routes are registered.
  • There is only a collection route here, as individual items can be accessed at their respective endpoints. Links to these singular resources are added in the response per item, however this currently only works with post types using the default WP_REST_Posts_Controller.
  • Only post types that have both public and show_in_rest set to true are searched. The post types to search can be further restricted with a type request parameter.
  • Only post statuses that have public set to true, plus inherit are searched.
  • The schema consists of only general fields that exist for every post. No metadata is included. Fields where the content may not actually be supported per post type are still included (with empty content), to have a consistent schema.
  • A major part of the implementation has been copied from core's WP_REST_Posts_Controller that already has a lot of the necessary functionality implemented. It's still too different though to extend it. We may wanna consider outsourcing some common functionality, but let's keep it simple for now.

Checklist:

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

felixarntz added some commits Apr 29, 2018

felixarntz added some commits Apr 29, 2018

/**
* Gets the post statuses allowed for search.
*
* @since 1.0.0

This comment has been minimized.

@Soean

Soean Apr 30, 2018

Member

since 2.9.0

/**
* Gets the post types allowed for search.
*
* @since 1.0.0

This comment has been minimized.

@Soean

Soean Apr 30, 2018

Member

since 2.9.0

@felixarntz

This comment has been minimized.

Member

felixarntz commented Apr 30, 2018

Note that the test failure comes from not including the 'gutenberg' text domain in translation strings. Since we're gonna move this over to core once it's done and not merge it into the plugin, I think we can ignore this?

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Apr 30, 2018

Since we're gonna move this over to core once it's done and not merge it into the plugin, I think we can ignore this?

Let's go ahead and include them for now. They're easy enough to remove after the fact.

// Retrieve the list of registered collection query parameters.
$registered = $this->get_collection_params();
$query_args = array(
'post_status' => array_keys( $this->get_allowed_post_stati() ),

This comment has been minimized.

@danielbachhuber

danielbachhuber Apr 30, 2018

Member

wp_link_query uses post_status=>publish. Is there a reason we're opening things up?

This comment has been minimized.

@felixarntz

felixarntz May 3, 2018

Member

Any post status that identifies as its content as public should be searched. That is publish only in most cases, but plugins may add to that.

I also included inherit to be able to search for attachments. If that should not be possible, I'd be okay with removing that status.

This comment has been minimized.

@danielbachhuber

danielbachhuber May 17, 2018

Member

We should focus on only supporting post_status=publish for now.

Support for post_status=inherit opens up a can of permissions-related worms. Custom post statuses should be handled in conjunction with #3144

'$schema' => 'http://json-schema.org/draft-04/schema#',
'title' => 'search',
'type' => 'object',
'properties' => array(

This comment has been minimized.

@danielbachhuber

danielbachhuber Apr 30, 2018

Member

I think we should drop: author, content, excerpt, date, date_gmt, guid, modified, modified_gmt, and status for a couple of reasons:

  1. They aren't absolutely necessary for link search, which is the immediate problem we need to solve.
  2. They'll significantly reduce complexity for the controller.

This comment has been minimized.

@felixarntz

felixarntz May 3, 2018

Member

I disagree as this is supposed to be merged into core. A REST controller in core should be available to cover common use-cases beyond Gutenberg. If this was a first test for Gutenberg internally, I'd be okay with using only the few fields needed for that very case, but for core, the typical properties expected should be available.

Furthermore, they don't really add complexity, but just increase the response. I'll implement the new _fields filtering so that those fields are not even generated when they're not needed. Gutenberg can then make a request like wp/v2/search?search=mytext&_fields=id,title,type,link.

This comment has been minimized.

@danielbachhuber

danielbachhuber May 5, 2018

Member

I'd be okay with using only the few fields needed for that very case, but for core, the typical properties expected should be available.

Can you explain how you define "typical properties", and where, say, modified_gmt fits in this category?

This comment has been minimized.

@felixarntz

felixarntz May 9, 2018

Member

I'd define as typical properties those that exist on every post regardless of post type, i.e. things that are part of the wp_posts database table. In that regard I'd also like to align the format with that from the other post type-specific controllers.

felixarntz added some commits May 3, 2018

@felixarntz

This comment has been minimized.

Member

felixarntz commented May 3, 2018

With the changes included in the latest commit, only item data for the fields requested is generated. This means that a core version later than https://core.trac.wordpress.org/changeset/43087 is required to run this PR.

felixarntz added some commits May 5, 2018

@felixarntz

This comment has been minimized.

Member

felixarntz commented May 5, 2018

311413e provides tests for the whole controller functionality.

d7b1762 introduces a type_exclude parameter, allowing post types to be excluded from the search.

I think this is now in a solid state to be considered for core merge. For Gutenberg and the link modal specifically, a request wp/v2/search?search=mytext&type_exclude=attachment&_fields=id,title,type,link will do what's necessary to replicate the current logic.

@felixarntz

This comment has been minimized.

Member

felixarntz commented May 5, 2018

Remaining test errors are only occurring because Gutenberg tests against the latest WordPress version, not against trunk. In core, all of this would work (again, since it requires https://core.trac.wordpress.org/changeset/43087).

@felixarntz

This comment has been minimized.

Member

felixarntz commented May 9, 2018

After talking to @danielbachhuber, it became clear that there are essentially two possible approaches for implementing a search endpoint:

  • Either an endpoint that is tailored to searching all posts. That is what's currently implemented here.
  • Or an endpoint that is tailored to searching all content of your WordPress installation, which by default would still only search posts, but allow for extension by plugins. You could theoretically also search terms, comments, users, anything. Such an endpoint would need to contain a more limited amount of fields, namely id, title and link - arguably also type because it would no longer be clear that the current resource is a "post". If someone needed the full response objects in a request, that could easily be possible via the _embed parameter.

For Gutenberg itself, a simple endpoint that could be just like the one implemented here without the unneeded schema properties and collection params would be sufficient. The good thing is that that version will allow to later shape it into either of the two directions. However, for core, we should decide whether we move forward with the first or the second approach.
That would be a good discussion to have in one of the next REST API meetings.

felixarntz added some commits May 23, 2018

@felixarntz

This comment has been minimized.

Member

felixarntz commented May 23, 2018

@danielbachhuber I made all changes you suggest, except changing the post statuses because I don't think it makes sense to get rid of them (see #6489 (comment)) - flagging this here since GH no longer shows it because the lines you commented have changed.

The gist is: Both custom post statuses and inherit are nothing new in the REST API - while Gutenberg doesn't necessarily need them for now, this controller is not exclusive to Gutenberg, and especially since post statuses in this regard are nothing we need to think about how to implement them, they should be included. #3144 is unrelated I think, since it deals with support custom post statuses in Gutenberg and its UI itself.

felixarntz added some commits May 23, 2018

@felixarntz

This comment has been minimized.

Member

felixarntz commented May 23, 2018

With the latest commit, I changed the UrlInput component to use the new search controller so that now posts of other post types are available as well.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 23, 2018

@felixarntz Looking pretty good. It doesn't look like it searches against attachment in Gutenberg though? Can you explain why status=inherit support is necessary if we aren't using it?

If it is necessary, we'll need unit test coverage for it, including permissions checks.

@felixarntz

This comment has been minimized.

Member

felixarntz commented May 23, 2018

@danielbachhuber

Can you explain why status=inherit support is necessary if we aren't using it?

I decided to exclude attachments from the search because they aren't currently searched in the link modal. We could change that if that is preferred, but regardless of that decision I think the controller should support attachments as this is going to be merged into WordPress core, and search is not only used in Gutenberg. I can add tests for it too.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 23, 2018

We could change that if that is preferred, but regardless of that decision I think the controller should support attachments as this is going to be merged into WordPress core, and search is not only used in Gutenberg.

Where else is search used in WordPress core?

@felixarntz

This comment has been minimized.

Member

felixarntz commented May 23, 2018

Where else is search used in WordPress core?

The main search functionality (search form in frontend) could use this for example. Plugins can use this. By making this only tailored to Gutenberg's very use-case, we force developers to unnecessarily rewrite a lot of code for only simple changes. Especially since this is nothing complicated and has been solved before, I don't think it's worth keeping it out.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 23, 2018

The main search functionality (search form in frontend) could use this for example.

Does it?

Plugins can use this.

Can plugins write their own code to modify the behavior of this controller?

By making this only tailored to Gutenberg's very use-case, we force developers to unnecessarily rewrite a lot of code for only simple changes.

Can you share the code that's necessary to rewrite?

Especially since this is nothing complicated and has been solved before, I don't think it's worth keeping it out.

This is good read on the topic. Notably, features should be added based on genuine need, not hypothetical.

Based on the information presented, support for searching attachments doesn't appear necessary for our current use-case.

@felixarntz

This comment has been minimized.

Member

felixarntz commented May 23, 2018

Does it?

Not yet obviously, but it would be a good use-case for this controller.

Can plugins write their own code to modify the behavior of this controller?

They cannot filter these things, but they can use the controller by making a request to it. If a plugin wants to search content including attachments, this controller would be worthless to them without it.

This is good read on the topic. Notably, features should be added based on genuine need, not hypothetical.

The WordPress search is a valid use-case for this controller, and without attachments it would be incomplete. What is the point of introducing a REST API controller that searches posts, but doesn't actually work for all posts? In addition, even a post of another post type can have a status of inherit in which case its parent post would determine the actual status. To quote the post, we should be "thinking of all users in the big picture" instead of just Gutenberg.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 23, 2018

They cannot filter these things, but they can use the controller by making a request to it. If a plugin wants to search content including attachments, this controller would be worthless to them without it.

Plugins can't filter the behavior of this controller to adapt it to their needs?

The WordPress search is a valid use-case for this controller, and without attachments it would be incomplete.

"without attachments it would be incomplete" seems to communicate a hypothetical future, not the current reality.

To my knowledge, and correct me if I'm wrong, WordPress frontend search doesn't include attachments by default. Am I wrong?

What is the point of introducing a REST API controller that searches posts, but doesn't actually work for all posts?

It seems to solve our immediate needs for link search just fine.

In addition, even a post of another post type can have a status of inherit in which case its parent post would determine the actual status.

Can you point me to a real-world example?

To quote the post, we should be "thinking of all users in the big picture" instead of just Gutenberg.

This statement seems to cherry-pick a quote and ignore the main point of the post.

@felixarntz

This comment has been minimized.

Member

felixarntz commented May 23, 2018

Plugins can't filter the behavior of this controller to adapt it to their needs?

It's currently not possible to filter things like the supported collection request parameters and how content is searched. Let me know if this should be added.

This statement seems to cherry-pick a quote and ignore the main point of the post.

I read the post and understand what it says, we apparently interpret it differently.

Anyway, the recent bits of the discussion seem to not move this forward. I think we need a third opinion on the topic of flexibility vs pragmatism here.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 23, 2018

It's currently not possible to filter things like the supported collection request parameters and how content is searched. Let me know if this should be added.

Sure, filters would be a great addition.

I think we need a third opinion on the topic of flexibility vs pragmatism here.

Sounds good. cc @pento

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented May 25, 2018

It's currently not possible to filter things like the supported collection request parameters and how content is searched. Let me know if this should be added.

Sure, filters would be a great addition.

This was specifically requested in #4598

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 27, 2018

@felixarntz Are you alright with removing status for now so we can finish up this pull request and land it?

@felixarntz

This comment has been minimized.

Member

felixarntz commented Jul 1, 2018

I talked with @rmccue for a while at WCEU, and we came up with a plan for this:

  • We agree that the search controller should not just replicate what the posts controller does, without the post type-specific stuff.
  • We also agree that a controller just called /search should be intended as a universal search controller, for all WordPress content, not only posts.
  • While it is currently impossible to search posts/terms/comments etc. altogether at once, we should implement the controller in a future-proof way that permits this. The first iteration would be okay to only support posts, closely followed by an iteration that should add support for terms and comments for individual searches as well.
  • We need to return data in a way that can be uniform for all content, as we of course need one uniform schema. We are thinking about:
    • id
    • title
    • url
    • type
    • subtype
    • possibly also content
  • As you can see, there are both a type and subtype in the above list. That is because these two values are necessary to fully identify what kind of content a certain item is. This is in line with the behavior for metadata after https://core.trac.wordpress.org/changeset/43378. For example, a type could be “post”, “term” or “comment”. A subtype could be “post”, “page”, “attachment”, “category” etc.
  • The request parameters available should be:
    • search: Obvious, search string to look for.
    • type: Allows to filter the type looked for. Initially, this will only support a single string, either “post”, “term” or “comment”. The default will be “post”. Alternatively, if we decide to only support posts for the first iteration, we can of course introduce “term” and “comment” later too. Having this parameter is future-proof as we could in theory at some point make it an array to allow cross-search between posts, terms etc., or even allow it to be empty and then search everything there is.
    • subtype: Allows to filter the subtype looked for. This should support an array of subtypes (post types, taxonomies). The default value should be empty, in order to search everything identified by type. Since this parameter is a bit more complex, we might be okay leaving it out of the first iteration.
    • plus the typical page and per_page parameters for pagination
  • An issue to figure out is the sort order. When searching posts, you often wanna sort them by date. However, not all types have a date. This is more complex, so we might be okay initially always sorting by the id.
  • Due to this being not a Gutenberg-exclusive enhancement, we suggest continuing work on this in the core ticket https://core.trac.wordpress.org/ticket/39965. Because it is pressing for Gutenberg though, the goal would be to have it be merged and backported as soon as possible, for one of the next minor releases.
@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jul 1, 2018

we came up with a plan for this:

👍 Works for me.

Because it is pressing for Gutenberg though, the goal would be to have it be merged and backported as soon as possible, for one of the next minor releases.

One nice thing about landing in Gutenberg early is that we can iterate on the endpoint based on real-world usage, and then commit to core once we think it's finalized. I'd prefer to follow this approach; however, if you prefer to work on the endpoint design via Trac patches that sounds reasonable to me.

@pento

This comment has been minimized.

Member

pento commented Jul 3, 2018

I'm inclined to agree with @danielbachhuber, let's get this into Gutenberg first, iterating on Trac is slow and annoying. Once we have Gutenberg's usage locked down, we'll be in a good place to look at expanding it to more data types.

@felixarntz

This comment has been minimized.

Member

felixarntz commented Jul 3, 2018

Sounds good to me. Will work on the new version either this weekend at WC Sevilla Contributor Day, or early next week. I'm inclined to create a new PR though and close this one, only referencing it from there. Is that good with you?

@pento

This comment has been minimized.

Member

pento commented Jul 10, 2018

Hey, @felixarntz: have you had a chance to look at this?

@felixarntz

This comment has been minimized.

Member

felixarntz commented Jul 11, 2018

@pento I will tomorrow!

@rmccue

This comment has been minimized.

Contributor

rmccue commented Jul 11, 2018

Just chiming in to note that @felixarntz summarised the conversation well. The key point for the type/subtype bit is just making sure that we have forward compatibility for the changes we might want to make down the line.

Agreed with landing in Gutenberg first for iteration speed as well.

@felixarntz felixarntz referenced this pull request Jul 11, 2018

Merged

Implement forward-compatible REST API search controller #7894

4 of 4 tasks complete
@felixarntz

This comment has been minimized.

Member

felixarntz commented Jul 11, 2018

@danielbachhuber @pento @rmccue I opened #7894 for the new implementation of the global search controller. Let's continue there.

@felixarntz felixarntz closed this Jul 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment