Product Query: Fix pagination issue #7109
Conversation
gigitux
added
type: bug
rubikuserbot
requested review from
a team and
Aljullu
and removed request for
a team
|
The release ZIP for this PR is accessible via: |
|
Size Change: 0 B Total Size: 874 kB ℹ️ View Unchanged
|
gigitux
force-pushed
the
fix/product-query-update
branch
from
1fb8941
to
16ea028
Compare
gigitux
added
the
skip-changelog
| if ( isset( $parsed_block['attrs']['__woocommerceVariationProps'] ) ) { | ||
| add_filter( | ||
| 'query_loop_block_query_vars', | ||
| array( $this, 'get_query_by_attributes' ), |
There was a problem hiding this comment.
Should we make this method inline? This way we don't need to set $this->parsed_block anymore.
There was a problem hiding this comment.
No, we shouldn't.
This way we don't need to set $this->parsed_block anymore.
It is still necessary this. If we don't use this anymore, the $parsed_block will always be an instance of our variations (for the check at line 54). This is a problem because, for example, if on the same page, we have:
- Products Query block.
- Query Loop block with post type set to
Post(so it should render the post on frontend).
The filter injects in the Query Loop block the query used by the Products Query block. Obviously, this is incorrect.
Let me know what you think, and if it makes sense to you, I will merge this PR!
There was a problem hiding this comment.
Re-reading this code, I wonder… is there any way we can make these lines conditional?
Like, is there any other filter we can use so that we don't run this on every block? It would be amazing if there ware a filter like rest_{$this->post_type}_query, but with something like pre_render_{$this->block_id}_block. You know what I mean? 🤔
There was a problem hiding this comment.
Re-reading this code, I wonder… is there any way we can make these lines conditional?
GitHub doesn't highlight the lines. From the URL, it seems that you are referring to these:
If yes, we already checked during one of our pair sessions: these lines are executed once: when the class has been instanced.
Like, is there any other filter we can use so that we don't run this on every block? It would be amazing if there ware a filter like rest_{$this->post_type}query, but with something like pre_render{$this->block_id}_block. You know what I mean? 🤔
Yeah, I searched during my exploration, but unfortunately doesn't exist.
There was a problem hiding this comment.
Oooh, right, thanks for the explanation. Another reason not to use an inline function is to avoid the same filter being added several times.
There was a problem hiding this comment.
I might have been confused. But with this PR, the filter isn't added for each block instance, that's because we are calling a class method so every time add_filter() is called, it knows it's the same method. But if the function was inline, it would be added as a filter every time it's called. (I might be wrong, though)
There was a problem hiding this comment.
If yes, we already checked during one of our pair sessions: these lines are executed once: when the class has been instanced.
@gigitux Sorry, I wasn't clear enough. I know these lines get executed once in the sense that the filter is registered only once. But the filter is run on the pre-render of every block. Of course we return early and no harm is done. But I still would have loved if it wasn't like that…
I hope this makes more sense.
There was a problem hiding this comment.
I hope this makes more sense.
It makes sense!
Of course we return early and no harm is done. But I still would have loved if it wasn't like that…
Agree, but it seems that it isn't possible :(
I might have been confused. But with this PR, the filter isn't added for each block instance, that's because we are calling a class method so every time add_filter() is called, it knows it's the same method. But if the function was inline, it would be added as a filter every time it's called. (I might be wrong, though)
The filter query_loop_block_query_vars is added every time that the Product Query variation is rendered.
The reason is that pre_render_block is triggered on the pre-render of every block.
Does it make sense?
There was a problem hiding this comment.
The filter
query_loop_block_query_varsis added every time that the Product Query variation is rendered.
The reason is thatpre_render_blockis triggered on the pre-render of every block.
I think the confusion here comes from what we understand by "the filter is added". It's true that add_filter() is called every time a Product Query variation is rendered. But given that we add a named function (in this case, a class method) filter, once it's added once, it isn't added again every time. Ie: if there are ten Product Query blocks in a page, get_query_by_attributes() won't be called 10 times per block, only 1. But if the filter added an inline function instead of a named function, get_query_by_attributes() would be called 10 times for every block. At least, that's how I understand it works, but I might be wrong.
There was a problem hiding this comment.
It makes sense! I did a test, and I can confirm what you wrote!
Just to be clear, in our case the function is called 4 times for each block. The reason is that the function gutenberg_build_query_vars_from_query_block (where there is the filter query_loop_block_query_vars) in the hierarchy of the Query Block is called 4 times:
This means that if on the page there are two Product Query blocks, the function get_query_by_attributes() is called 8 times.
| if ( isset( $parsed_block['attrs']['__woocommerceVariationProps'] ) ) { | ||
| add_filter( | ||
| 'query_loop_block_query_vars', | ||
| array( $this, 'get_query_by_attributes' ), |
There was a problem hiding this comment.
Oooh, right, thanks for the explanation. Another reason not to use an inline function is to avoid the same filter being added several times.
|
I'm merging this PR. Happy to continue the discussion above if you have other thoughts. Thanks for the review! 🙇 |
@sunyatasattva and I worked together on this PR.
During some tests, I noticed that the pagination didn't work for custom queries (for example
on sale productsfilter). The reason is that blocks related to the pagination usinggutenberg_build_query_vars_from_query_blockwhere it is applied the filterquery_loop_block_query_vars. So, removing the filter isn't the right approach because, in the case of custom query, the blocks related to the pagination have to continue to use the query injected by the filter.This PR is a follow-up of #6952.
Testing
User Facing Testing
WooCommerce Visibility
Changelog
N/A