6.0 Backport to add support for custom taxonomies filtering for Query Loop (from Gutenberg) #2488
Conversation
hellofromtonya
changed the title
| $term_ids = array_filter( $term_ids ); | ||
| $query['category__in'] = $term_ids; | ||
| // Migrate `categoryIds` and `tagIds` to `tax_query` for backwards compatibility. | ||
| if ( ! empty( $block->context['query']['categoryIds'] ) || ! empty( $block->context['query']['tagIds'] ) ) { |
There was a problem hiding this comment.
I can't see why this outer if is needed, the same checks are being done inside it.
There was a problem hiding this comment.
Just to 'encapsulate' (let's say) the 'migration' code. Also, both old attributes merge into a single one and without the extra if we would have more checks about new value and merge, etc..
I think there is no harm to leave this as is.
| $term_ids = array_filter( $term_ids ); | ||
| $query['tag__in'] = $term_ids; | ||
| if ( ! empty( $block->context['query']['taxQuery'] ) ) { | ||
| $query['tax_query'] = array(); |
There was a problem hiding this comment.
This looks like it will stomp on any categories and tags in the shim above if the query block has both a legacy value and taxQuery defined.
There was a problem hiding this comment.
I'm not sure if it will be possible to have the old and new attributes, as there is the client parsing that migrates the block. So in the case of deprecated blocks, we just fill the new attribute from the old ones.
There was a problem hiding this comment.
I'm not sure if it will be possible to have the old and new attributes, as there is the client parsing that migrates the block.
Thanks, I gather that will come in as part of a package.
There was a problem hiding this comment.
Using an else if instead of an if would make this explicit for the future readers of this code. I'm thinking:
else if ( ! empty( $block->context['query']['taxQuery'] ) ) {Not a blocker for the backport, though.
|
@ntsekouras can you help with backporting this functionality? |
|
I also see files listed in the description that @jsnajdr added, so let's keep him in the loop. |
Looking now. Thanks for the ping! |
|
The sanitization fixes have been merged in GB, so we should update this PR with the changes from that one. |
|
|
Code changes look good pending fixes to the tests. Let's keep the existing blocks in the test suite (ie with tagId, etc) and modify the expected result. The new format can go in as additional tests. (I say this in the full knowledge it might be part of your own plan) |
…ctory-controller.php.
src/wp-includes/rest-api/endpoints/class-wp-rest-block-pattern-categories-controller.php
Outdated
Show resolved
Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-block-patterns-controller.php
Outdated
Show resolved
Hide resolved
|
This PR captures block patterns and adding custom taxonomies filtering as changes are intermingled in the files. See the list of files backported in the description. @ntsekouras @ironprogrammer Can you double-check that all changes from the files listed are included in this PR please? Some of the changes "seemed" specific to the Gutenberg plugin; so I didn't include them.
Tests are failing. I'll fix those once confirmed that all changes to be backported from those files are here in this PR. |
|
In response to @hellofromtonya:
I concur that
Tests needed the new routes to be registered. Please see PR 3 above, which includes updates that get those working. Please review the amendments and feel free to slurp into your PR any changes you agree with. Please note that for lack of spelunking time today, I did not address the failure for |
|
As this contains REST API, someone ( including me ) for the REST API maintainer team should really review this before it get merged into core. |
| } | ||
|
|
||
| $request = new WP_REST_Request( 'GET', '/wp/v2/pattern-directory/patterns' ); | ||
| $request['slug'] = implode( ',', $pattern_settings ); |
There was a problem hiding this comment.
The implode is unnecessary. The API can accept real arrays.
There was a problem hiding this comment.
I get the following error if I replace
$request['slug'] = implode( ',', $pattern_settings );with
$request['slug'] = $pattern_settings;:
Uncaught TypeError: rawurlencode(): Argument #1 ($string) must be of type string, array given in /src/wp-content/plugins/gutenberg/lib/compat/wordpress-6.0/class-gutenberg-rest-pattern-directory-controller.php:79
This error doesn't occur if implode gets called.
There was a problem hiding this comment.
I think you are right. Gutenberg_REST_Pattern_Directory_Controller has to be fixed to accept arrays instead of using implode to concatenate values.
I will submit a PR shortly.
Please ignore my previous message.
There was a problem hiding this comment.
I've submitted a PR that fixes this issue.
I would appreciate your review, @TimothyBJacobs.
cc: @spacedmonkey
Trac issue: https://core.trac.wordpress.org/ticket/55574
Github PR: #2591
src/wp-includes/rest-api/endpoints/class-wp-rest-block-pattern-categories-controller.php
Show resolved
Hide resolved
| $data = $this->add_additional_fields_to_object( $data, $request ); | ||
| $data = $this->filter_response_by_context( $data, $context ); | ||
|
|
||
| return rest_ensure_response( $data ); |
There was a problem hiding this comment.
This should typically be filterable. It looks like the Pattern Registry does allow for more fields than just these two, so it would be usable by developers.
There was a problem hiding this comment.
Which kind of filtering do you have in mind? I can already register an additional field with register_rest_field( 'block-pattern', ... ).
| _load_remote_block_patterns(); // Patterns with the `core` keyword. | ||
| _load_remote_featured_patterns(); // Patterns in the `featured` category. | ||
| _register_remote_theme_patterns(); // Patterns requested by current theme. |
There was a problem hiding this comment.
Are these methods guarded against running more than once in a request? Looking at _load_remote_block_patterns at least, it seems it will try and register the patterns again. It's very important that the REST API is reentrant.
There was a problem hiding this comment.
They are not guarded. When writing them, I assumed that the handler can ever run only once. But that was wrong. What would be a good way to prevent the _load_remote_* being run twice? Some $already_loaded global variable?
There was a problem hiding this comment.
Excellent, let's wait for confirmation from @TimothyBJacobs on the changes proposed.
There was a problem hiding this comment.
these methods don't get called twice during a simple GET
During regular requests they probably never are, but what @TimothyBJacobs probably had in mind is that the handler can be called multiple times when doing special things: batching requests, executing the endpoints when preloading, embedding linked requests into one response... In these cases, multiple requests are being processed within one parent one.
There was a problem hiding this comment.
Thank you, @jsnajdr.
I understand it.
I was primarily concerned about single requests (fortunately, the issue doesn't affect single requests).
I wouldn't submit a fix if I weren't sure that the issue could affect batch requests etc.
| $context = ! empty( $request['context'] ) ? $request['context'] : 'view'; | ||
| $data = $this->add_additional_fields_to_object( $data, $request ); | ||
| $data = $this->filter_response_by_context( $data, $context ); | ||
| return rest_ensure_response( $data ); |
There was a problem hiding this comment.
Same thing about filter support.
src/wp-includes/rest-api/endpoints/class-wp-rest-block-patterns-controller.php
Show resolved
Hide resolved
src/wp-includes/rest-api/endpoints/class-wp-rest-block-patterns-controller.php
Show resolved
Hide resolved
|
Hi, @TimothyBJacobs and @spacedmonkey -- thank you for your feedback! 🙌🏻 Updates for this last round of comments are being addressed over at #2567 (forked from this PR). |
|
I'm still trying to find out how to go about all those backports to WordPress core. #2567 has failing CI jobs. This branch was passing all CI checks so I consider committing it as is and re-apply all other changes as individual commits directly in WP core. |
|
I'm getting closer to have #2567 pass all CI checks. |
|
I committed all the changes included in #2567 with https://core.trac.wordpress.org/changeset/53152. We still need to address the reamining feedback listed in this PR. |
Backports changes from Gutenberg to add support for custom taxonomies filtering [Block Library - Query Loop].
lib/compat/wordpress-6.0/blocks.phplib/compat/wordpress-6.0/block-patterns.phplib/compat/wordpress-6.0/block-patterns-update.phplib/compat/wordpress-6.0/class-wp-rest-block-pattern-categories-controller.phplib/compat/wordpress-6.0/class-wp-rest-block-patterns-controller.phplib/compat/wordpress-6.0/class-wp-rest-block-patterns-controller.phpphpunit/class-wp-rest-block-pattern-categories-controller-test.phpphpunit/class-wp-rest-block-patterns-controller-test.phplib/compat/wordpress-6.0/class-wp-theme-json-gutenberg.phpWP_Theme_JSON:: get_patterns()Also being tracked in this Gutenberg issue.
Trac ticket: https://core.trac.wordpress.org/ticket/55505
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.