Fix query results of ProductQuery block by removing the default stock status query attribute #7748
Conversation
…k status query attribute
rubikuserbot
requested review from
a team and
nefeline
and removed request for
a team
dinhtungdu
added
status: needs review
type: bug
dinhtungdu
requested review from
gigitux and
sunyatasattva
and removed request for
nefeline
|
The release ZIP for this PR is accessible via: |
|
Size Change: +1 B (0%) Total Size: 971 kB
ℹ️ View Unchanged
|
TypeScript Errors ReportFiles with errors: 428 🎉 🎉 This PR does not introduce new TS errors. |
|
Thank you for the investigation, @dinhtungdu! Indeed, the more we look, the more interesting cases we discover. However, I have to say, I disagree with this approach. I think it is a bit unclear to the user. Moreover, the products shown in the shortcode and the Product Query block are not the same if you have the “Hide out of stock” products setting enabled:
On top of that, we should consider that stock statuses are customizeable by the user. I don't know which query is wrong and what should be the correct way of displaying products. But I think we should figure that out and fix the query, not patch the editor UI with something potentially confusing. Is a fix PHP-side possible? Why does the existance of the meta query impact the results? |
Is it unclear that
Can you explain this point in more details? I'm not sure I fully get it. This PR doesn't affect the ability to customize the stock statuses, users can still customize it when they need it.
Great catch! I missed testing the out of stock setting. Investigating... |
My initial investigation: when ➡ Because of this, I think we should investigate if there is any way to parse queries using WC functions. Database screenshot
|
In my opinion, it is a bit different. The Sale filter is a boolean to show only products on sale when enabled. It is disabled by default, and, as such, the expected results are displayed in the preview. With the Stock Status, it is not exactly all statuses pre-selected, as it also depends by the global “Hide out of stock from catalog” option. If that's selected, then the stock status input will reflect that and remind the users that they decided to globally hide all the out of stock products. So they will manually need to add that status to the input to make sure those out of stock products are shown. What do you think?
Sorry, I should have been more clear. This was in reference to the description of the PR:
As stock statuses are customizable, the input is set to display all of them by default. I didn't mean that it affects in some way the ability to customize, just that we need to consider other unpredictable use cases for when they have different custom stock statuses enabled.
How much less performant are I agree that we should try and use as many WC functions as we can. |
It makes more sense to me now. But from the performance perspective, I still have a concern. Adding stock status by default means that we always use a meta query, even when merchants just want to display all statuses, comparing to
My immediate answer is, I don't know. I actually didn't think about performance when I said we should use WC functions. The reason to use the WC functions is to achieve the same queries as WC use, which produces consistent results and reduces maintenance effort. |
|
I think it could be helpful to do a performance comparison with a large set of Products, and then take an informed decision on whether this is truly a problem or not. I think it would also be very educational in general, as I have these PHP Lint recommendations on not using What do you think? If you feel I'm being too obnoxious, I'm also ok just merging this as it is. But I think it'd be helpful in the long run to use this as an opportunity to understand and make better decisions in the future. |
I agree 💯 . Marking this PR as |
dinhtungdu
added
the
status: on hold
|
Just FYI, from a design standpoint, I talked to @vivialice and asked her to take a look into what could be the best for users. As the for the performance impact, the point above still stands and I'm happy to explore that! |
|
@sunyatasattva FYI, this is the PR implemented Moreover, in woocommerce/woocommerce#12558, WC added rating terms to the At this point, I don't think it's worth doing performance comparisons between tax and meta queries, as we're likely to end up with the same conclusion. And I think we should figure out how to make our queries align with WC core's ones. |
|
Close in favors of #7951 |
This PR fixes the query results issue of the Product Query block which is caused by the default stock status query attribute.
Currently, we set the stock status attribute of the Product Query block to all available statuses:
instock,outofstock, andonbackorder. This cause two issues:instock,outofstock, andonbackorder) affects the query results as described in Product Query: query results are different from shortcodes/PHP query loops #7746. While I agree that the query result shouldn't be different as we're querying for all stock statuses, we should make the Product Query block return the same query results as the corresponding PHP query loop.Fixes #7746
Accessibility
prefers-reduced-motionOther Checks
Screenshots
Testing
Automated Tests
User Facing Testing
[recent_products]shortcode block.WooCommerce Visibility
Performance Impact
The performance of the default Product Query block should be improved because it doesn't use meta_query for the default state.