-
Notifications
You must be signed in to change notification settings - Fork 219
Product Query: pass any product taxonomies existing in the URL parameters. #6152
Changes from all commits
c517960
4a32bc5
e53b264
6cd5777
ca9c215
b4905b2
ffb7b02
90a1320
06b34f7
9831249
8843a78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,12 +95,24 @@ public function prepare_objects_query( $request ) { | |
'and' => 'AND', | ||
]; | ||
|
||
// Gets all registered product taxonomies and prefixes them with `tax_`. | ||
// This is neeeded to avoid situations where a users registers a new product taxonomy with the same name as default field. | ||
// eg an `sku` taxonomy will be mapped to `tax_sku`. | ||
$all_product_taxonomies = array_map( | ||
function ( $value ) { | ||
return '_unstable_tax_' . $value; | ||
}, | ||
get_taxonomies( array( 'object_type' => array( 'product' ) ), 'names' ) | ||
); | ||
|
||
// Map between taxonomy name and arg key. | ||
$taxonomies = [ | ||
$default_taxonomies = [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question @tjcafferkey. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@cpapazoglou it looks like after the merge it uses There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤦, sorry for this and good catch. I was merging with wrong order. Fixed here 6885fd2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tjcafferkey is the following preferable?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @cpapazoglou this seems to work as expected now. However like @Aljullu mentioned I will leave the final review for @opr There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think as it currently is works fine. |
||
'product_cat' => 'category', | ||
'product_tag' => 'tag', | ||
]; | ||
|
||
$taxonomies = array_merge( $all_product_taxonomies, $default_taxonomies ); | ||
|
||
// Set tax_query for each passed arg. | ||
foreach ( $taxonomies as $taxonomy => $key ) { | ||
if ( ! empty( $request[ $key ] ) ) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should check whether the slug after
tax_
is a real taxonomy on the site before adding this param?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that as a problem, the parameter will be collected anyway but will be disregarded here
https://github.com/woocommerce/woocommerce-gutenberg-products-block/blob/5be9023bcbc1918f84e9951f0acca114dcca99d4/src/StoreApi/Utilities/ProductQuery.php#L101-L127
I don't have strong opinion on this though, just want to avoid two more operation (get all taxonomies, check against the parameter passed).