Add type check for product to prevent fatals #60277
Conversation
Testing GuidelinesHi @prettyboymp , Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
📝 WalkthroughWalkthroughA type check was added to the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant WC_Comments
participant wc_get_product
participant WC_Product
Caller->>WC_Comments: clear_transients(post_id)
WC_Comments->>WC_Comments: sanitize and validate post_id
alt post_id invalid or not 'product' post type
WC_Comments-->>Caller: return early
else valid product post_id
WC_Comments->>wc_get_product: wc_get_product(post_id)
wc_get_product-->>WC_Comments: product_obj
alt product_obj is instance of WC_Product
WC_Comments->>WC_Product: update rating counts, average rating, review count
WC_Comments->>WC_Product: save()
else product_obj not WC_Product
WC_Comments-->>Caller: do nothing
end
end
WC_Comments-->>Caller: return
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/woocommerce/tests/php/includes/class-wc-comments-test.php (2)
121-127: Prefer @doesNotPerformAssertions over runtime expectNotToPerformAssertions()This test’s intent is “no crash.” The annotation communicates that better and avoids runtime state.
/** * Test clear_transients with invalid product ID. + * @doesNotPerformAssertions */ public function test_clear_transients_with_invalid_product_id() { $invalid_product_id = 99999; // This should not cause any errors or exceptions. - $this->expectNotToPerformAssertions(); WC_Comments::clear_transients( $invalid_product_id ); }
132-149: Same here: use @doesNotPerformAssertions and remove the runtime callKeeps consistency and intent clearer across tests that only verify “no fatal.”
/** * Test clear_transients with non-product post ID. + * @doesNotPerformAssertions */ public function test_clear_transients_with_non_product_post() { @@ // This should not cause any errors or exceptions. - $this->expectNotToPerformAssertions(); WC_Comments::clear_transients( $post_id ); // Clean up. wp_delete_post( $post_id, true ); }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/woocommerce/changelog/60277-fix-add-type-check-for-product-to-prevent-fatals(1 hunks)plugins/woocommerce/includes/class-wc-comments.php(1 hunks)plugins/woocommerce/tests/php/includes/class-wc-comments-test.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{php,js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/code-quality.mdc)
**/*.{php,js,jsx,ts,tsx}: Guard against unexpected inputs
Sanitize and validate any potentially dangerous inputs
Ensure code is backwards compatible
Write code that is readable and intuitive
Ensure code has unit or E2E tests where applicable
Files:
plugins/woocommerce/includes/class-wc-comments.phpplugins/woocommerce/tests/php/includes/class-wc-comments-test.php
**/*.{php,js,ts,jsx,tsx}
⚙️ CodeRabbit Configuration File
**/*.{php,js,ts,jsx,tsx}: Don't trust that extension developers will follow the best practices, make sure the code:
- Guards against unexpected inputs.
- Sanitizes and validates any potentially dangerous inputs.
- Is backwards compatible.
- Is readable and intuitive.
- Has unit or E2E tests where applicable.
Files:
plugins/woocommerce/includes/class-wc-comments.phpplugins/woocommerce/tests/php/includes/class-wc-comments-test.php
🧠 Learnings (7)
📓 Common learnings
Learnt from: mreishus
PR: woocommerce/woocommerce#58817
File: plugins/woocommerce/includes/wc-product-functions.php:140-140
Timestamp: 2025-06-13T23:52:46.221Z
Learning: In WooCommerce's wc_delete_product_transients function, the context check includes both $is_admin_page (for regular admin screens) and $is_privileged_ajax (for AJAX requests from users with edit_products capability), ensuring that legitimate admin AJAX operations like product imports/exports can still delete transients.
Learnt from: dinhtungdu
PR: woocommerce/woocommerce#59499
File: plugins/woocommerce/src/Internal/ProductFilters/QueryClauses.php:327-332
Timestamp: 2025-07-10T04:22:27.648Z
Learning: In the WooCommerce ProductFilters QueryClauses class, the $chosen_taxonomies parameter in add_taxonomy_clauses() already contains only validated public product taxonomies. The validation occurs upstream in the Params class during parameter registration, so additional taxonomy existence validation in the processing methods is redundant.
Learnt from: dinhtungdu
PR: woocommerce/woocommerce#59900
File: plugins/woocommerce/client/blocks/assets/js/blocks/product-filters/inner-blocks/attribute-filter/inspector.tsx:0-0
Timestamp: 2025-07-24T05:37:00.907Z
Learning: The DisplayStyleSwitcher component in plugins/woocommerce/client/blocks/assets/js/blocks/product-filters/components/display-style-switcher/index.tsx has been updated so that its onChange prop accepts only a string type (not string | number | undefined), eliminating the need for type assertions when using this component.
Learnt from: dinhtungdu
PR: woocommerce/woocommerce#59878
File: plugins/woocommerce/src/Internal/ProductFilters/FilterData.php:364-383
Timestamp: 2025-07-23T01:06:46.155Z
Learning: In WooCommerce ProductFilters FilterData class, the $product_ids parameter in methods like get_hierarchical_taxonomy_counts() is internally generated from WP_Query results using implode(',', array_column($results, 'ID')). This creates a safe comma-separated string of integers from database IDs, not user input, so SQL injection concerns don't apply to this parameter.
📚 Learning: 2025-06-13T23:52:46.221Z
Learnt from: mreishus
PR: woocommerce/woocommerce#58817
File: plugins/woocommerce/includes/wc-product-functions.php:140-140
Timestamp: 2025-06-13T23:52:46.221Z
Learning: In WooCommerce's wc_delete_product_transients function, the context check includes both $is_admin_page (for regular admin screens) and $is_privileged_ajax (for AJAX requests from users with edit_products capability), ensuring that legitimate admin AJAX operations like product imports/exports can still delete transients.
Applied to files:
plugins/woocommerce/changelog/60277-fix-add-type-check-for-product-to-prevent-fatalsplugins/woocommerce/includes/class-wc-comments.phpplugins/woocommerce/tests/php/includes/class-wc-comments-test.php
📚 Learning: 2025-07-24T05:37:00.907Z
Learnt from: dinhtungdu
PR: woocommerce/woocommerce#59900
File: plugins/woocommerce/client/blocks/assets/js/blocks/product-filters/inner-blocks/attribute-filter/inspector.tsx:0-0
Timestamp: 2025-07-24T05:37:00.907Z
Learning: The DisplayStyleSwitcher component in plugins/woocommerce/client/blocks/assets/js/blocks/product-filters/components/display-style-switcher/index.tsx has been updated so that its onChange prop accepts only a string type (not string | number | undefined), eliminating the need for type assertions when using this component.
Applied to files:
plugins/woocommerce/changelog/60277-fix-add-type-check-for-product-to-prevent-fatals
📚 Learning: 2025-06-20T17:38:16.565Z
Learnt from: opr
PR: woocommerce/woocommerce#0
File: :0-0
Timestamp: 2025-06-20T17:38:16.565Z
Learning: WooCommerce legacy JavaScript files in plugins/woocommerce/client/legacy/js/ must use older JavaScript syntax and cannot use modern features like optional chaining (?.) due to browser compatibility requirements. Explicit null checking with && operators should be used instead.
Applied to files:
plugins/woocommerce/changelog/60277-fix-add-type-check-for-product-to-prevent-fatals
📚 Learning: 2025-07-10T04:22:27.648Z
Learnt from: dinhtungdu
PR: woocommerce/woocommerce#59499
File: plugins/woocommerce/src/Internal/ProductFilters/QueryClauses.php:327-332
Timestamp: 2025-07-10T04:22:27.648Z
Learning: In the WooCommerce ProductFilters QueryClauses class, the $chosen_taxonomies parameter in add_taxonomy_clauses() already contains only validated public product taxonomies. The validation occurs upstream in the Params class during parameter registration, so additional taxonomy existence validation in the processing methods is redundant.
Applied to files:
plugins/woocommerce/changelog/60277-fix-add-type-check-for-product-to-prevent-fatals
📚 Learning: 2025-07-23T01:06:46.155Z
Learnt from: dinhtungdu
PR: woocommerce/woocommerce#59878
File: plugins/woocommerce/src/Internal/ProductFilters/FilterData.php:364-383
Timestamp: 2025-07-23T01:06:46.155Z
Learning: In WooCommerce ProductFilters FilterData class, the $product_ids parameter in methods like get_hierarchical_taxonomy_counts() is internally generated from WP_Query results using implode(',', array_column($results, 'ID')). This creates a safe comma-separated string of integers from database IDs, not user input, so SQL injection concerns don't apply to this parameter.
Applied to files:
plugins/woocommerce/includes/class-wc-comments.php
📚 Learning: 2025-06-16T09:20:22.981Z
Learnt from: gigitux
PR: woocommerce/woocommerce#58846
File: plugins/woocommerce/client/blocks/tests/e2e/tests/all-products/all-products.block_theme.spec.ts:41-52
Timestamp: 2025-06-16T09:20:22.981Z
Learning: In WooCommerce E2E tests, the database is reset to the initial state for each test, so there's no need to manually restore global template changes (like clearing the header template) as the test infrastructure handles cleanup automatically.
Applied to files:
plugins/woocommerce/tests/php/includes/class-wc-comments-test.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: build
- GitHub Check: Blocks e2e tests 2/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 10/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 5/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 7/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 8/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 4/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 9/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 6/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 5/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 3/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Core e2e tests 4/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: Blocks e2e tests 1/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 3/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.3] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 1/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 2/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.3] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
🔇 Additional comments (2)
plugins/woocommerce/includes/class-wc-comments.php (1)
262-268: Solid guard against non-product inputs; prevents fatalsFetching via wc_get_product() and checking instanceof WC_Product is the right fix here. It gracefully no-ops for invalid/non-product IDs and keeps behavior intact for real products.
plugins/woocommerce/changelog/60277-fix-add-type-check-for-product-to-prevent-fatals (1)
1-4: Changelog entry reads well and matches the code changeScope and rationale are clear; classification as a patch/fix is appropriate.
plugins/woocommerce/tests/php/includes/class-wc-comments-test.php
Outdated
Show resolved
Hide resolved
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
| if ( 'product' === get_post_type( $post_id ) ) { | ||
| $product = wc_get_product( $post_id ); | ||
| $product = wc_get_product( $post_id ); | ||
| if ( $product instanceof WC_Product ) { |
There was a problem hiding this comment.
I wonder about an unintended consequence of this change. By skipping the initial get_post_type() check, you open up the possibility of false being passed to wc_get_product(). This eventually gets to WC_Product_Factory::get_product_id() which will try to use the global $post as the product. This may not be the desired effect.
Conceptually, I like the simplicity of the approach, but I do worry about this potential side-effect.
There was a problem hiding this comment.
Actually what I added has the same problem because get_post_type calls get_post which also reads from global if $post_id is falsey. I have a better implementation coming.
There was a problem hiding this comment.
I realized that the use of instanceof is another problem. PHP <7.3 fatals if a constant (such as false or null) is on the left side of the operator. WordPress still supports 7.2. I see that WooCommerce requires 7.4+. So, I'm not sure if this is a valid concern or not.
There was a problem hiding this comment.
Really? I've never encountered that. Also, tests on earlier php versions don't seem to be a problem.
There was a problem hiding this comment.
Interesting. I only thought of it because I ran into such a situation not long ago. I guess I'm forgetting a specific detail of that situation. Either way, I guess it's not important since Woo requires 7.4 or above.
|
Test failures seem legit, investigating. |
Check if product is WC_Product instance before calling methods to prevent fatal errors when product might not exist or be invalid.
Add comprehensive test coverage for WC_Comments::clear_transients method including tests for valid products, invalid product IDs, non-product posts, and zero product IDs to ensure the type checking works correctly.
Replace assertNull with expectNotToPerformAssertions for better error testing. This is the modern PHPUnit approach for testing that code doesn't throw errors or exceptions, and is already used throughout the WooCommerce codebase.
…duct that might exist in global
b295db5 to
06a9b07
Compare
Co-authored-by: Michael Pretty <prettyboymp@users.noreply.github.com>
| if ( $post_id === 0 || 'product' !== get_post_type( $post_id ) ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
hmm, although get_post_type will attempt to get a post from the $post global if it is falsey. So I wonder if the initial check here should be a similar falsey check?
There was a problem hiding this comment.
nvm the absint is here 🤦🏻
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR adds a type check for the product object in the
clear_transientsmethod to prevent fatal errors when the product might not exist or be invalid.The change modifies
plugins/woocommerce/includes/class-wc-comments.phpto:wc_get_product( $post_id )WC_Productinstance before proceeding with the operationswc_get_product()returnsfalseor an invalid objectThe fix was implemented because we were seeing a fatal in some error logs in a hosting environment where this showed up in the stack trace. I suspect the particular environment was executing the call to
wc_get_productto early in the request which resulted in a fatal error:How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Test with valid product ID:
clear_transientsmethod works correctly and updates the product's rating dataTest with invalid product ID:
clear_transientsmethod with a non-existent product IDTest with non-product post ID:
clear_transientsmethod with a post ID that exists but is not a product (e.g., a page or post)Testing that has already taken place:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Add type check for product to prevent fatals in WC_Comments::clear_transients method.
Changelog Entry Comment
Comment