Fix Product categories, Product Tags & Keyword filter not working in Products block #8377
Conversation
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: 0 B Total Size: 1.1 MB ℹ️ View Unchanged
|
imanish003
added
block-type: product-query
src/BlockTypes/ProductQuery.php
Outdated
| // "Product Categories" & "Product Tags" could be provided using "Filters" ToolsPanel available in Inspector Controls. | ||
| if ( isset( $query['tax_query'] ) ) { | ||
| $result['tax_query'] = $this->get_filter_by_product_categories_or_tags_query( $query['tax_query'] ); | ||
| } |
There was a problem hiding this comment.
@sunyatasattva @gigitux Can you please take a look at this PR? I have a query regarding these lines of code.
As I am not sure what else could be there in $query['tax_query'], therefore I have created get_filter_by_product_categories_or_tags_query function to extract queries related to only Product categories & Product tags.
I am wondering if we could use $query['tax_query'] as it is 🤔 Is there any possibility of extra items in $query['tax_query'] which we won't need & therefore we need to extract only what we need((For example, only Product categories & tags))?
There was a problem hiding this comment.
Also, I can see that there is get_global_query function. But code of this function only runs if is_custom_inherit_global_query_implementation_enabled is true.
Only this function extracts Keyword which could be provided using "Filters" ToolsPanel available in Inspector Controls. Don't we need Keyword when is_custom_inherit_global_query_implementation_enabled is false?
There was a problem hiding this comment.
I am wondering if we could use $query['tax_query'] as it is 🤔 Is there any possibility of extra items in $query['tax_query'] which we won't need & therefore we need to extract only what we need((For example, only Product categories & tags))?
I don't recall exactly the reasoning behind it, as @gigitux has worked on that part of the code, but I believe it is intentional. Perhaps something to do with the interaction with filters blocks if we take tax_query as it is?
Or maybe it was a prophylactic measure to enable the custom_inherit_global_query, which we eventually want to switch to.
Don't we need Keyword when is_custom_inherit_global_query_implementation_enabled is false?
Yes, we definitely do. It must have been an oversight we didn't catch, as we were focused on implementing the custom inherit, before the scope was changed.
There was a problem hiding this comment.
I don't remember very well, tbh 😭.
In any case, I think that we should not use the $query['tax_query'] as it is to be ready when we will switch to the custom inherit query logic.
We already have the logic to merge the queries.
There was a problem hiding this comment.
Hi @gigitux 👋
In any case, I think that we should not use the $query['tax_query'] as it is to be ready when we will switch to the custom inherit query logic.
Got it, then the changes I made in this PR looks good to me. In short, I created get_filter_by_product_categories_or_tags_query function to extract product_tag & product_cat from the tax_query as you can see here. Also, I extracted the keyword here. Please let me know if you notice any problem with this implementation.
We already have the logic to merge the queries.
Right, but while calling merge_queries function, we were not passing tax_query for product categories, product tags & keyword. I made the changes in this PR to pass these to merge_queries function.
Please let me know if I misunderstood something & if you see any problem with changes I made in this PR.
There was a problem hiding this comment.
@gigitux That makes sense. There are two sections of filters in Editor i.e. Advance Filters & Filters (see screenshot)
As per my understanding, we are creating queries for Advance filters in get_queries_by_attributes (which isn't obvious from the name of function).
I am thinking to do one of the following:
- Maybe we can rename this function to
get_filter_queries& return queries forAdvance Filters&Filtersfrom this function. - Rename this function to
get_advance_filter_queriesto make it more readable and clear. And, create new function calledget_filter_querieswhich will return queries for product categories, product tags & keyword i.e. all the filters. Then passget_filter_queries()as argument here.
Which of these you think would be better approach?
There was a problem hiding this comment.
As per my understanding, we are creating queries for Advance filters in get_queries_by_attributes (which isn't obvious from the name of function).
That function generates queries by the custom attributes that the merchant sets via the editor. Furthermore, I think that there should not be a relationship between the UI and the name of the function of the backend.
So, I would create another function to create a valid query object starting from the $tax_query object.
There was a problem hiding this comment.
That function generates queries by the custom attributes that the merchant sets via the editor.
Got it, maybe we can rename the function to get_queries_by_custom_attributes just to make it more readable & clear. What do you think? 🤔
That function generates queries by the custom attributes that the merchant sets via the editor. Furthermore, I think that there should not be a relationship between the UI and the name of the function of the backend.
So, I would create another function to create a valid query object starting from the $tax_query object.
That makes sense. I will create a new function. Thanks for the feedback 🙌🏻
There was a problem hiding this comment.
What do you think?
It makes sense! 👍
That makes sense. I will create a new function. Thanks for the feedback 🙌🏻
💪
There was a problem hiding this comment.
@gigitux I made the changes in this commit. Would you like to have a quick look?
There was a problem hiding this comment.
I'm getting some strange behaviour.
Sometimes when I input a category, I get this console error, and autocomplete doesn't work. Since the category input is a core one, it might be a bug from there, but worth mentioning:
Otherwise, when I input a broad category such as “Clothing”, in the front-end I only get the results of that category and not its subcategories. E.g. “Clothing > Hoodies” do not appear. I believe the expected result of when I choose a broad category, would be to show all products from that category, including children. What do you think?
Keywords and tags seem to work correctly.
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
status: stale
|
Hi @sunyatasattva, Thanks for the feedback 🙌🏻
I tried to reproduce it by adding & removing the categories, but I wasn't able to reproduce it. Can you please share the steps to reproduce it? IMO We can report the issue on GB repo once we know how to reproduce this issue.
Looks like this issue is there in Although, we can also fix it by fetching the sub-categories items. But, IMO it should be fixed in |
…he query. Functions: - get_filter_by_product_categories_or_tags_query - get_filter_by_keyword_query Also rename the function `get_queries_by_attributes` to `get_queries_by_custom_attributes` to make it more clear what it does.
imanish003
changed the title
|
He, @imanish003! I noticed that this fix doesn't work with custom taxonomies. This how to reproduce the issue:
A video that shows the issue. I should see only one product, the Screen.Capture.on.2023-02-16.at.11-11-37.mov |
As user can add custom taxonomies, we need to make sure that we include all taxonomies in the query. To fetch all product taxonomies, I used following function: ```php get_taxonomies( array( 'object_type' => array( 'product' ) ), 'names' ) ```
|
Hey @gigitux, That's an interesting corner case. Thanks for catching it 🙌🏻 I was using a static array for taxonomies: $product_taxonomies = [ 'product_cat', 'product_tag' ];In e6bd862 commit, I have used $product_taxonomies = get_taxonomies( array( 'object_type' => array( 'product' ) ), 'names' );Can you please have a look again? |
src/BlockTypes/ProductQuery.php
Outdated
| @@ -220,8 +222,8 @@ function( $acc, $query ) { | |||
| * duplicated items. | |||
| */ | |||
| if ( | |||
| ! empty( $merged_query['post__in'] ) && | |||
| count( $merged_query['post__in'] ) > count( array_unique( $merged_query['post__in'] ) ) | |||
| ! empty( $merged_query['post__in'] ) && | |||
There was a problem hiding this comment.
Not sure, but I guess that we should revert this formatting change.
src/BlockTypes/ProductQuery.php
Outdated
| @@ -337,8 +339,8 @@ private function get_stock_status_query( $stock_statii ) { | |||
| * meta query for stock status. | |||
| */ | |||
| if ( | |||
| count( $stock_statii ) === count( $stock_status_options ) && | |||
There was a problem hiding this comment.
Not sure, but I guess that we should revert this formatting change.
src/BlockTypes/ProductQuery.php
Outdated
| // phpcs:ignore WordPress.DB.SlowDBQuery.slow_db_query_meta_query | ||
| 'meta_query' => array( | ||
| array( | ||
| 'key' => '_stock_status', | ||
| 'value' => $filtered_stock_status_values, | ||
| 'operator' => 'IN', |
There was a problem hiding this comment.
Not sure, but I guess that we should revert this formatting change.
src/BlockTypes/ProductQuery.php
Outdated
| * | ||
| * @param WP_Query $query The query to extract the keyword filter from. | ||
| * @return array The keyword filter, or an empty array if none is found. | ||
| * @throws InvalidArgumentException If $query is not an array. |
There was a problem hiding this comment.
I thank that we should do this check, in this way, we will not throw any errors. Now we don't catch the error.
src/BlockTypes/ProductQuery.php
Outdated
| $this->get_queries_by_applied_filters() | ||
| $this->get_queries_by_custom_attributes( $parsed_block ), | ||
| $this->get_queries_by_applied_filters(), | ||
| $this->get_filter_by_taxonomies_query( $query['tax_query'] ), |
There was a problem hiding this comment.
| $this->get_filter_by_taxonomies_query( $query['tax_query'] ), | |
| $this->get_filter_by_taxonomies_query( isset($query['tax_query'] ? $query['tax_query']: array()), |
Now, if the Products block doesn't have any filter defined, tax_query doesn't exist, and WP returns an error.
|
Hey @gigitux, I truly appreciate your attention to detail 🚀 I have made the necessary revisions based on your recommendations. Can you please have a look again? |
This PR fix this issue, according to which following filters set from Inspector Controls for Products block aren't working:
Fixes #8249
Testing
WooCommerce Visibility
Changelog