Opened 8 years ago
Last modified 4 years 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)
Change History (82)
#2
@
8 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
@
8 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.
#4
@
8 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:
↓ 9
@
8 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.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
8 years ago
#7
follow-up:
↓ 8
@
8 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
@
8 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
@
8 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.
#10
@
8 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:
↓ 13
@
8 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
@
8 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.
#14
@
8 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.
8 years ago
#16
@
8 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
@
8 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.
8 years ago
#19
@
8 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
@
8 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
@
8 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
@
8 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.
8 years ago
This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-restapi by rmccue. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.
7 years ago
This ticket was mentioned in Slack in #core-restapi by adamsilverstein. View the logs.
7 years ago
#30
@
7 years 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
@
7 years 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:
#36
@
7 years ago
Shouldn't this change have the keyword needs-dev-note
? Is it safe to introduce it in a minor release?
#37
@
7 years 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:
- using the keyboard, highlight one of the results in the suggestions
- press Enter
- the term gets added
Now:
- repeat 1 and 2
- the term is not added, it's just inserted in the input field
- now you have to press Enter again to actually add the term
#38
in reply to:
↑ 35
@
7 years ago
Replying to adamsilverstein:
In 42619:
What was the reason for not using the @expectedDeprecated
annotation?
#39
@
7 years 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
@
7 years 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
@
7 years 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?
#42
follow-up:
↓ 51
@
7 years ago
- 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
@
7 years 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:
↓ 45
@
7 years 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
@
7 years 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
@
7 years 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
@
7 years ago
Ok, will do once I resolve the unit test issue raised by @ocean90 - awaiting his feedback.
#49
@
7 years 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
@
7 years ago
Thanks for catching that @dlh I'll get that in with the test update in 38922.9.diff.
#51
in reply to:
↑ 42
@
7 years ago
Replying to adamsilverstein:
- 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
👍
#52
@
7 years ago
38922.11.diff is 38922.9.diff + 38922.10.diff refreshed against trunk
#54
@
6 years 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
@
6 years 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
@
6 years 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
@
6 years 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
@
6 years 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.
6 years ago
#60
@
6 years 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
@
6 years 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.
#63
@
6 years ago
This should be reverted, given:
- It doesn't work for custom taxonomies.
- Our efforts should generally be applied to Gutenberg.
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.