This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Fix: Product Query editor preview with Stock Status setting #7682
Conversation
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
…tor preview can show the correct products corresponding to the enabled stock status
rubikuserbot
requested review from
a team and
albarin
and removed request for
a team
dinhtungdu
added
status: needs review
type: bug
|
The release ZIP for this PR is accessible via: |
TypeScript Errors ReportFiles with errors: 431 🎉 🎉 This PR does not introduce new TS errors. |
|
Size Change: 0 B Total Size: 972 kB ℹ️ View Unchanged
|
src/BlockTypes/ProductQuery.php
Outdated
Comment on lines
99
to
102
| return $this->merge_queries( | ||
| array_merge( $args, $on_sale_query ), | ||
| $stock_query | ||
| ); |
There was a problem hiding this comment.
What is the advantage of this over array_merge( $args, $on_sale_query, $stock_query )? We need to consider that we'll add more and more queries, already in #7687 I add another one which will conflict here, how should we do this in the future?
E.g.
return $this->merge_queries(
array_merge( $args, $on_sale_query, $best_selling_query ),
$stock_query
);
How do we decide what's $b in the merge_queries call?
My suggestion here is to actually refactor the merge_queries call to accept multiple arguments, like most PHP array functions do. So the signature would be merge_queries(array ...$queries). I had suggested it here when this function was implemented, but we didn't go ahead with it because the advantages were not clear.
Now that we meet this problem, I think the advantages of having that is clearer, because we'll keep adding queries and we should just be able to do:
return $this->merge_queries(
$args, $on_sale_query, $best_selling_query, $stock_query
);
and don't worry about the internal implementation of that. What do you think?
There was a problem hiding this comment.
What is the advantage of this over
array_merge( $args, $on_sale_query, $stock_query )?
@sunyatasattva With the current implementation of the class and structure of WP_Query, we have to use merge_queries for tax and meta query as tax_query and meta_query are arrays containing arrays.
How do we decide what's
$bin themerge_queriescall?
As I mentioned above, queries containing tax_query and/or meta_query are $b.
What do you think?
I strongly agree. merge_queries should be the source of trust in merging queries. I can refactor it either in this PR or a follow-up PR. Tagging @gigitux here for more perspectives :)
There was a problem hiding this comment.
I created #7697, so we have more things on the table to discuss.
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
dinhtungdu
removed
the
status: stale
7 tasks
Script Dependencies ReportThe
This comment was automatically generated by the |
|
@sunyatasattva I updated this PR to use the latest |
7 tasks
|
@sunyatasattva I updated this PR base on our discussion in #7743. |
7 tasks
sunyatasattva
approved these changes
Dec 1, 2022
src/BlockTypes/ProductQuery.php
Outdated
Comment on lines
325
to
327
| if ( empty( $stock_statii ) ) { | ||
| return array(); | ||
| } |
There was a problem hiding this comment.
Alright, I see what you mean. We need to decide what is the approach we take at least within this class. I don't have a super strong opinion. Given the conversation in #7796, what do you think of the middle-ground I proposed there?
There was a problem hiding this comment.
Just commented there, I will update this PR following what we do with yours.
sunyatasattva
added
the
status: blocker
3 tasks
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR updates the REST query to include the stock status meta query, so the editor preview can show the correct products corresponding to the enabled stock status.
This PR also update the
merge_queiresmethod to ensure properties are arrays before merging.Follow up #7397
Screenshots
Screen.Recording.2022-11-15.at.11.02.24.mov
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility
Performance Impact
Changelog