Add support for the format property in query
#7314
Conversation
Adds handling the format property ($request['format'] ) in the class WP_REST_Posts_Controller. Adds handling the format property ($block->context['query']['format']) in the function build_query_vars_from_query_block.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…er::test_registered_query_params
…oken logic In `build_query_vars_from_query_block`, the variable that contains the format attribute had the wrong name, which caused a PHP warning. The variable name was updated to `$formats`. One of the if-statements was closed incorrectly, which meant that all the logic for handling the format property was not used. Because of this, the wrong posts showed in the query loop block on the front of the site. The closing curly bracket for the if-statement was moved to solve this. One additional check was added to the wrapping if-statement: It now checks that `$block->context['query']['format']` is not empty, and is an array, before running the rest of the code.
Generated an updated fixture in `wp-api-generated.js` by running the PHPUnit tests. The update adds `format` to `mockedApiResponse.Schema`
… post formats. `WP_REST_Posts_Controller`: remove unnecessary array conversion for the `format` parameter. This addresses feedback that was left on the Gutenberg pull request.
… to `$query_params['format']` In `WP_REST_Posts_Controller` `get_collection_params` class method; add `uniqueItems` to the `$query_params['format']` array. The purpose is to require all items in the array to be unique. This will help manage user errors, if duplicate items are passed in the `format` parameter. This change addresses feedback left on the Gutenberg pull request.
Adds the regenerated fixtures which includes `uniqueItems` in the `format` array in `mockedApiResponse.Schema`. The fixture was regenerated by running the PHPUNit tests.
src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php
Outdated
Show resolved
Hide resolved
Use multiline comments instead of multiple single line comments.
There was a problem hiding this comment.
I've added some note inline, the most important being for validating the values when generating the query within the block. I'll leave it up to you to decide on how to handle invalid data.
It would also be good to see some unit tests added to ensure that the REST endpoint is returning an expected set of posts. Particularly for standard as the need to query for a non-existent taxomomy is a little unusual.
src/wp-includes/blocks.php
Outdated
| @@ -2420,6 +2421,52 @@ function build_query_vars_from_query_block( $block, $page ) { | |||
| } | |||
| } | |||
| } | |||
| if ( ! empty( $block->context['query']['format'] ) && is_array( $block->context['query']['format'] ) ) { | |||
| $formats = $block->context['query']['format']; | |||
There was a problem hiding this comment.
Validation: Ensure that the formats come from the supported values only provided by get_post_format_slugs().
$formats = array_intersect( $formats, get_post_format_slugs() );
src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php
Outdated
Show resolved
Hide resolved
src/wp-includes/blocks.php
Outdated
| @@ -2420,6 +2421,52 @@ function build_query_vars_from_query_block( $block, $page ) { | |||
| } | |||
| } | |||
| } | |||
| if ( ! empty( $block->context['query']['format'] ) && is_array( $block->context['query']['format'] ) ) { | |||
| $formats = $block->context['query']['format']; | |||
| $tax_query = array( 'relation' => 'OR' ); | |||
There was a problem hiding this comment.
My reading of this code is that if someone sets up a query for both formats and taxomomies, this will return posts that contain either.
Eg a query for post format: link; category: rest-api will return:
- Any posts with the post format link, regardless of category
- Any posts in the category rest-api, regardless of format.
There was a problem hiding this comment.
Yeah I think something is not working correctly.
If I use OR or AND, the result is the same: Posts that have both the category and the format.
I think the correct use is AND.
But then why is not OR returning all posts that have either the format or the category?
There was a problem hiding this comment.
Here is an example query loop for listing posts in category 4, with either the gallery or link format. The category id needs to be updated to match a category available on the installation.
<!-- wp:query {"queryId":1,"query":{"perPage":10,"pages":0,"offset":0,"postType":"post","order":"desc","orderBy":"date","author":"","search":"","exclude":[],"sticky":"","inherit":false,"format":["link","gallery"],"taxQuery":{"category":[4]}}} -->
<div class="wp-block-query"><!-- wp:post-template -->
<!-- wp:post-title /-->
<!-- /wp:post-template --></div>
<!-- /wp:query -->
There was a problem hiding this comment.
I have changed it to AND though it bothers me why OR is working the way I am seeing it in testing 😄
There was a problem hiding this comment.
🤔 In the WP_REST_Posts_Controller class, there is also:
// Enable filtering by both post formats and other taxonomies by combining them with `AND`.
if ( isset( $args['tax_query'] ) ) {
$args['tax_query'][] = array(
'relation' => 'AND',
$tax_query,
);
} else {
$args['tax_query'] = $tax_query;
}
There was a problem hiding this comment.
I am badly regretting not documenting why I added this relation in the first place.
Once I get back to work I will continue doing more logging of the different queries:
But I am now thinking that I meant for this to be the relation between the formats.
That I choose 'OR' because the user can query for both 'video' and 'link' and it needs to be 'OR' because a post can only have one format. It always have to list posts that has either 'video' or 'link', never both.
There was a problem hiding this comment.
Also I think $tax_query was not a good name for the variable, it too can cause confusion.
There was a problem hiding this comment.
After more testing and logging of $args['tax_query'] in the WP_REST_Posts_Controller class, I found that:
-
Setting the relation on the array to either
ORorANDworks when the query has two formats such as link and gallery. Because there is only one condition; the relation is not actually used, there are no conditions to create a relation between. -
Using
ORis needed when the query includesstandardand a non-default format such as link and gallery, because now there are two conditions.
So that is why the testing earlier showed the same results: I was not testing with a combination of standard.
In the `build_query_vars_from_query_block` function and the `WP_REST_Posts_Controller` class, a query with the `NOT EXISTS` operator is used to exclude all items that have a post format assigned. This commit removes a `terms` parameter from the two queries, because the terms are redundant when `NOT EXISTS` is used.
When querying for the `format` property in the `build_query_vars_from_query_block` function and the `WP_REST_Posts_Controller` class, the query may be combined with other taxonomy queries. For example, the user may expect the result to be posts with a specific format and a specific category. Previously, the `relation` parameter in the query was set to `OR`, which should return items that has either the format or the taxonomy. This commit changes the `relation` to `AND` with the intention to limit the result to items that have both the queried taxonomy and the format.
Validate the that the `format` property passed to the query using the block context is either the default (standard) or a supported post format, registered with `add_theme_support()`. Compare `format` to the supported formats in `get_post_format_slugs()` and remove invalid formats.
…en querying for the `format` When querying for the `format` property in the `WP_REST_Posts_Controller` class, the `relation` parameter in the query was incorrectly set to `AND`. This commit changes the relation back to `OR`. This is needed because when a user queries for a combination of the `standard` post format and any other format, the result should include items with either the given format, or no format (standard). A post item can not have more than one format. The commit also renames the variable `$tax_query` to `$formats_query` to improve the readability.
…g for the `format` When querying for the `format` property in `build_query_vars_from_query_block`, the `relation` parameter in the query was incorrectly set to `AND`. This commit changes the relation back to `OR`. This is needed because when a user queries for a combination of the `standard` post format and any other format, the result should include items with either the given format, or no format (standard). A post item can not have more than one format. The commit also renames the variable `$tax_query` to `$formats_query` to improve the readability.
This commit corrects inline docBlocks, removes unnecessary string concatenations, and tries to resolve a PHP coding standard issue about the formatting of an array.
When querying for formats, the relation between the format and other taxonomies like categoryíes and tags should be `AND`. The intention is to allow users to query for a post with a specific category and a specific format.
|
Hey @carolinan. JavaScript packages were updated in r59072 so you might wish to rebase this for easier testing. The deadline to commit this backport is 6.7 Beta 1 which is scheduled for 1 October. |
|
I tested this with the updated packages and was getting a block error when I tried to add the format. Is there something that needs to be merged to the packages in the GB repo before it should work?
It's a little confusing with the same variable name in both the legacy query, the tax query so I'm wondering if we can do a little more tidying up there: diff --git a/src/wp-includes/blocks.php b/src/wp-includes/blocks.php
index f3d138475f..25cb1c0d24 100644
--- a/src/wp-includes/blocks.php
+++ b/src/wp-includes/blocks.php
@@ -2346,6 +2346,7 @@ function build_query_vars_from_query_block( $block, $page ) {
'order' => 'DESC',
'orderby' => 'date',
'post__not_in' => array(),
+ 'tax_query' => array(),
);
if ( isset( $block->context['query'] ) ) {
@@ -2395,34 +2396,35 @@ function build_query_vars_from_query_block( $block, $page ) {
}
// Migrate `categoryIds` and `tagIds` to `tax_query` for backwards compatibility.
if ( ! empty( $block->context['query']['categoryIds'] ) || ! empty( $block->context['query']['tagIds'] ) ) {
- $tax_query = array();
+ $tax_query_back_compat = array();
if ( ! empty( $block->context['query']['categoryIds'] ) ) {
- $tax_query[] = array(
+ $tax_query_back_compat[] = array(
'taxonomy' => 'category',
'terms' => array_filter( array_map( 'intval', $block->context['query']['categoryIds'] ) ),
'include_children' => false,
);
}
if ( ! empty( $block->context['query']['tagIds'] ) ) {
- $tax_query[] = array(
+ $tax_query_back_compat[] = array(
'taxonomy' => 'post_tag',
'terms' => array_filter( array_map( 'intval', $block->context['query']['tagIds'] ) ),
'include_children' => false,
);
}
- $query['tax_query'] = $tax_query;
+ $query['tax_query'] = array_merge( $query['tax_query'], $tax_query_back_compat );
}
if ( ! empty( $block->context['query']['taxQuery'] ) ) {
- $query['tax_query'] = array();
+ $tax_query = array();
foreach ( $block->context['query']['taxQuery'] as $taxonomy => $terms ) {
if ( is_taxonomy_viewable( $taxonomy ) && ! empty( $terms ) ) {
- $query['tax_query'][] = array(
+ $tax_query[] = array(
'taxonomy' => $taxonomy,
'terms' => array_filter( array_map( 'intval', $terms ) ),
'include_children' => false,
);
}
}
+ $query['tax_query'] = array_merge( $query['tax_query'], $tax_query );
}
if ( ! empty( $block->context['query']['format'] ) && is_array( $block->context['query']['format'] ) ) {
$formats = $block->context['query']['format'];
@@ -2479,13 +2481,14 @@ function build_query_vars_from_query_block( $block, $page ) {
*/
if ( count( $formats_query ) > 1 ) {
// Enable filtering by both post formats and other taxonomies by combining them with `AND`.
- if ( isset( $query['tax_query'] ) ) {
- $query['tax_query'][] = array(
+ if ( empty( $query['tax_query'] ) ) {
+ $query['tax_query'] = $formats_query;
+ } else {
+ $query['tax_query'] = array(
'relation' => 'AND',
+ $query['tax_query'],
$formats_query,
);
- } else {
- $query['tax_query'] = $formats_query;
}
}
}However, the code looks good to me in terms of PHP, it's just the blcok crashing that needs to be figured out. |
|
I have submitted a pull request that fixes this bug and it is waiting for review. |
I don't mind :) |
|
@peterwilsoncc My only question is about the first |
…logic In function `build_query_vars_from_query_block`: Address feedback from the code review. - Update the variable name used for the tax query to improve readability. Inside the condition that migrates category- and tag id's for backwards compatibility, the new variable name for the tax query is `$tax_query_back_compat`. - Use `array_merge` to merge `$query['tax_query']` with the edited queries inside each condition. -Update the logic of the if/else statement used to determine wether the `format` parameter should combined with an existing tax query. The purpose of the update is to: -Ensure that the `AND` relation between the taxonomies and the post formats is nested at the correct level in the array. - Improve readability.
In `Tests_Blocks_wpBlock`, five test assertions were failing because the arrays used in the `assertSame` no longer matched the `$query` variable. The missmatch was caused by an update to the function `build_query_vars_from_query_block`. This commit updates the arrays in the test to match the function.
The suggested change always defined However, I'm glad you asked because I want to double check I didn't just introduce a bug. I'll double check that prior to merge. |
…oop filter This pull request copies changes that were made in WordPress core in WordPress/wordpress-develop#7314: The pull request that added the PHP changes related to #64167. - Improved logic for the use of the `AND` `relation` key when a user queries for both a format and a taxonomy. - Improved inline documentation, for clarity and readability. - Improved validation of the queried formats against the registered formats. - Misc: renaming of variables for better readability.
…oop filter (#66037) This pull request copies changes that were made in WordPress core in WordPress/wordpress-develop#7314: The pull request that added the PHP changes related to #64167. - Improved logic for the use of the `AND` `relation` key when a user queries for both a format and a taxonomy. - Improved inline documentation, for clarity and readability. - Improved validation of the queried formats against the registered formats. - Misc: renaming of variables for better readability. Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
…oop filter (#66037) This pull request copies changes that were made in WordPress core in WordPress/wordpress-develop#7314: The pull request that added the PHP changes related to WordPress/gutenberg#64167. - Improved logic for the use of the `AND` `relation` key when a user queries for both a format and a taxonomy. - Improved inline documentation, for clarity and readability. - Improved validation of the queried formats against the registered formats. - Misc: renaming of variables for better readability. Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Source: WordPress/gutenberg@b95553f
…oop filter (WordPress#66037) This pull request copies changes that were made in WordPress core in WordPress/wordpress-develop#7314: The pull request that added the PHP changes related to WordPress#64167. - Improved logic for the use of the `AND` `relation` key when a user queries for both a format and a taxonomy. - Improved inline documentation, for clarity and readability. - Improved validation of the queried formats against the registered formats. - Misc: renaming of variables for better readability. Co-authored-by: carolinan <poena@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
Adds handling the format property ($request['format'] ) in the class WP_REST_Posts_Controller.
Adds handling the format property ($block->context['query']['format']) in the function build_query_vars_from_query_block.
Trac ticket: https://core.trac.wordpress.org/ticket/62014
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.