WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 13 days ago

#38922 reviewing defect (bug)

Use REST API for ajax tag search

Reported by: nacin Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Taxonomy Keywords: needs-patch
Focuses: rest-api Cc:

Description

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.

Attachments (12)

38922.diff (4.1 KB) - added by nacin 3 years ago.
38922-2.diff (5.1 KB) - added by chriscct7 3 years ago.
38922.2.diff (2.5 KB) - added by adamsilverstein 3 years ago.
38922.3.diff (4.2 KB) - added by adamsilverstein 2 years ago.
38922.4.diff (5.4 KB) - added by rmccue 2 years ago.
Deprecate function further, and update docs
38922.5.diff (5.6 KB) - added by adamsilverstein 2 years ago.
38922.6.diff (5.6 KB) - added by adamsilverstein 2 years ago.
38922.7.diff (5.4 KB) - added by ryelle 20 months ago.
38922.8.diff (5.4 KB) - added by adamsilverstein 18 months ago.
38922.9.diff (3.8 KB) - added by adamsilverstein 18 months ago.
38922.10.diff (681 bytes) - added by dlh 18 months ago.
38922.11.diff (4.4 KB) - added by adamsilverstein 17 months ago.

Download all attachments as: .zip

Change History (81)

@nacin
3 years ago

#1 @nacin
3 years ago

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.

#2 @nacin
3 years ago

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

#3 @chriscct7
3 years ago

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.

Last edited 3 years ago by chriscct7 (previous) (diff)

@chriscct7
3 years ago

#4 @adamsilverstein
3 years ago

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

#5 follow-up: @afercia
3 years ago

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.

Last edited 2 years ago by afercia (previous) (diff)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


3 years ago

#7 follow-up: @swissspidy
3 years ago

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

#8 in reply to: ↑ 7 @afercia
3 years ago

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.

#9 in reply to: ↑ 5 @adamsilverstein
2 years ago

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_post_tag?) to reduce the returned fields to only those that we need.

Last edited 2 years ago by adamsilverstein (previous) (diff)

#10 @adamsilverstein
2 years ago

@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.

#11 follow-up: @jnylen0
2 years ago

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.

#12 @afercia
2 years ago

@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.

#13 in reply to: ↑ 11 @adamsilverstein
2 years ago

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
2 years ago

Deprecate function further, and update docs

#14 @rmccue
2 years ago

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.

This ticket was mentioned in Slack in #core-restapi by rmccue. View the logs.


2 years ago

#16 @adamsilverstein
2 years ago

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

#17 @adamsilverstein
2 years ago

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

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


2 years ago

#19 @adamsilverstein
2 years ago

  • Keywords commit added

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.

#20 @adamsilverstein
2 years ago

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.

#21 @adamsilverstein
2 years ago

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.

#22 @adamsilverstein
2 years ago

@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?

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-restapi by rmccue. View the logs.


23 months ago

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


22 months ago

This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.


22 months ago

#28 @adamsilverstein
22 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#29 @adamsilverstein
21 months ago

  • Milestone changed from Awaiting Review to 5.0

@ryelle
20 months ago

#30 @ryelle
20 months ago

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.

#31 @adamsilverstein
19 months ago

@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:

https://cl.ly/142N1w290o2k/Edit_Post__WordPress_Local_Development...__WordPress_2017-12-11_10-39-55.jpg

Last edited 19 months ago by adamsilverstein (previous) (diff)

#32 @adamsilverstein
18 months ago

  • Milestone changed from 5.0 to 4.9.4

#33 @adamsilverstein
18 months ago

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

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.

#34 @adamsilverstein
18 months ago

  • Milestone changed from 4.9.4 to 5.0

#35 follow-up: @adamsilverstein
18 months ago

In 42619:

Remove unit tests for deprecated ajax tag search function.

Fixes unit tests failing since r42614.

Ammends [42614].
See #38922.

#36 @afercia
18 months ago

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

#37 @afercia
18 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#38 in reply to: ↑ 35 @ocean90
18 months ago

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?

#39 @adamsilverstein
18 months ago

@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.

#40 @adamsilverstein
18 months ago

@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.

#41 @adamsilverstein
18 months ago

@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?

Last edited 18 months ago by adamsilverstein (previous) (diff)

#42 follow-up: @adamsilverstein
18 months ago

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

#43 @afercia
18 months ago

@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?

#44 follow-up: @afercia
18 months ago

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).

#45 in reply to: ↑ 44 @adamsilverstein
18 months ago

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)

#46 @afercia
18 months ago

@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.

#47 @adamsilverstein
18 months ago

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

#48 @afercia
18 months ago

  • Keywords needs-dev-note added

Agree needs-dev-note is appropriate here.

@dlh
18 months ago

#49 @dlh
18 months ago

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.

#50 @adamsilverstein
17 months ago

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

#51 in reply to: ↑ 42 @ocean90
17 months ago

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

👍

#53 @adamsilverstein
17 months ago

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

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.

#54 @danielbachhuber
13 months ago

  • Keywords has-patch commit needs-dev-note removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#55 @adamsilverstein
13 months ago

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?

#56 @danielbachhuber
13 months ago

@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.

#57 @adamsilverstein
13 months ago

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.

#58 @enrico.sorcinelli
13 months ago

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.

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


12 months ago

#60 @danielbachhuber
10 months ago

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.

#61 @danielbachhuber
9 months ago

  • Keywords needs-patch added
  • Milestone changed from 5.0 to 5.1

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.

#62 @jrf
7 months ago

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

#63 @danielbachhuber
7 months ago

This should be reverted, given:

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

#64 @adamsilverstein
7 months ago

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

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


6 months ago

#66 @desrosj
6 months ago

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.

#67 @desrosj
6 months ago

In 44538:

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

This was mistakenly removed in [44537].

See #38922.

#68 @desrosj
6 months ago

  • Milestone changed from 5.1 to Future Release

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

#69 @adamsilverstein
13 days ago

  • Owner adamsilverstein deleted
  • Status changed from reopened to reviewing
Note: See TracTickets for help on using tickets.