Add tax query #2145 #2273
Add tax query #2145 #2273
Conversation
Adds tax query support to GET /wp/v2/posts/. Added a tax query validation function as well. Hopefully I can pass travis first time for once!
Fix post controller test with comma
|
Test failing because tax_query is potentially a slow query so is it not a good idea to support this? Need assistance! Is my validation what is throwing that error? |
|
This goes somewhat hand-in-hand with date query, I'm not sure whether we want to match the same naming as WP internally for the fields, or if we should support the full tax query. A first iteration may be good to allow |
Reverting yoda conditions to the way they were.
…ax-query-WP-API#2145 # Conflicts: # lib/endpoints/class-wp-rest-posts-controller.php
Resolving Conflicts
Exposing tax_query in the filter as opposed to a query param.
|
So at this point it is probably ridiculous to merge this one line of code in. So @joehoyle if you want to add this into #2287 it would probably make sense to do it there instead of 8 commits for one line of code haha! If I am missing something that needs to be done with tax_query as a filter param let me know. Currently it works well with something like: GET wp/v2/posts/?filter[tax_query][0][taxonomy]=category&filter[tax_query][0][field]=slug&filter[tax_query][0][terms]=uncategorized I think this combined with Joe's #2287 should solve #2145 and #2277 and add some nice parameters to handle taxonomy querying. |
|
I think we'll still want this PR, as it adds it to filter. I didn't use this technique in #2287, as we need to providing whitelisting for the taxonomies in |
|
@joehoyle I had a validation function that validated the tax_query itself which did include white listing minus the show_in_rest => true. I will try to figure out a way to add that in. Do we want that in depth of validation of the filter param if tax_query is set or do we just want to white list taxonomies that are show_in_rest => true? |
Adding validation back in for tax_query as a filter param. Also added a function for validating filter params. Not sure if this is necessary, it runs relatively fast still feedback appreciated for what direction this should take.
|
Definitely will need some feedback on the direction for this latest commit. I tested it with #2287 and there are no conflicts. So if you wanted to find posts that were both in tags 1 and categories 1 then with #2287 you could do something like this. GET wp/v2/posts/?tags=1&categories=1&filter[tax_query][relation]=AND I think this in combination with @joehoyle 's work will make handling more robust tax queries a lot simpler. Feedback is always appreciated! |
Holy moly. I need to think on this one for a little bit. |
|
@danielbachhuber I spoke briefly with @rachelbaker one night about validation of the filter param. We both kind of came to the conclusion that it would be overkill. So we can decide whether we want to selectively validate certain arguments passed into the filter or whether we want none at all. I think validating all of them would just be too much code and as new WP_Query args are added it would make maintaining the REST API more difficult as well. I'm pretty sure WP_Query() validates the arguments passed to it so the validation only really serves as a way to tell the user they made a bad request, which might be important, might not. As we have more top level params, that cover a lot of use cases, I imagine the filter param will be used less and probably by power users who will understand WP_Query so validation may not be necessary at all. @joehoyle , @rmccue , @rachelbaker , @danielbachhuber thoughts would be appreciated! |
|
@BE-Webdesign the problem with not sanitizing / filtering the tax_query is we would be potentially allowing taxonomies to be queried that have specifically not been opted into the REST API via I think the sanitize function for tax_query shouldn't be too difficult when it gets down to it. Especially if we on'y check the taxonomy names, rather than a full sanitization of all values. |
|
@joehoyle Okay so cut this down to just checking for valid taxonomy names which will be queried against for show_in_rest => true? Will other filter params be validate/sanitized at some point as well? |
| continue; | ||
| } | ||
| } | ||
| return $is_valid; |
There was a problem hiding this comment.
If there are no $values then would't this code throw an error since $is_valid is not declared anywhere? I would just return true here if you want that to be the default value, or declare $is_valid = true before the foreach.
There was a problem hiding this comment.
Ya I should put an empty check there and have some declaration of $is_valid, most likely this is being tossed anyways for something simpler. This is more of an experiment rather than what is being merged. But thank you for pointing that out because that is a glaring mistake haha 😄
Simplifying validation. Condensed the validation function to just worry about the relation parameter whether it is AND or OR, and making sure that taxonomies that are queried against only have show_in_rest => true
|
Going to simplify this some more. |
Adding tests for valid filter and invalid filter for tax queries.
Simplifies validation more.
|
Failing for use of tax_query potentially being a slow query. |
|
@kjbenk Thanks for the tip!!! Going to put it in. |
Adding in WPCS whitelisting for tax_query. A great suggestion by kjbenk.
tests/test-rest-posts-controller.php
Outdated
| $request = new WP_REST_Request( 'GET', '/wp/v2/posts' ); | ||
| $request->set_param( 'filter', array( | ||
| // WPCS: tax_query ok. | ||
| 'tax_query' => array( |
There was a problem hiding this comment.
You need to add the comments inline. So this would be the correct syntax 'tax_query' => array( // WPCS: tax_query ok. or at least I think :)
Fixing CodeSniffer tests.
Causes PHP CodeSniffer to ignore the tax query lines.
Current coverage is 94.57% (diff: 97.29%)@@ develop #2273 diff @@
==========================================
Files 11 11
Lines 3612 3649 +37
Methods 172 174 +2
Messages 0 0
Branches 0 0
==========================================
+ Hits 3415 3451 +36
- Misses 197 198 +1
Partials 0 0
|
| } | ||
| continue; | ||
| default : | ||
| continue; |
There was a problem hiding this comment.
Nitpick, but for a single-case switch I'd vote in favor of an if. (unless this is a code standard of which I was not aware)
There was a problem hiding this comment.
Yeah it was originally an entire validation of tax_query, but now it handles only one case. If this ever wanted to be expanded though it should go back to switch. I can make a modification to it sometime soon.
Fixes WP-API#2145, this was put into a new PR to make it cleaner. Original PR is in WP-API#2273. This enables `tax_query` in the `filter` param and validates that the taxonomies that are queried are `show_in_rest => true`
|
Closed in favor of #2748. |
6 participants
Adds tax query support to GET wp/v2/posts with custom validation function for #2145. Let me know what we can do to improve this.
Simple call for posts in the uncategorized category taxonomy.
/wp/v2/posts?tax_query[0][taxonomy]=category&tax_query[0][field]=slug&tax_query[0][terms]=uncategorized
More Complex:
/wp/v2/posts?tax_query[relation]=OR&tax_query[0][taxonomy]=category&tax_query[0][field]=slug&tax_query[0][terms][0]=quotes&tax_query[1][relation]=AND&tax_query[1][0][taxonomy]=post_format&tax_query[1][0][field]=slug&tax_query[1][0][terms][0]=post-format-quote&tax_query[1][1][taxonomy]=category&tax_query[1][1][field]=slug&tax_query[1][1][terms][0]=wisdom