Use REST API for ajax tag search

[nacin]

Proof of concept. Probably can't be done because you lose the context on the filter -- we will continue to see this for existing ajax actions, but this will be super nice for newer things.

Similar: #38920.

[nacin]

38922.diff​ follows the same pattern as #38920 which uses a localized rest_url which isn't really ideal. Is it worth having a helper a la ajaxurl? wp-api.js doesn't seem to expose a nice method for this. wp.api.url('wp/v2/tags') would be nice, perhaps. Or obviously I could use wp-api.js, but wanted to try this approach as a targeted diff.

Obviously we wouldn't actually remove the old action, and instead deprecate it.

[nacin]

There's a typo on the minLength line I'm not going to bother to fix right now, but it works otherwise.

[chriscct7]

38922-2 corrects the extra = and ends the line with , instead of ;.

Also keeps the old ajax function, and uses it to retrieve the limit in a new GET request so as to preserve hook backwards compatibility with context. In order to have context but not have a second GET request the REST API would likely need to have an endpoint that uses the min-chars filter.

[adamsilverstein]

38922.2.diff​ Builds off the original patch:

• correct some typos in ajax-actions.php
• add back cache set for potential repeat calls
• deprecate wp_ajax_ajax_tag_search

[afercia]

Worth noting the REST API response return by default all the fields, even the ones that in this case are not needed, while the traditional old AJAX way returns just the terms. Quickly testing on a VVV install, searching for co, here's a quick comparison:

AJAX: 430B, 112ms

claycold
Codex
comments
content
shortcode

REST API: 3.6KB, 168ms

[
{"id":76,"count":0,"description":"","link":"http:\/\/src.wordpress-develop.dev\/tag\/claycold\/","name":"claycold","slug":"claycold","taxonomy":"post_tag","meta":[],"_links":{"self":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/tags\/76"}],"collection":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/tags"}],"about":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/taxonomies\/post_tag"}],"wp:post_type":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/posts?tags=76"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}},
{"id":77,"count":4,"description":"","link":"http:\/\/src.wordpress-develop.dev\/tag\/codex\/","name":"Codex","slug":"codex","taxonomy":"post_tag","meta":[],"_links":{"self":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/tags\/77"}],"collection":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/tags"}],"about":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/taxonomies\/post_tag"}],"wp:post_type":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/posts?tags=77"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}},
{"id":78,"count":5,"description":"","link":"http:\/\/src.wordpress-develop.dev\/tag\/comments-2\/","name":"comments","slug":"comments-2","taxonomy":"post_tag","meta":[],"_links":{"self":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/tags\/78"}],"collection":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/tags"}],"about":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/taxonomies\/post_tag"}],"wp:post_type":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/posts?tags=78"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}},
{"id":79,"count":13,"description":"","link":"http:\/\/src.wordpress-develop.dev\/tag\/content-2\/","name":"content","slug":"content-2","taxonomy":"post_tag","meta":[],"_links":{"self":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/tags\/79"}],"collection":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/tags"}],"about":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/taxonomies\/post_tag"}],"wp:post_type":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/posts?tags=79"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}},
{"id":146,"count":6,"description":"","link":"http:\/\/src.wordpress-develop.dev\/tag\/shortcode\/","name":"shortcode","slug":"shortcode","taxonomy":"post_tag","meta":[],"_links":{"self":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/tags\/146"}],"collection":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/tags"}],"about":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/taxonomies\/post_tag"}],"wp:post_type":[{"href":"http:\/\/src.wordpress-develop.dev\/wp-json\/wp\/v2\/posts?tags=146"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}
]

I'm really not an expert about the REST API, just wondering what is the advantage in using it in this specific case.

[swissspidy]

I'm really not an expert about the REST API, just wondering what is the advantage in using them in this specific case.

Providing more data in the response will be very useful in the future when you want to distinguish tags with the same name (#37975), do some sort of autocompletion (#31696), exclude tags that have already been added (#21258) or perhaps show the usage count of the terms in parentheses.

Regarding the patches, the term_search_min_chars hook shouldn't be documented twice. See ​https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#4-1-duplicate-hooks

[afercia]

Replying to swissspidy:

Providing more data in the response will be very useful in the future

I guess that can be done with AJAX too, if and when there will be the need for more data. Adding more data now, when they're not needed, doesn't make much sense to me. In this specific example the difference is just ~ 3 KB but the REST API response is bigger and slower anyways. Think at the case of #38920 where the whole posts are returned, including the post content which can be huge in size.

[adamsilverstein]

Replying to afercia:

Worth noting the REST API response return by default all the fields, even the ones that in this case are not needed,

This is a very good point, and something we are discussing supporting directly in #38131. In the meantime, we could address the issue here using a filter (rest_prepare_tag?) to reduce the returned fields to only those that we need.

[adamsilverstein]

@afercia in 38922.3.diff​ I added a filter on rest_prepare_post_tag to reduce the returned data. the result is the response size now matches the wp-ajax response. We can expand this filter if we want to include additional fields.

[jnylen0]

I would prefer that we solve the general case in #38131 instead of adding special logic based on $_GET['action']. That logic isn't very RESTful and I foresee hundreds of other special cases appearing for wp-admin usage if we're not careful.

The extra response size seems like something we can live with until #38131 is done.

[afercia]

@adamsilverstein @jnylen0 thanks, I totally trust your judgment since you're the experts here :) As a non-expert, I think solving the general issue in #38131 make sense though.

The extra response size seems like something we can live with until #38131 is done.

Yep, after all, in this specific case with tags, the size difference is pretty small. However, in other cases it's potentially huge. See #38920.

[adamsilverstein]

Replying to jnylen0:

The extra response size seems like something we can live with until #38131 is done.

Sounds fine waiting and agree we will need this functionality repeatedly. I added the patch for consideration and to demonstrate how to filter the response.

[rmccue]

Deprecate function further, and update docs

[rmccue]

38922.4.diff​ moves the function to wp-admin/includes/deprecated.php and adds a _deprecated_function call. It also removes the duplicate documentation for the filter. I renamed rest_url to restURL to match the casing of all the rest of the parameters there, otherwise it's a bit out of place.

There's no other filters we need to keep compatibility with AFAICT. There are the deeper-level filters inside core, but not sure we need compatibility with those? If not, +1 on commit.

[adamsilverstein]

@rmccue the latest patch looks good and works well. what other lower level filters are you thinking of that we would loose?

[adamsilverstein]

38922.5.diff​ addresses a JSHint error 'tempID' is defined but never used.

[adamsilverstein]

Discussed the 'deeper filter' concern with @rmccue. His concern was around hooks buried inside functions called by this request: get_terms, etc. When we make this change the same hooks will still run, however, the context will be different since this will now be a REST request instead of an AJAX request.

Developers relying on checking AJAX_REQUEST etc. inside their hook callbacks will need to adjust their code and we should make sure we note that in the developer docs/release field guide.

[adamsilverstein]

Reviewing this further I noticed that we dropped two context parameters (tax and search) from the term_search_min_chars filter... before apply_filters( 'term_search_min_chars', 2, $tax, $s ); after apply_filters( 'term_search_min_chars', 2 ) - I think this is what @nacin was referring to in the ticket description.

It may be fine to drop these context parameters; if we do we need to document that change. I'm going to try to scan the plugin repository for use of this filter and also look at when the filter was introduced if the parameters are valuable.

[adamsilverstein]

Searching the plugins directory I couldn't find any use of the term_search_min_chars filter.

We can't support the search and tax parameters here because the filter is run before the search is executed (previously it ran on the search callback). Let's remove/deprecate these context parameters. This should be ok as long as we document it since they appear unused and other filters are already available to let developers adjust the results for the search query or rest response that would include access to the tax/search.

[adamsilverstein]

@DrewAPicture can you recommend the proper way to document removing a filter's context parameters? Anything beyond adding a description of when and why the parameters were removed in the docblock? Thinking something like:

Previous to 4.8.0 passed taxonomy and search context parameters to the callback. Use the 'rest_prepare_{$this->taxonomy}' filter to adjust responses instead.

What do you think?

any concerns about removing the context parameters given that it is unused by plugins and developers could now use the rest_prepare_{$this->taxonomy} to similarly filter search min chars based on search and taxonomy?

[ryelle]

38922.7.diff​ updates the previous patch to use _fields, allowing us to restrict the response to just tag ID & name. The only thing I'm not sure about is version info in the documentation -- what the @deprecated tag should read, and the "Previous to X, this filter…" info in the tag docs.

[adamsilverstein]

@ryelle This works great in my testing, thanks! It is nice seeing the tiny api response.

The docblock with @deprecated looks right although the _deprecated_function call should also be 4.9.

I tested this in Gutenberg tag search and noticed I'm getting the full tag data payload - it would be good to add the _fields parameter there as well. I opened an issue: ​https://github.com/WordPress/gutenberg/issues/3913

API response after patch:

[adamsilverstein]

In 42614:

Taxonomy: Use REST API for ajax tag search.

Deprecate wp_ajax_ajax_tag_search and switch to using the REST API when searching tags in the tags meta box.

Props nacin, chriscct7, afercia, swissspidy, jnylen0, rmccue, ryelle.
Fixes #38922.

[adamsilverstein]

In 42619:

Remove unit tests for deprecated ajax tag search function.

Fixes unit tests failing since r42614.

Ammends [42614].
See #38922.

[afercia]

Shouldn't this change have the keyword needs-dev-note? Is it safe to introduce it in a minor release?

[afercia]

Sorry to reopen, the interaction has changed and it's slightly worsened for keyboard users.
Previously:

1. using the keyboard, highlight one of the results in the suggestions
2. press Enter
3. the term gets added

Now:

1. repeat 1 and 2
2. the term is not added, it's just inserted in the input field
3. now you have to press Enter again to actually add the term

[ocean90]

Replying to adamsilverstein:

In 42619:

Remove unit tests for deprecated ajax tag search function.

Fixes unit tests failing since r42614.

Ammends [42614].
See #38922.

What was the reason for not using the @expectedDeprecated annotation?

[adamsilverstein]

@afercia thanks for the feedback, I will work on correcting that insertion issue you noted. Not sure about the version we should release this in, I was originally thinking 4.9.4 - I'm not certain what goes in minor releases at this point. What do you think? Agree needs-dev-note is appropriate here.

@ocean90 I wasn’t aware of @expectedDeprecated - thanks for pointing that out. I will restore the tests and change the expected statements to match.

[adamsilverstein]

@afercia Looking at the code, it seems like this is the intended behavior - I'm not sure why the patch changed this here, but as is I can select multiple tags, then hitting enter inserts them all. I realize this is a slight change from before, but wondering whether its maybe better? Still digging into why it changed.

[adamsilverstein]

@afercia I did some testing and it turns out the behavior of the suggest completion on enter was not affected here - it was changed in r41988 (see #42234) where the handler for the input event was changed from keyup to keypress. (cc @SergeyBiryukov)

Perhaps we can listen for both events or listen for the 'input' event instead if this is an issue that needs addressing?

[adamsilverstein]

In 38922.9.diff​

• reintroduce Tests_Ajax_TagSearch with expectedDeprecated

Does this look good @ocean90? test continue passing after this - ​https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/335159915

[afercia]

@adamsilverstein yeah sorry, I later realized that the changed behavior was happening also on stable.

I'm not certain what goes in minor releases at this point.

I share your feeling :) Maybe not something that needs a dev note?

[afercia]

Re: the events and Enter key: worth noting there's now the need to press Enter twice to add a term. Previously, one Enter key press was enough because the term was actually added when releasing the Enter key (on keyup).

[adamsilverstein]

Replying to afercia:

Re: the events and Enter key: worth noting there's now the need to press Enter twice to add a term. Previously, one Enter key press was enough because the term was actually added when releasing the Enter key (on keyup).

@afercia - Ok, so do you think this change is ok as long as we make a release note about it (ps. it changed in r41988, not on this ticket)

[afercia]

@adamsilverstein please do feel free to close this ticket I'll open a new one as the new interaction is not so ideal for keyboard users.

[adamsilverstein]

Ok, will do once I resolve the unit test issue raised by @ocean90 - awaiting his feedback.

[afercia]

Agree needs-dev-note is appropriate here.

[dlh]

It looks like [42614] included old version numbers in the @deprecated tag and _deprecated_function() call. 38922.10.diff​ would bump both to 5.0.0.

[adamsilverstein]

Thanks for catching that @dlh I'll get that in with the test update in 38922.9.diff​.

[ocean90]

Replying to adamsilverstein:

In 38922.9.diff​

• reintroduce Tests_Ajax_TagSearch with expectedDeprecated

Does this look good @ocean90? test continue passing after this - ​https://travis-ci.org/adamsilverstein/wordpress-develop-fork/builds/335159915

👍

[adamsilverstein]

38922.11.diff​ is 38922.9.diff​ + 38922.10.diff​ refreshed against trunk

[adamsilverstein]

In 42737:

Taxonomy: restore TagSearch unit tests and correct deprecated version string.

Reverts unit test removal, instead changing them to expect the function to be deprecated.
Correct the version the ajax callback was deprecated.

Amends [42614].

Props dlh, ocean90.
Fixes #38922.

[danielbachhuber]

This needs to work for custom taxonomies too, per #44388

[adamsilverstein]

This needs to work for custom taxonomies too, per #44388

@danielbachhuber thanks for re-opening. this will require taxonomies have show_in_rest set to true. otherwise we should consider reverting and since we have REST support for tag search in Gutenberg already. what do you think?

[danielbachhuber]

@adamsilverstein I don't have a strong preference. Unless there's a good argument for keeping this enhancement, it's probably easier to revert and punt.

[adamsilverstein]

Unless there's a good argument for keeping this enhancement, it's probably easier to revert and punt.

Yea, I was thinking the same as I'd rather focus my efforts on Gutenberg. Unless some else wants to work on this or is strongly opposed I'll go ahead and revert and close as wontfix.

[enrico.sorcinelli]

As I mentioned in my original #44388, the fast patch I proposed will transform /tags end point (and in general all taxonomies end points) as an open proxy end point for all taxonomies even for those that are been registered without show_in_rest, that maybe in general is not too good.

Vice-versa, by constraining the search only to REST exposed taxonomies, assumes that users has to explicitly register custom taxonomy by setting show_in_rest to a true value.

So reverting to ajax, seems to be the a reasonable solution.

[danielbachhuber]

Should we revert this out of 5.0? It still needs to work for custom taxonomies, and it seems the priority for 5.0 should be Gutenberg.

[danielbachhuber]

Given the branching strategy for 5.0 (5.0 branch was created from the 4.8 branch instead of trunk), we can go ahead and let r42614 be for the time being.

[jrf]

The version number for the deprecation needs to be changed - see #45657.

[danielbachhuber]

This should be reverted, given:

1. It doesn't work for custom taxonomies.
2. Our efforts should generally be applied to Gutenberg.

[adamsilverstein]

Agree, we should revert this in trunk once trunk is re-opened.

[desrosj]

In 44537:

REST API: Restore use of wp_ajax_ajax_tag_search() for tag search.

This solution does not work with custom taxonomies in the current state.

Reverts [42614,42619,42737].

Props danielbachhuber.
See #38922.

[desrosj]

In 44538:

Docs: Restore output annotation in tags-suggest.js.

This was mistakenly removed in [44537].

See #38922.

[desrosj]

This has been reverted. Moving to Future Release, but feel free to take it in another direction.

[TimothyBlynJacobs]

We do have a wp/v2/search endpoint now that supports tags, but this still looks to be blocked by #42785.