Update build_query_vars_from_query_block to handle new taxQuery structure#10632
Update build_query_vars_from_query_block to handle new taxQuery structure#10632ntsekouras wants to merge 7 commits intoWordPress:trunkfrom
build_query_vars_from_query_block to handle new taxQuery structure#10632Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| $tax_query = array(); | ||
| // If there are keys other than include/exclude, it's the old | ||
| // format e.g. "taxQuery":{"category":[4]} | ||
| if ( ! empty( array_diff( array_keys( $tax_query_input ), array( 'include', 'exclude' ) ) ) ) { |
There was a problem hiding this comment.
Technically someone could have a taxonomy named include or exclude, no?
There was a problem hiding this comment.
Technically yes, but I'd consider it very hard to encounter.. This affects the REST API too and people can't use it properly since there is already the include to limit the result set to specific IDs and the respective exclude arg.
Not sure if we should consider updating register_taxonomy to have some reserved keys or update the REST API to somehow handle these cases, but maybe it's not worth it since it seems quite edge case to have taxonomies with these names? 🤔
peterwilsoncc
left a comment
There was a problem hiding this comment.
For the previous migration, we converted the old format to the new prior to constructing the query.
wordpress-develop/src/wp-includes/blocks.php
Lines 2697 to 2715 in edaed03
I think migrating will make it easier for future developers following the code so I'd suggest doing so here.
As this is the third change to the taxQuery format, it might be worht doing it in a serate function wp_migrate_query_block_tax_query_or_something()
We do the same with
I thought about that while implementing this, but I felt it's better to keep it as is because:
Said that, if you have a strong opinion on this I can update, but I don't feel it will make much difference personally. |
|
Do you think we can land as is based on my last comment or it's something more to address? We need to land this soon for 7.0. Thanks! |
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
Co-authored-by: Weston Ruter <westonruter@gmail.com>
b4769ec to
7b90e4b
Compare
|
I didn't justify my approval, but just quickly:
|
Trac ticket: https://core.trac.wordpress.org/ticket/64416
In GB we've updated the structure of Query Loop's taxQuery to also handle exclusion of terms in this PR (WordPress/gutenberg#73790).
The new structure is:
We need to update build_query_vars_from_query_block to handle this new taxQuery structure.
Testing instructions
Since this change should be handling Query Loop structure for 7.0 after the packages sync, we can test by:
Query Loop(through inspector controls) and add ataxonomy:categoryfilter with your created category.taxQuerystructure. Make sure to update thecategory idfrom the snippet, with the category id you've created. This refers to the"taxQuery":{"include":{"category":[3]}}part, where you should probably need to update the3.This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.