Product Query: make sure request params are set when enhancing REST query #7796
Conversation
|
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
|
|
@sunyatasattva I can reproduce the issue, it behaves differently depend on PHP version. On PHP 7.4, it throws a warning, and the products still show up in the editor preview. On PHP 8.0, it throws a fatal error, and the editor preview is broken: So I think we should fix this issue by updating |
|
Excellent! [Insert ”Why not both?” gif here.] I will update the PR to include both the approaches, as the method can be called independently but also as part of the REST API filter. |
If we return an empty array from |
There was a problem hiding this comment.
It works now! Thanks @sunyatasattva, for creating this PR 🙇
So I think we should fix this issue by updating get_product_attributes_query to ensure it always returns an array.
@dinhtungdu, I would prefer that we add the check ONLY outside of this function. Following the PHPDoc, the function accepts an array, so if it receives another type, it is an expected behavior having the error.
Dreaming PHP with a strong type system
src/BlockTypes/ProductQuery.php
Outdated
| $woo_attributes = $request->get_param( '__woocommerceAttributes' ); | ||
| $on_sale_query = $request->get_param( '__woocommerceOnSale' ) === 'true' ? $this->get_on_sale_products_query() : array(); | ||
| $orderby_query = isset( $orderby ) ? $this->get_custom_orderby_query( $orderby ) : array(); | ||
| $tax_query = isset( $woo_attributes ) ? $this->get_product_attributes_query( $woo_attributes ) : array(); |
There was a problem hiding this comment.
Why is the variable called tax_query and not attributes_query? 🤔
There was a problem hiding this comment.
I mean, just because the return value of get_product_attributes_query returns a tax_query, so I thought it would be a bit more explicit on what's going on when glancing at the code.
There was a problem hiding this comment.
If in the future, we have more than one query that returns tax_query, then we will need to update the naming here.
If we follow this way, then I should update (again 😄 ) #7682 to use $meta_query for stock status.
There was a problem hiding this comment.
Good point, it makes sense to rename it attributes_query.
sunyatasattva
added
skip-changelog
|
Hey folks, thanks for the review! Sorry @dinhtungdu for yesterday, I was quite tired as it was late for me, so I wasn't very lucid. I still am not sure how to exactly reproduce, so please feel free to correct the testing steps above. Otherwise, I'm converting the PR to Ready for Review and giving you a little time to see if you agree with @gigitux's statement here. I don't have a strong opinion: I see @gigitux's point on it being expected to fail, but I also see no harm in making it innocuous when it does (maybe the harm is that what we swallow the real problem and the exception). |
There was a problem hiding this comment.
@dinhtungdu, I would prefer that we add the check ONLY outside of this function. Following the PHPDoc, the function accepts an array, so if it receives another type, it is an expected behavior having the error.
The reason I suggested doing so is: if we check the argument, the logic of update_rest_query is easier to read because the flow isn't interrupted by shorthand conditions. I share @sunyatasattva point that I also see no harm in making it innocuous.
But I see your point and I agree, as being explicit may help us more in the long run. Just want to make sure we follow this convention at least inside the ProductQuery class (I will need to update my PRs too).
I still am not sure how to exactly reproduce, so please feel free to correct the testing steps above.
The testing instruction works for me, but I think we should mention the PHP version in the testing instruction too. We should also make sure for testers to enable debug and debug log to see the PHP warnings for PHP 7.4.
src/BlockTypes/ProductQuery.php
Outdated
| $woo_attributes = $request->get_param( '__woocommerceAttributes' ); | ||
| $on_sale_query = $request->get_param( '__woocommerceOnSale' ) === 'true' ? $this->get_on_sale_products_query() : array(); | ||
| $orderby_query = isset( $orderby ) ? $this->get_custom_orderby_query( $orderby ) : array(); | ||
| $tax_query = isset( $woo_attributes ) ? $this->get_product_attributes_query( $woo_attributes ) : array(); |
There was a problem hiding this comment.
If in the future, we have more than one query that returns tax_query, then we will need to update the naming here.
If we follow this way, then I should update (again 😄 ) #7682 to use $meta_query for stock status.
Co-authored-by: Tung Du <[email protected]>
What if I update the |
Sounds like a win-win to me, so I agree 💯 . |
|
@gigitux are you ok with this? I'm updating the PR but can always revert in case you don't like it. |
It's super fine for me! Thanks for updating the PR 🙇 |
Also rename attributes query variable name.
|
Cool! I've also renamed the variable following your suggestion! |
This PR is a follow-up to #7743, which in some cases would introduce errors (which could be fatal under PHP 8.0) on the editor side when the REST API request values would be unexpectedly empty.
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility