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

Products block: tax_query should be calculated only if __woocommerceAttributes is valid #9049

Merged
merged 4 commits into from
Apr 18, 2023
Merged

Products block: tax_query should be calculated only if __woocommerceAttributes is valid #9049

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b0f2ca5
Fix: tax_query should be calculated only if __woocommerceAttributes i…
imanish003 Apr 14, 2023
e606957
Merge branch 'trunk' into fix/8765-rest-api-products-block-bug
imanish003 Apr 14, 2023
24a2f8e
Merge branch 'trunk' into fix/8765-rest-api-products-block-bug
imanish003 Apr 17, 2023
8fdbb43
Merge branch 'trunk' into fix/8765-rest-api-products-block-bug
imanish003 Apr 18, 2023
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
23 changes: 13 additions & 10 deletions src/BlockTypes/ProductQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,16 +135,19 @@ private function merge_tax_queries( ...$queries ) {
* @param array $args Query args.
* @param WP_REST_Request $request Request.
*/
public function update_rest_query( $args, $request ) {
$orderby = $request->get_param( 'orderby' );
$woo_attributes = $request->get_param( '__woocommerceAttributes' );
$woo_stock_status = $request->get_param( '__woocommerceStockStatus' );
$on_sale_query = $request->get_param( '__woocommerceOnSale' ) === 'true' ? $this->get_on_sale_products_query() : array();
$orderby_query = isset( $orderby ) ? $this->get_custom_orderby_query( $orderby ) : array();
$attributes_query = is_array( $woo_attributes ) ? $this->get_product_attributes_query( $woo_attributes ) : array();
$stock_query = is_array( $woo_stock_status ) ? $this->get_stock_status_query( $woo_stock_status ) : array();
$visibility_query = $this->get_product_visibility_query( $stock_query );
$tax_query = $this->merge_tax_queries( $attributes_query, $visibility_query );
public function update_rest_query( $args, $request ): array {
$woo_attributes = $request->get_param( '__woocommerceAttributes' );
$is_valid_attributes = is_array( $woo_attributes );
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we return early at this point? Ie:

if ( ! is_array( $woo_attributes ) ) {
  return $args;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides that, I see there is no changelog entry in the PR, is that intentional? I think this change might be worth mentioning in the changelog.

To be honest, I wasn't completely certain if this needed a changelog entry. However, I've now added one just in case. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could we return early at this point? Ie:

I considered adding an early return, but I realized we are using three attributes:

  • __woocommerceStockStatus
  • __woocommerceAttributes
  • __woocommerceStockStatus

What if __woocommerceAttributes is null, but the other attributes have correct values? I wanted to avoid this corner case. However, I'm not certain if this situation will ever occur. 🤷🏻‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right, I didn't think that __woocommerceAttributes could be undefined while the other could be defined. 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Aljullu In my opinion, let's maintain the current implementation for now so we can release the fix. If necessary, I can revisit and make adjustments later. Does that sounds good to you? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure thing!

$orderby = $request->get_param( 'orderby' );
$woo_stock_status = $request->get_param( '__woocommerceStockStatus' );
$on_sale = $request->get_param( '__woocommerceOnSale' ) === 'true';

$on_sale_query = $on_sale ? $this->get_on_sale_products_query() : [];
$orderby_query = $orderby ? $this->get_custom_orderby_query( $orderby ) : [];
$attributes_query = $is_valid_attributes ? $this->get_product_attributes_query( $woo_attributes ) : [];
$stock_query = is_array( $woo_stock_status ) ? $this->get_stock_status_query( $woo_stock_status ) : [];
$visibility_query = is_array( $woo_stock_status ) ? $this->get_product_visibility_query( $stock_query ) : [];
$tax_query = $is_valid_attributes ? $this->merge_tax_queries( $attributes_query, $visibility_query ) : [];

return array_merge( $args, $on_sale_query, $orderby_query, $stock_query, $tax_query );
}
Expand Down