Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Fix: Product Query editor preview with Stock Status setting #7682

Merged
merged 9 commits into from
Dec 5, 2022
Merged

Fix: Product Query editor preview with Stock Status setting #7682

Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
18de845
fix: update the rest query to include stock status meta query, so edi…
dinhtungdu Nov 15, 2022
4306b2f
bot: update checkstyle.xml
github-actions[bot] Nov 15, 2022
e2a170f
Merge branch 'trunk' into fix/product-query-preview-stock-status
dinhtungdu Nov 16, 2022
630a6c0
Merge branch 'trunk' into fix/product-query-preview-stock-status
dinhtungdu Nov 29, 2022
b17417e
Merge branch 'trunk' into fix/product-query-preview-stock-status
dinhtungdu Nov 29, 2022
c456f87
refactor the get_stock_status_query to return an empty array for empt…
dinhtungdu Nov 29, 2022
459d617
Merge branch 'trunk' into fix/product-query-preview-stock-status
dinhtungdu Nov 30, 2022
c0e1dbd
Merge branch 'trunk' into fix/product-query-preview-stock-status
dinhtungdu Dec 2, 2022
8c53a13
update stock query handling to align with attribute query
dinhtungdu Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion src/BlockTypes/ProductQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,12 @@ public function update_query( $pre_render, $parsed_block ) {
*/
public function update_rest_query( $args, $request ) {
$on_sale_query = $request->get_param( '__woocommerceOnSale' ) !== 'true' ? array() : $this->get_on_sale_products_query();
$stock_query = empty( $request->get_param( '__woocommerceStockStatus' ) ) ? array() : $this->get_stock_status_query( $request->get_param( '__woocommerceStockStatus' ) );

return array_merge( $args, $on_sale_query );
return $this->merge_queries(
array_merge( $args, $on_sale_query ),
$stock_query
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage of this over array_merge( $args, $on_sale_query, $stock_query )? We need to consider that we'll add more and more queries, already in #7687 I add another one which will conflict here, how should we do this in the future?

E.g.

return $this->merge_queries(
   array_merge( $args, $on_sale_query, $best_selling_query ),
   $stock_query
);

How do we decide what's $b in the merge_queries call?

My suggestion here is to actually refactor the merge_queries call to accept multiple arguments, like most PHP array functions do. So the signature would be merge_queries(array ...$queries). I had suggested it here when this function was implemented, but we didn't go ahead with it because the advantages were not clear.

Now that we meet this problem, I think the advantages of having that is clearer, because we'll keep adding queries and we should just be able to do:

return $this->merge_queries(
   $args, $on_sale_query, $best_selling_query, $stock_query
);

and don't worry about the internal implementation of that. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage of this over array_merge( $args, $on_sale_query, $stock_query )?

@sunyatasattva With the current implementation of the class and structure of WP_Query, we have to use merge_queries for tax and meta query as tax_query and meta_query are arrays containing arrays.

How do we decide what's $b in the merge_queries call?

As I mentioned above, queries containing tax_query and/or meta_query are $b.

What do you think?

I strongly agree. merge_queries should be the source of trust in merging queries. I can refactor it either in this PR or a follow-up PR. Tagging @gigitux here for more perspectives :)

Copy link
Member Author

@dinhtungdu dinhtungdu Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created #7697, so we have more things on the table to discuss.

}

/**
Expand Down Expand Up @@ -175,6 +179,18 @@ function( $acc, $query ) {
* @return array
*/
private function merge_queries( $a, $b ) {
/**
* Ensure the properties are arrays before merging to avoid fatal errors.
*/
$a = wp_parse_args(
$a,
array(
'post__in' => array(),
'meta_query' => array(),
'tax_query' => array(),
)
);

$a['post__in'] = ( isset( $b['post__in'] ) && ! empty( $b['post__in'] ) ) ? $this->intersect_arrays_when_not_empty( $a['post__in'], $b['post__in'] ) : $a['post__in'];
$a['meta_query'] = ( isset( $b['meta_query'] ) && ! empty( $b['meta_query'] ) ) ? array_merge( $a['meta_query'], array( $b['meta_query'] ) ) : $a['meta_query'];
$a['tax_query'] = ( isset( $b['tax_query'] ) && ! empty( $b['tax_query'] ) ) ? array_merge( $a['tax_query'], array( $b['tax_query'] ) ) : $a['tax_query'];
Expand Down