Refactor ProductQuery::merge_queries to accept multiple queries
#7697
Conversation
|
The release ZIP for this PR is accessible via: |
TypeScript Errors ReportFiles with errors: 428 🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: 0 B Total Size: 970 kB ℹ️ View Unchanged
|
dinhtungdu
changed the title
ProductQuery::mereg_queries to accept multiple queriesProductQuery::merge_queries to accept multiple queries
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the |
and preparing the input arrays
dinhtungdu
added
status: needs review
type: refactor
dinhtungdu
added
the
skip-changelog
src/BlockTypes/ProductQuery.php
Outdated
| 'suppress_filters', | ||
| 'tax_query', | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Maybe it is premature optimization, but we recreate $valid_query_vars array for each query. Isn't it better to move the creation of $valid_query_vars outside this function?
There was a problem hiding this comment.
Thank you for the head-up. I addressed this in 0a3bb43. To me it's more readability than performance improvement as we don't process more than two level depth arrays at the moment, and we still need to recreate the array each time the class is initialized. But it makes the code easier to read so it makes sense to me to implement the optimization.
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
|
@sunyatasattva Can I have your review on this PR as well, as you're working on Adding support for filtering by attributes within the block? |
src/BlockTypes/ProductQuery.php
Outdated
| if ( empty( array_intersect( $this->get_valid_query_vars(), array_keys( $query ) ) ) ) { | ||
| return $this->merge_queries( $acc, ...array_values( $query ) ); | ||
| } | ||
| return $this->array_merge_recursive_replace_non_array_properties( $acc, $query ); |
There was a problem hiding this comment.
Would you mind explaining a little bit more about this logic?
- What do you mean we “unpack it”?
- What are the use-cases for non valid query keys? Are they our custom
__woocommercequeries? - Why do we recursively replace them?
I'd just like to understand the entire flow a bit more and why these things are needed (and perhaps it could be useful to document them in the PHPDocs for posterity).
There was a problem hiding this comment.
- What do you mean we “unpack it”?
It's restructuring. I used unpack as PHP uses that word. I updated the comment to include both (unpack/destructure).
- Why do we recursively replace them?
We recursively replace only non-array values. array_merge_recursive(), which I used initially, will merge the value with the same key, but it's not what we want. For example:
$base = ['orderby' => 'date']
$new = ['orderby' => 'meta_value_num']
With array_merge_recursive(), the result will be: ['orderby' => ['date', 'meta_value_num']]
But we want this: ['orderby' => 'meta_value_num']
- What are the use-cases for non valid query keys? Are they our custom __woocommerce queries?
Explaining this makes me think I might be overthinking.
Are they our custom __woocommerce queries?
No. Query keys (or query vars) here are WP_Query variables. Using valid here is misleading, it should be built-in instead. I check if the current array contains the built-in query vars to deal with unknown incoming arrays. For example:
If we feed merge_queries with an array like:
[
[
'meta_query' => [
...<meta_queries_here>
]
]
]
merge_queries will restructure the array until it gets an array containing the meta_query key, which is a built-in query variable, then starts merging.
But, as this is a private method, which is only used inside the ProductQuery class, so we can control the inputs of merge_queries calls, I think this logic can be simplified by destructuring until we get the associative arrays, new commit is coming...
There was a problem hiding this comment.
But, as this is a private method, which is only used inside the ProductQuery class, so we can control the inputs of
merge_queriescalls, I think this logic can be simplified by destructuring until we get the associative arrays, new commit is coming...
We can't do this because we're returning associative arrays, for example get_queries_by_attributes() or get_queries_by_applied_filters().
private function get_queries_by_applied_filters() {
return array(
'price_filter' => $this->get_filter_by_price_query(),
'attributes_filter' => $this->get_filter_by_attributes_query(),
'stock_status_filter' => $this->get_filter_by_stock_status_query(),
);
}
We can check and unpack/destructure array contains those keys (price_filter, attributes_filter, stock_status_filter), but we will have to update the logic every time we add more queries.
There was a problem hiding this comment.
After considering, I reverted e6df617. I think the benefit of a versatile merge_queries() method outperforms the complexity checking for valid query variables.
There was a problem hiding this comment.
Thanks a lot @dinhtungdu for the amazing explanation and in-depth commentary! This was extremely helpful to understand this somewhat convoluted logic. And I agree that simplifying is better in this case: glad that some rubberducking was also useful to you.
Just a small note:
What do you mean we “unpack it”?
It's restructuring. I used unpack as PHP uses that word. I updated the comment to include both (unpack/destructure).
By this you mean: ...array_values( $query ) right? In which case, I guess we call this “unpacking” “spreading” and not “destructuring” in the JS world:
Destructuring is used to create varibles from array items or object properties. Spread syntax is used to unpack iterables such as arrays, objects, and function calls.
Just in case you want to update your comment. But it's not a big deal. Thanks a lot for unpacking ( 🤭 ) the code for me.
There was a problem hiding this comment.
By this you mean:
...array_values( $query )right? In which case, I guess we call this “unpacking” “spreading” and not “destructuring” in the JS world:
Just in case you want to update your comment. But it's not a big deal. Thanks a lot for unpacking ( 🤭 ) the code for me.
I usually misuse those terms 🤦🏽. I updated the code. Thanks so much, it's easier to memorize for me now 🥰
|
Thanks @dinhtungdu, it looks really good to me. I just have a clarification question, but otherwise the code seems very solid and I like this API more! 👏 |
Instead of checking if array contains built-in query variables, we only check if the array contains string keys as we can control the inputs of merge_queries.
This reverts commit e6df617.
This PR refactors
ProductQuery::merge_queriesmethod to accept multiple arguments. This makesmerge_queriesthe single source of trust in merging queries for Product Query blocks. This PR also improves the readability of theProductQueryclass.More context: #7682 (comment)
Fixes #7745
Testing
Automated Tests
User Facing Testing
Ensure products and order of products are the same as before refactoring.
WooCommerce Visibility
Performance Impact
N/a