This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 217
Product Query: Fix pagination issue #7109
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we shouldn't.
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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reading this code, I wonder… is there any way we can make these lines conditional?
https://github.com/woocommerce/woocommerce-blocks/pull/7109/files#diff-efb947e345af5e501dfaa2980b055b3b31c308b31e8db23049b6b48fecee33d8L31-L36
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 likepre_render_{$this->block_id}_block
. You know what I mean? 🤔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.
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.
Yeah, I searched during my exploration, but unfortunately doesn't exist.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense!
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?
cc @sunyatasattva
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 filterquery_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.