[Experimental] Add: hierarchical taxonomy ordering for Product Filter Taxonomy block #60076
Conversation
Testing GuidelinesHi @kmanijak , 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:
|
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. |
📝 WalkthroughWalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProductFilterTaxonomy
participant FilterDataProvider
participant FilterData
participant TaxonomyHierarchyData
Client->>ProductFilterTaxonomy: render()
ProductFilterTaxonomy->>TaxonomyHierarchyData: get_hierarchy_map()/get_descendants()/get_parent()
ProductFilterTaxonomy-->>Client: Rendered filter with hierarchical terms
Client->>FilterDataProvider: with(query_clauses)
FilterDataProvider->>FilterData: new FilterData(query_clauses, hierarchy_data)
FilterData->>TaxonomyHierarchyData: get_descendants()/get_parent()
FilterData-->>FilterDataProvider: FilterData instance
Note over CacheController,TaxonomyHierarchyData: On term create/edit/delete
CacheController->>TaxonomyHierarchyData: clear_cache(taxonomy)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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/src/Internal/ProductFilters/TaxonomyHierarchyData.php (1)
218-231: Consider optimizing for deep hierarchies.The recursive implementation works well for typical taxonomies but could be optimized for better performance with deep hierarchies.
Consider an iterative approach with a queue to avoid potential stack overflow and improve performance:
private function compute_descendants( int $term_id, array $children ): array { - $descendants = array(); - - if ( ! isset( $children[ $term_id ] ) ) { - return $descendants; - } - - foreach ( $children[ $term_id ] as $child_id ) { - $descendants[] = $child_id; - $descendants = array_merge( $descendants, $this->compute_descendants( $child_id, $children ) ); - } - - return array_unique( $descendants ); + $descendants = array(); + $queue = array( $term_id ); + $visited = array(); + + while ( ! empty( $queue ) ) { + $current = array_shift( $queue ); + + if ( isset( $visited[ $current ] ) ) { + continue; + } + + $visited[ $current ] = true; + + if ( isset( $children[ $current ] ) ) { + foreach ( $children[ $current ] as $child_id ) { + if ( $current !== $term_id ) { + $descendants[] = $current; + } + $queue[] = $child_id; + } + } + } + + return array_values( array_unique( $descendants ) ); }plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (1)
199-263: Excellent edge case coverage!The tests comprehensively cover important system behaviors:
- Cache functionality with proper invalidation testing
- Circular reference protection prevents infinite loops
- Empty taxonomy handling ensures robustness
- Direct database manipulation tests real-world edge cases
However, there's a minor static analysis issue to address:
public function test_caching(): void { $electronics_id = $this->create_test_term( 'Electronics' ); - $laptops_id = $this->create_test_term( 'Laptops', $electronics_id ); + $this->create_test_term( 'Laptops', $electronics_id );The
$laptops_idvariable is created but never used in the test. Either use it in assertions or remove the assignment.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
plugins/woocommerce/src/Blocks/BlockTypes/ProductFilterTaxonomy.php(3 hunks)plugins/woocommerce/src/Internal/ProductFilters/CacheController.php(3 hunks)plugins/woocommerce/src/Internal/ProductFilters/FilterData.php(5 hunks)plugins/woocommerce/src/Internal/ProductFilters/FilterDataProvider.php(3 hunks)plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/ProductFilters/FilterDataTest.php(4 hunks)plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.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/tests/php/src/Internal/ProductFilters/FilterDataTest.phpplugins/woocommerce/src/Blocks/BlockTypes/ProductFilterTaxonomy.phpplugins/woocommerce/src/Internal/ProductFilters/FilterData.phpplugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.phpplugins/woocommerce/src/Internal/ProductFilters/CacheController.phpplugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.phpplugins/woocommerce/src/Internal/ProductFilters/FilterDataProvider.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/tests/php/src/Internal/ProductFilters/FilterDataTest.phpplugins/woocommerce/src/Blocks/BlockTypes/ProductFilterTaxonomy.phpplugins/woocommerce/src/Internal/ProductFilters/FilterData.phpplugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.phpplugins/woocommerce/src/Internal/ProductFilters/CacheController.phpplugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.phpplugins/woocommerce/src/Internal/ProductFilters/FilterDataProvider.php
🧠 Learnings (8)
📓 Common learnings
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: samueljseay
PR: woocommerce/woocommerce#58716
File: plugins/woocommerce/client/blocks/assets/js/blocks/mini-cart/iapi-frontend.ts:83-101
Timestamp: 2025-06-17T07:07:53.443Z
Learning: In WooCommerce blocks, when porting existing code patterns that have known issues (like parseInt truncating decimal money values), maintain consistency with existing implementation rather than making isolated fixes. The preference is for systematic refactoring approaches (like broader Dinero adoption) over piecemeal changes.
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.
Learnt from: jorgeatorres
PR: woocommerce/woocommerce#59675
File: .github/workflows/release-bump-as-requirement.yml:48-65
Timestamp: 2025-07-15T15:39:21.856Z
Learning: In WooCommerce core repository, changelog entries for all PRs live in `plugins/woocommerce/changelog/` directory and are processed during releases, not at the repository root level.
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: ralucaStan
PR: woocommerce/woocommerce#58782
File: plugins/woocommerce/client/blocks/assets/js/base/utils/render-frontend.tsx:0-0
Timestamp: 2025-06-16T16:12:12.148Z
Learning: For WooCommerce checkout blocks, lazy loading was removed in favor of direct imports to prevent sequential "popping" effects during component loading. This approach prioritizes user experience over code splitting, with minimal bundle size impact and improved performance (1.7s to 1.1s speed score improvement). The checkout flow benefits from having all components load together rather than incrementally.
plugins/woocommerce/tests/php/src/Internal/ProductFilters/FilterDataTest.php (3)
Learnt from: dinhtungdu
PR: #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.
Learnt from: dinhtungdu
PR: #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: gigitux
PR: #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.
plugins/woocommerce/src/Blocks/BlockTypes/ProductFilterTaxonomy.php (4)
Learnt from: dinhtungdu
PR: #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: #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.
Learnt from: dinhtungdu
PR: #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: Aljullu
PR: #58809
File: plugins/woocommerce/src/Blocks/BlockTypes/ProductButton.php:222-225
Timestamp: 2025-06-13T17:11:13.732Z
Learning: In plugins/woocommerce/src/Blocks/BlockTypes/ProductButton.php, the team intentionally relies on toggling the disabled CSS class (via data-wp-class--disabled) instead of binding the disabled attribute, to mirror the behavior of the classic WooCommerce template.
plugins/woocommerce/src/Internal/ProductFilters/FilterData.php (3)
Learnt from: dinhtungdu
PR: #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.
Learnt from: dinhtungdu
PR: #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: mreishus
PR: #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.
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (1)
Learnt from: dinhtungdu
PR: #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.
plugins/woocommerce/src/Internal/ProductFilters/CacheController.php (4)
Learnt from: dinhtungdu
PR: #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: #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.
Learnt from: mreishus
PR: #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: vladolaru
PR: #58784
File: plugins/woocommerce/src/Internal/Admin/Settings/PaymentsProviders/Paytrail.php:29-35
Timestamp: 2025-06-18T09:58:10.616Z
Learning: In WooCommerce codebase, prefer using wc_string_to_bool() over filter_var($value, FILTER_VALIDATE_BOOLEAN) when converting option values (especially 'yes'/'no' style flags) to boolean. The WooCommerce helper function is more idiomatic and handles the conversion consistently with core WooCommerce patterns.
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php (2)
Learnt from: dinhtungdu
PR: #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: #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.
plugins/woocommerce/src/Internal/ProductFilters/FilterDataProvider.php (2)
Learnt from: dinhtungdu
PR: #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.
Learnt from: dinhtungdu
PR: #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.
🧬 Code Graph Analysis (1)
plugins/woocommerce/src/Internal/ProductFilters/CacheController.php (4)
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php (1)
TaxonomyHierarchyData(20-277)plugins/woocommerce/src/Internal/ProductFilters/FilterDataProvider.php (1)
init(44-46)plugins/woocommerce/src/Internal/ProductFilters/QueryClauses.php (1)
init(39-41)plugins/woocommerce/src/Internal/ProductFilters/MainQueryController.php (2)
init(39-42)register(49-52)
🪛 PHPMD (2.15.0)
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php
201-201: Avoid unused local variables such as '$laptops_id'. (Unused Code Rules)
(UnusedLocalVariable)
plugins/woocommerce/src/Internal/ProductFilters/CacheController.php
66-66: Avoid unused parameters such as '$term_id'. (Unused Code Rules)
(UnusedFormalParameter)
66-66: Avoid unused parameters such as '$tt_id'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (30)
plugins/woocommerce/tests/php/src/Internal/ProductFilters/FilterDataTest.php (4)
21-27: LGTM!The property declaration correctly adds the
TaxonomyHierarchyDatainstance with proper PHPDoc documentation.
35-37: LGTM!The initialization correctly retrieves the
TaxonomyHierarchyDatainstance from the container, following the established pattern.
335-335: LGTM!Correctly clears the taxonomy hierarchy cache after setting up test data to ensure fresh hierarchy information is used in assertions.
372-372: LGTM!Correctly clears the taxonomy hierarchy cache after setting up test data, maintaining consistency with the other hierarchical test.
plugins/woocommerce/src/Internal/ProductFilters/CacheController.php (4)
20-26: LGTM!The property is correctly declared with appropriate PHPDoc documentation.
27-36: LGTM!The
initmethod correctly implements dependency injection following the established pattern in product filter internal classes.
45-48: LGTM!Correctly registers hooks for all term lifecycle events to maintain cache consistency.
59-71: LGTM!The method correctly clears the taxonomy hierarchy cache only for hierarchical taxonomies. The unused
$term_idand$tt_idparameters are required by the WordPress action signature.plugins/woocommerce/src/Internal/ProductFilters/FilterDataProvider.php (2)
28-33: LGTM!The property is correctly declared with appropriate PHPDoc documentation.
57-57: LGTM!Correctly passes the
TaxonomyHierarchyDatainstance to theFilterDataconstructor.plugins/woocommerce/src/Blocks/BlockTypes/ProductFilterTaxonomy.php (4)
9-9: LGTM!The import statement correctly adds the
TaxonomyHierarchyDataclass.
151-151: LGTM!Correctly delegates term retrieval to the new
get_hierarchical_termsmethod which handles both hierarchical and non-hierarchical taxonomies.
301-383: Well-implemented hierarchical term ordering!The method correctly handles both hierarchical and non-hierarchical taxonomies, leveraging
TaxonomyHierarchyDatafor efficient parent lookups while preserving WordPress native sibling ordering through a depth-first traversal.
385-416: LGTM!Clean recursive implementation for depth-first traversal that maintains WordPress sibling ordering.
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php (6)
1-32: LGTM!Well-structured class declaration with clear documentation explaining the tiered architecture approach. The cache group constant and in-memory cache property are appropriately defined.
40-97: Excellent caching implementation!The method implements a robust multi-level caching strategy with proper validation, version checking, and debug mode consideration. The 30-day cache expiration is reasonable for taxonomy data.
99-124: LGTM!The threshold of 1000 terms is a reasonable cutoff for strategy selection. Making
get_threshold()protected provides flexibility for future customization.
132-170: Efficient full hierarchy map implementation!Pre-computing all descendants is a good trade-off for small taxonomies, providing O(1) lookup performance for hierarchy queries.
178-209: LGTM!Memory-efficient adjacency list implementation appropriate for large taxonomies, computing descendants on-demand rather than pre-computing.
240-276: Clean and well-designed public API!The methods provide a consistent interface regardless of the underlying storage strategy, with proper fallbacks and targeted cache clearing.
plugins/woocommerce/src/Internal/ProductFilters/FilterData.php (5)
8-8: LGTM!The import statement correctly adds the required
TaxonomyHierarchyDataclass for dependency injection.
26-42: LGTM!The dependency injection pattern is correctly implemented with proper typing, documentation, and assignment. This maintains backwards compatibility while enabling the optimized hierarchy functionality.
339-339: LGTM!Correctly passes the raw taxonomy name to the hierarchical counting method, allowing proper sanitization to occur within the method scope while providing the original name to the hierarchy data service.
375-477: Excellent refactoring with performance optimization!The method has been expertly refactored to use the new
TaxonomyHierarchyDataservice while maintaining security and improving performance:Security & Input Validation:
- Proper sanitization with
esc_sqlandwc_sanitize_taxonomy_name- Safe handling of integer term IDs with
absintPerformance Improvements:
- Replaces multiple individual WordPress function calls with optimized service calls
- Uses batch counting with CASE statements to reduce database queries
- Leverages cached hierarchy data
Code Quality:
- Clean separation of concerns
- Proper error handling for empty results
- Maintains existing functionality contract
486-499: Well-implemented helper method with proper caching!The method correctly implements:
- Security: Uses
$wpdb->prepare()for safe database queries- Performance: Object caching to avoid repeated database lookups
- Data integrity: Safe integer conversion with
absint()- Cache management: Appropriate cache key generation and expiration
This efficiently supports the hierarchical counting logic while maintaining security best practices.
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (5)
13-38: Excellent testing utility class!The
TestableTaxonomyHierarchyDatasubclass provides a clean way to test different hierarchy strategies by overriding the threshold. This follows good testing practices by allowing controlled testing of internal logic without external dependencies.
43-91: Proper test class structure with excellent cleanup!The test class follows WordPress testing best practices:
- Extends
WP_UnitTestCaseappropriately- Implements proper setup/teardown cycle
- Manages test term cleanup to prevent test pollution
- Ensures cache isolation between tests
This creates a reliable testing environment.
100-149: Comprehensive testing of core functionality!The helper method and basic tests are well-implemented:
create_test_term()properly creates and tracks terms for cleanup- Hierarchy map tests thoroughly verify the full map strategy structure
- Assertions validate parents, children, and descendants mappings correctly
- Tests confirm the expected data relationships
154-194: Thorough testing of public API methods!The tests comprehensively verify:
get_parent()correctly handles both existing and non-existent termsget_descendants()returns complete hierarchical relationships- Multi-level hierarchy traversal works correctly
- Edge cases return appropriate default values (0 for parent, empty array for descendants)
268-309: Excellent validation of optimization strategies!The tests thoroughly verify both hierarchy strategies:
- Full map strategy: Confirms pre-computed descendants for performance
- Adjacency strategy: Verifies on-demand computation for memory efficiency
- Strategy selection: Uses testable subclass to control threshold behavior
- Functional equivalence: Both strategies produce correct results
This validates the core performance optimization approach where strategy selection is based on taxonomy size.
plugins/woocommerce/src/Internal/ProductFilters/FilterDataProvider.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php (2)
105-132: Consider documenting the strategy selection criteria more explicitly.The filter documentation is comprehensive, but the actual strategy selection logic only considers term count via the filter without using it directly. Consider adding a comment explaining why the default strategy is always 'full_map' regardless of term count, or implement the original size-based logic mentioned in the PR objectives.
private function get_optimal_strategy( string $taxonomy ): string { $term_count = wp_count_terms( array( 'taxonomy' => $taxonomy ) ); + // Default to full_map strategy for optimal performance with 30-day caching. + // Developers can override via filter for memory-constrained environments. + $default_strategy = 'full_map'; + /** * Filters the hierarchy caching strategy for taxonomies.
226-239: Efficient recursive descendant computation with proper cycle prevention.The recursive algorithm correctly computes descendants and uses
array_unique()to prevent duplicates. However, consider adding cycle detection to prevent infinite recursion in case of corrupted taxonomy data.-private function compute_descendants( int $term_id, array $children ): array { +private function compute_descendants( int $term_id, array $children, array $visited = array() ): array { $descendants = array(); + // Prevent infinite recursion in case of circular references. + if ( in_array( $term_id, $visited, true ) ) { + return $descendants; + } + $visited[] = $term_id; + if ( ! isset( $children[ $term_id ] ) ) { return $descendants; } foreach ( $children[ $term_id ] as $child_id ) { $descendants[] = $child_id; - $descendants = array_merge( $descendants, $this->compute_descendants( $child_id, $children ) ); + $descendants = array_merge( $descendants, $this->compute_descendants( $child_id, $children, $visited ) ); } return array_unique( $descendants ); }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/woocommerce/src/Blocks/BlockTypes/ProductFilterTaxonomy.php(3 hunks)plugins/woocommerce/src/Internal/ProductFilters/CacheController.php(3 hunks)plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/woocommerce/src/Blocks/BlockTypes/ProductFilterTaxonomy.php
🧰 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/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.phpplugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.phpplugins/woocommerce/src/Internal/ProductFilters/CacheController.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/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.phpplugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.phpplugins/woocommerce/src/Internal/ProductFilters/CacheController.php
🧠 Learnings (4)
📓 Common learnings
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: samueljseay
PR: woocommerce/woocommerce#58716
File: plugins/woocommerce/client/blocks/assets/js/blocks/mini-cart/iapi-frontend.ts:83-101
Timestamp: 2025-06-17T07:07:53.443Z
Learning: In WooCommerce blocks, when porting existing code patterns that have known issues (like parseInt truncating decimal money values), maintain consistency with existing implementation rather than making isolated fixes. The preference is for systematic refactoring approaches (like broader Dinero adoption) over piecemeal changes.
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.
Learnt from: jorgeatorres
PR: woocommerce/woocommerce#59675
File: .github/workflows/release-bump-as-requirement.yml:48-65
Timestamp: 2025-07-15T15:39:21.856Z
Learning: In WooCommerce core repository, changelog entries for all PRs live in `plugins/woocommerce/changelog/` directory and are processed during releases, not at the repository root level.
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: ralucaStan
PR: woocommerce/woocommerce#58782
File: plugins/woocommerce/client/blocks/assets/js/base/utils/render-frontend.tsx:0-0
Timestamp: 2025-06-16T16:12:12.148Z
Learning: For WooCommerce checkout blocks, lazy loading was removed in favor of direct imports to prevent sequential "popping" effects during component loading. This approach prioritizes user experience over code splitting, with minimal bundle size impact and improved performance (1.7s to 1.1s speed score improvement). The checkout flow benefits from having all components load together rather than incrementally.
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (3)
Learnt from: dinhtungdu
PR: #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: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/woo-phpunit.mdc:0-0
Timestamp: 2025-07-18T14:55:07.629Z
Learning: Run WooCommerce PHPUnit tests for specific files or directories using the command: pnpm run test:php:env {relative_path} --verbose, and the command must be run in the plugins/woocommerce directory.
Learnt from: dinhtungdu
PR: #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.
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php (2)
Learnt from: dinhtungdu
PR: #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: #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.
plugins/woocommerce/src/Internal/ProductFilters/CacheController.php (4)
Learnt from: dinhtungdu
PR: #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: #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.
Learnt from: mreishus
PR: #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: vladolaru
PR: #58784
File: plugins/woocommerce/src/Internal/Admin/Settings/PaymentsProviders/Paytrail.php:29-35
Timestamp: 2025-06-18T09:58:10.616Z
Learning: In WooCommerce codebase, prefer using wc_string_to_bool() over filter_var($value, FILTER_VALIDATE_BOOLEAN) when converting option values (especially 'yes'/'no' style flags) to boolean. The WooCommerce helper function is more idiomatic and handles the conversion consistently with core WooCommerce patterns.
🪛 PHPMD (2.15.0)
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php
172-172: Avoid unused local variables such as '$laptops_id'. (Unused Code Rules)
(UnusedLocalVariable)
plugins/woocommerce/src/Internal/ProductFilters/CacheController.php
66-66: Avoid unused parameters such as '$term_id'. (Unused Code Rules)
(UnusedFormalParameter)
66-66: Avoid unused parameters such as '$term_taxonomy_id'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ 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)
- GitHub Check: Blocks e2e tests 9/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 8/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 7/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 4/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 10/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 5/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 6/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 2/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Blocks e2e tests 3/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 1/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 3/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 5/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.2] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 4/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.2] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 1/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Lint - @woocommerce/plugin-woocommerce
- GitHub Check: build
🔇 Additional comments (19)
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php (6)
16-28: LGTM: Well-structured class design with proper caching foundation.The class structure follows WooCommerce patterns with appropriate internal marking and type declarations. The cache group constant and in-memory cache array provide a solid foundation for the multi-level caching strategy.
36-93: Excellent caching strategy with proper fallback handling.The multi-level caching approach is well-implemented:
- In-memory cache for immediate subsequent calls
- Transient cache with versioning for persistence across requests
- WP_DEBUG bypass for development environments
- Proper cache invalidation using WC_Cache_Helper versioning
The strategy selection and map building logic handles both performance and memory optimization cases appropriately.
140-178: Robust full hierarchy map implementation with proper error handling.The implementation correctly:
- Handles WP_Error and empty results from get_terms()
- Builds complete parent-child relationships
- Pre-computes all descendants for optimal query performance
- Uses appropriate array structures for efficient lookups
186-217: Clean adjacency map implementation for memory efficiency.The adjacency list approach is well-implemented for memory-constrained scenarios, providing the essential parent-child relationships without pre-computed descendants.
248-270: Well-designed public API with consistent error handling.Both
get_parent()andget_descendants()methods provide clean APIs with proper fallbacks:
- Return sensible defaults (0 for parent, empty array for descendants)
- Handle strategy differences transparently
- Use null coalescing operator for clean code
277-284: Efficient targeted cache clearing implementation.The cache clearing method properly handles both in-memory and transient cache cleanup for the specific taxonomy, avoiding unnecessary cache invalidation of other taxonomies.
plugins/woocommerce/src/Internal/ProductFilters/CacheController.php (4)
7-7: LGTM: Proper import for the new dependency.
20-36: Well-implemented dependency injection pattern.The init method follows WooCommerce's dependency injection pattern correctly with proper type hints and final modifier to prevent override.
44-48: Appropriate hook registration for taxonomy cache invalidation.The action hooks are correctly registered for all term lifecycle events that could affect hierarchy relationships. The priority and parameter count are properly set for WordPress compatibility.
59-71: Effective cache clearing with proper hierarchical taxonomy filtering.The method correctly filters for hierarchical taxonomies before clearing cache, preventing unnecessary operations. The unused parameters flagged by static analysis are required for WordPress action compatibility - they cannot be removed.
Note: The
$term_idand$term_taxonomy_idparameters are required by WordPress action hooks and cannot be removed despite static analysis warnings.plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (9)
12-46: Excellent test setup with proper dependency injection and cleanup.The test class properly uses WooCommerce's container for dependency injection and implements thorough cleanup in both setUp() and tearDown() methods to prevent test pollution.
71-79: Well-designed helper method for test term creation.The helper method properly creates terms, tracks them for cleanup, and includes assertion to ensure term creation success.
84-88: Proper edge case testing for non-hierarchical taxonomies.This test correctly verifies that non-hierarchical taxonomies return empty arrays, which is the expected behavior.
93-120: Comprehensive testing of full map strategy structure.The test thoroughly validates all aspects of the full hierarchy map:
- Correct parent-child relationships
- Pre-computed descendants
- Proper array structure with all required keys
125-136: Thorough testing of parent retrieval with edge cases.The test covers both valid parent relationships and the edge case of non-existent terms, ensuring robust API behavior.
142-165: Comprehensive descendant retrieval testing.The test creates a complex hierarchy and validates descendant counts and relationships at multiple levels, including leaf nodes and non-existent terms.
170-188: Effective caching behavior validation.The test verifies that caching works correctly and that cache clearing forces recomputation while maintaining data integrity.
Note: The unused
$laptops_idvariable flagged by static analysis doesn't affect test correctness - it's created as part of the test hierarchy setup.
193-207: Good edge case testing for empty taxonomies.The test properly handles the scenario of taxonomies with no terms, ensuring the API gracefully handles empty states.
212-232: Excellent strategy switching test with proper filter management.The test correctly uses WordPress filters to switch strategies and validates that the adjacency map structure differs from the full map while maintaining functional compatibility.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (2)
27-62: LGTM! Proper test setup and cleanup with minor typo.The setup and teardown methods follow excellent unit testing practices with proper dependency injection, cache clearing, and test isolation. The cleanup logic ensures no test artifacts remain.
Fix the typo in the comment:
- // Clean up. test terms. + // Clean up test terms.
167-188: Remove unused variable in caching test.The test logic for caching is sound, but there's an unused variable that should be removed.
public function test_caching(): void { $electronics_id = $this->create_test_term( 'Electronics' ); - $laptops_id = $this->create_test_term( 'Laptops', $electronics_id ); + $this->create_test_term( 'Laptops', $electronics_id );
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.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/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.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/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php
🧠 Learnings (2)
📓 Common learnings
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: samueljseay
PR: woocommerce/woocommerce#58716
File: plugins/woocommerce/client/blocks/assets/js/blocks/mini-cart/iapi-frontend.ts:83-101
Timestamp: 2025-06-17T07:07:53.443Z
Learning: In WooCommerce blocks, when porting existing code patterns that have known issues (like parseInt truncating decimal money values), maintain consistency with existing implementation rather than making isolated fixes. The preference is for systematic refactoring approaches (like broader Dinero adoption) over piecemeal changes.
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.
Learnt from: jorgeatorres
PR: woocommerce/woocommerce#59675
File: .github/workflows/release-bump-as-requirement.yml:48-65
Timestamp: 2025-07-15T15:39:21.856Z
Learning: In WooCommerce core repository, changelog entries for all PRs live in `plugins/woocommerce/changelog/` directory and are processed during releases, not at the repository root level.
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: ralucaStan
PR: woocommerce/woocommerce#58782
File: plugins/woocommerce/client/blocks/assets/js/base/utils/render-frontend.tsx:0-0
Timestamp: 2025-06-16T16:12:12.148Z
Learning: For WooCommerce checkout blocks, lazy loading was removed in favor of direct imports to prevent sequential "popping" effects during component loading. This approach prioritizes user experience over code splitting, with minimal bundle size impact and improved performance (1.7s to 1.1s speed score improvement). The checkout flow benefits from having all components load together rather than incrementally.
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (3)
Learnt from: dinhtungdu
PR: #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: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/woo-phpunit.mdc:0-0
Timestamp: 2025-07-18T14:55:07.629Z
Learning: Run WooCommerce PHPUnit tests for specific files or directories using the command: pnpm run test:php:env {relative_path} --verbose, and the command must be run in the plugins/woocommerce directory.
Learnt from: dinhtungdu
PR: #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.
🪛 PHPMD (2.15.0)
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php
172-172: Avoid unused local variables such as '$laptops_id'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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)
- GitHub Check: Blocks e2e tests 7/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 10/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 9/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 4/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 8/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Blocks e2e tests 2/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 3/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 1/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 1/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 5/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 4/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 2/6 - @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.2] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.2] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Lint - @woocommerce/plugin-woocommerce
- GitHub Check: build
🔇 Additional comments (8)
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (8)
1-26: LGTM! Well-structured test class setup.The file header, namespace, and class declaration follow PHP and WordPress coding standards with proper strict typing, documentation, and appropriate inheritance from
WP_UnitTestCase.
64-79: LGTM! Well-designed helper method.The
create_test_termhelper method properly creates test terms with error handling, tracks them for cleanup, and follows DRY principles. The assertion provides clear error messages for debugging.
81-88: LGTM! Comprehensive test for non-hierarchical taxonomies.The test correctly verifies that non-hierarchical taxonomies return empty arrays, which is the expected behavior.
90-120: LGTM! Thorough test of full map strategy.Excellent test coverage for the full map strategy including:
- Creation of multi-level hierarchy
- Verification of parents, children, and descendants mappings
- Proper assertions for pre-computed descendants
122-136: LGTM! Comprehensive parent retrieval testing.The test covers all hierarchy levels and includes proper edge case testing for non-existent terms.
139-165: LGTM! Thorough descendants testing with complex hierarchy.Excellent test that creates a multi-branch hierarchy and validates descendant retrieval at different levels with proper count assertions and edge cases.
190-207: LGTM! Proper empty taxonomy edge case testing.The test correctly handles the edge case of empty taxonomies and properly cleans up by unregistering the test taxonomy.
209-232: LGTM! Well-implemented adjacency strategy test.Excellent test that uses filters to force the adjacency strategy and properly verifies:
- Adjacency maps don't have pre-computed descendants
- On-demand computation still works correctly
- Proper filter cleanup
Implements tiered architecture approach for taxonomy hierarchy data: - Small taxonomies (<1000 terms): Full hierarchy map with pre-computed descendants - Medium taxonomies (1000-10000 terms): Adjacency list with on-demand computation - Large taxonomies (10000+ terms): Chunked lazy loading approach Features: - Auto-scaling strategy selection based on term count - Separate cache group for hierarchy data (independent of filter data) - WP_DEBUG cache bypass for development - Performance optimizations to eliminate get_ancestors/get_term_children calls - Proper cache versioning and invalidation Part of issue #59960 hierarchical term ordering implementation.
Adds lazy loading functionality for large taxonomies (10000+ terms): - get_children_chunk(): Load direct children for specific parent with caching - get_children_with_meta(): Load children with full term data for UI rendering - get_descendants_chunked(): Recursively load all descendants with chunked approach Implementation features: - Per-parent caching to avoid repeated queries - Progressive loading - only fetch terms when needed - UI-ready metadata including depth, menu_order, term names - Replaces expensive get_term_children() calls with optimized chunked loading - Recursive descendant loading with caching to prevent N+1 queries This completes the tiered architecture for handling taxonomies of any size.
Provides consistent interface that abstracts strategy differences: - get_parent(): Get parent term ID for any term - get_children(): Get direct children for any term - get_descendants(): Get all descendants for any term - get_depth(): Get hierarchy depth level for any term - get_terms_by_depth(): Get terms organized by depth level - get_term_meta(): Get term metadata (depth, menu_order) Benefits: - Single interface works across all strategies (full_map, adjacency_map, chunked_lazy) - FilterData and frontend components use same API regardless of taxonomy size - Implementation details completely abstracted away - Consistent method signatures and return formats - Performance optimized - uses fastest available method per strategy - Future-proof - internal optimizations won't break consuming code This eliminates the need for consumers to handle different data structures based on taxonomy size, providing a clean and maintainable interface.
Removes chunked strategy for extreme cases (10,000+ categories) to focus on real-world usage: **Simplified Architecture:** - Small taxonomies (<1000 terms): Full hierarchy map with pre-computed descendants - Large taxonomies (1000+ terms): Adjacency list with on-demand computation **Changes:** - Remove MEDIUM_TAXONOMY_THRESHOLD constant (10,000 limit) - Remove chunked_lazy strategy entirely - Remove 6 chunked-specific methods (get_children_chunk, get_children_with_meta, etc.) - Simplify strategy selection to binary choice - Update documentation to reflect two-strategy approach **Benefits:** - Covers 99.9% of real-world stores (most have <5000 categories) - Reduces codebase complexity by ~200 lines - Easier to maintain, test, and debug - Better performance optimization for common cases - Simplified unified API with same clean interface **Developer Escape Hatch:** For extreme cases, developers can extend the class or override get_optimal_strategy() method to implement custom strategies as needed. This pragmatic approach balances performance, maintainability, and real-world practicality.
Streamlines the caching architecture by centralizing all cache operations:
**Unified Caching:**
- Single cache key per taxonomy: 'wc_hierarchy_{taxonomy}'
- Stores only the hierarchy map (no strategy caching needed)
- Eliminates separate full_map/adjacency_map cache keys
**Optimized Cache Flow:**
- Removed unnecessary strategy caching (only used during build)
- Inlined get_cache/set_cache logic directly in get_hierarchy_map()
- Eliminated redundant method abstractions used in only one place
**Performance Improvements:**
- Two-layer caching: in-memory + transient with proper versioning
- WP_DEBUG cache bypass for development
- Simplified unified API methods use single get_hierarchy_map() call
- Automatic map type detection from data structure
**Code Simplification:**
- Removed ~80 lines of unnecessary abstraction
- Eliminated duplicate cache logic in strategy methods
- Single entry point for all caching operations
- Cleaner separation between caching and building logic
This follows YAGNI principles by removing abstractions that weren't actually needed
while maintaining all performance benefits and cache reliability.
- Change from DAY_IN_SECONDS to MONTH_IN_SECONDS - Taxonomy hierarchy data is very stable and only changes when terms are modified - Proper cache invalidation via clear_cache() handles updates - Reduces cache rebuilding frequency for better performance
…dency - Add TaxonomyHierarchyData to FilterDataProvider constructor with nullable parameter - Update with() method to pass hierarchy_data to FilterData instances - Ensures proper dependency injection for hierarchical taxonomy optimization - FilterData instances now consistently receive TaxonomyHierarchyData
…oller - Add TaxonomyHierarchyData dependency injection to CacheController using init() pattern - Register WordPress term hooks (created_term, edited_term, delete_term) for automatic cache clearing - Implement selective cache invalidation for hierarchical taxonomies only - Use consistent taxonomy_hierarchy_data variable naming throughout - Ensures taxonomy hierarchy cache stays synchronized with WordPress term changes
Replace complex WordPress-order-dependent sorting with simplified approach: - Get terms without WordPress ordering, apply our own sorting consistently - Use context-aware counts for accurate count-based sorting (not database counts) - Sort siblings at each hierarchy level before building depth-first structure - Single unified sorting method handles both name and count criteria - Eliminates conditional sorting logic for cleaner, more maintainable code
…rchyData Replace complex threshold logic with simple, developer-friendly approach: - Default to 'full_map' strategy (optimal with 30-day caching) - Add 'woocommerce_taxonomy_hierarchy_strategy' filter for customization - Update documentation to reflect new approach - Maintain backward compatibility with existing threshold method Benefits: - Optimal default performance for 95% of use cases - Full developer control for edge cases via filter - Simpler, more maintainable codebase - Extensible for future strategy additions The filter allows developers to customize strategy per taxonomy, environment, or based on dynamic conditions like memory constraints or term count.
ab2ac7d to
3902790
Compare
There was a problem hiding this comment.
I left some minor questions or suggestions but overall good job!
I must admit it took me while to go through it, not the easiest PR to review! 😅
Tested:
- hierarchical order by number of products and by name
- invalidating cache by adding new category or changing the category parent
The only disturbing aspect is that the order in editor is "absolute" with no hierarchy. Is that something you plan to address separately because it's pretty misleading.
| $map['descendants'][ $term_id ] = $this->compute_descendants( $term_id, $map['children'] ); | ||
| } | ||
|
|
||
| return $map; |
There was a problem hiding this comment.
If the difference between build_full_hierarchy_map and build_adjacency_hierarchy_map is only
// Pre-compute all descendants for each term.
foreach ( array_keys( $map['parents'] ) as $term_id ) {
$map['descendants'][ $term_id ] = $this->compute_descendants( $term_id, $map['children'] );
}
what about extracting the whole common body between these two to share? There's lot of duplication now. Or am I missing more differences?
There was a problem hiding this comment.
After consideration, I think with a 30-day cache, we only need the full map. So I simplified the logic in 5f409cd
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php
Outdated
Show resolved
Hide resolved
| $args = array( | ||
| 'taxonomy' => $taxonomy, | ||
| 'hide_empty' => false, | ||
| 'fields' => 'all', |
There was a problem hiding this comment.
Do we need all of the fields? On large sites it may cause performance issues when used extensively (p55Cj4-4sD-p2).
Applies to other places where we call get_terms.
There was a problem hiding this comment.
Yes, not all the fields, but we need ID, title, and slug to build the filter options list.
In the PR description, I also mentioned an idea to reduce the get_terms calls. Does it sound good to you? Do you think we should do it within this PR?
I believe we could further improve performance by storing additional term data—such as name, slug, and menu order—directly in the hierarchical taxonomy data. This would allow us to eliminate get_terms calls entirely, resulting in even faster retrieval and processing of taxonomy terms. However, I think we should do it in a follow-up PR as this one is already big.
There was a problem hiding this comment.
In the PR description, I also mentioned an idea to reduce the get_terms calls. Does it sound good to you? Do you think we should do it within this PR?
Comment makes sense and totally ok to do it in separate PR!
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php (1)
128-141: Consider optimizing with memoization for better performance.The recursive implementation is correct but could benefit from caching computed descendants during the same computation cycle to avoid recomputing the same subtrees.
Consider adding in-memory caching during computation:
private function compute_descendants( int $term_id, array $children ): array { + static $memo = array(); + + if ( isset( $memo[ $term_id ] ) ) { + return $memo[ $term_id ]; + } + $descendants = array(); if ( ! isset( $children[ $term_id ] ) ) { + $memo[ $term_id ] = $descendants; return $descendants; } foreach ( $children[ $term_id ] as $child_id ) { $descendants[] = $child_id; $descendants = array_merge( $descendants, $this->compute_descendants( $child_id, $children ) ); } - return array_unique( $descendants ); + $result = array_unique( $descendants ); + $memo[ $term_id ] = $result; + return $result; }
🧹 Nitpick comments (1)
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (1)
170-188: Remove unused variable as flagged by static analysis.The test properly validates caching behavior, but the
$laptops_idvariable is unused and should be removed.public function test_caching(): void { $electronics_id = $this->create_test_term( 'Electronics' ); - $laptops_id = $this->create_test_term( 'Laptops', $electronics_id ); + $this->create_test_term( 'Laptops', $electronics_id ); // First call should build and cache the map. $map1 = $this->sut->get_hierarchy_map( $this->taxonomy );
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.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/src/Internal/ProductFilters/TaxonomyHierarchyData.phpplugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.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/src/Internal/ProductFilters/TaxonomyHierarchyData.phpplugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php
🧠 Learnings (3)
📓 Common learnings
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#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.
Learnt from: samueljseay
PR: woocommerce/woocommerce#58716
File: plugins/woocommerce/client/blocks/assets/js/blocks/mini-cart/iapi-frontend.ts:83-101
Timestamp: 2025-06-17T07:07:53.443Z
Learning: In WooCommerce blocks, when porting existing code patterns that have known issues (like parseInt truncating decimal money values), maintain consistency with existing implementation rather than making isolated fixes. The preference is for systematic refactoring approaches (like broader Dinero adoption) over piecemeal changes.
Learnt from: jorgeatorres
PR: woocommerce/woocommerce#59675
File: .github/workflows/release-bump-as-requirement.yml:48-65
Timestamp: 2025-07-15T15:39:21.856Z
Learning: In WooCommerce core repository, changelog entries for all PRs live in `plugins/woocommerce/changelog/` directory and are processed during releases, not at the repository root level.
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: ralucaStan
PR: woocommerce/woocommerce#58782
File: plugins/woocommerce/client/blocks/assets/js/base/utils/render-frontend.tsx:0-0
Timestamp: 2025-06-16T16:12:12.148Z
Learning: For WooCommerce checkout blocks, lazy loading was removed in favor of direct imports to prevent sequential "popping" effects during component loading. This approach prioritizes user experience over code splitting, with minimal bundle size impact and improved performance (1.7s to 1.1s speed score improvement). The checkout flow benefits from having all components load together rather than incrementally.
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php (2)
Learnt from: dinhtungdu
PR: #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: #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.
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (3)
Learnt from: dinhtungdu
PR: #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: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/woo-phpunit.mdc:0-0
Timestamp: 2025-07-18T14:55:07.629Z
Learning: Run WooCommerce PHPUnit tests for specific files or directories using the command: pnpm run test:php:env {relative_path} --verbose, and the command must be run in the plugins/woocommerce directory.
Learnt from: dinhtungdu
PR: #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.
🪛 PHPMD (2.15.0)
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php
172-172: Avoid unused local variables such as '$laptops_id'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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)
- GitHub Check: Blocks e2e tests 9/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 7/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 10/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 6/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 5/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 2/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 1/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Metrics - @woocommerce/plugin-woocommerce [performance]
- GitHub Check: Core e2e tests 5/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Blocks e2e tests 3/10 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 6/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 4/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 2/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 1/6 - @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.2] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 7.4 WP: latest - 1 [WP 6.7.2] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 1/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: PHP: 8.4 WP: latest [WP latest] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Lint - @woocommerce/plugin-woocommerce
- GitHub Check: build
🔇 Additional comments (12)
plugins/woocommerce/src/Internal/ProductFilters/TaxonomyHierarchyData.php (6)
15-27: LGTM! Proper class structure and security measures.The class follows WooCommerce conventions with proper ABSPATH guard, internal annotation, and well-defined properties.
35-70: LGTM! Well-implemented caching strategy.The method implements a solid multi-layer caching approach with proper hierarchical taxonomy validation and debug-aware caching behavior.
83-118: LGTM! Efficient hierarchy building algorithm.The method properly handles WordPress get_terms() errors, builds relationships efficiently, and pre-computes descendants for optimal performance in product filtering scenarios.
150-153: LGTM! Clean and consistent implementation.The method correctly uses the hierarchy map and handles non-existent terms consistently with WordPress conventions.
162-165: LGTM! Clean and consistent implementation.The method correctly uses the hierarchy map and handles non-existent terms appropriately by returning an empty array.
172-179: LGTM! Proper cache clearing implementation.The method correctly clears both in-memory and persistent caches with consistent cache key construction.
plugins/woocommerce/tests/php/src/Internal/ProductFilters/TaxonomyHierarchyDataTest.php (6)
12-79: LGTM! Well-structured test class with proper setup and cleanup.The test class follows WooCommerce testing patterns with proper dependency injection, cache management, and test data cleanup.
84-88: LGTM! Good edge case testing for non-hierarchical taxonomies.The test properly validates that non-hierarchical taxonomies return empty arrays as expected.
93-120: LGTM! Comprehensive test of core hierarchy functionality.The test creates a realistic multi-level hierarchy and thoroughly validates all relationship mappings including pre-computed descendants.
125-136: LGTM! Thorough testing of parent retrieval including edge cases.The test validates correct parent relationships and properly handles non-existent terms by returning 0.
142-165: LGTM! Comprehensive testing of descendant retrieval.The test creates a complex multi-branch hierarchy and thoroughly validates descendant relationships at all levels, including proper edge case handling.
193-207: LGTM! Excellent edge case testing for empty taxonomies.The test properly validates behavior with empty hierarchical taxonomies and includes proper test taxonomy cleanup.
@kmanijak It may be a sign of overengineering. I simplified the codebase. Can you take another look?
Yeah, I mentioned this in the PR description. I'm currently finding the best way to do it. |
There was a problem hiding this comment.
@kmanijak It may be a sign of overengineering. I simplified the codebase. Can you take another look?
Thanks for simplifying it! TBH It looks much better now and I'm glad you took another round about it!
The only disturbing aspect is that the order in editor is "absolute" with no hierarchy. Is that something you plan to address separately because it's pretty misleading.
Yeah, I mentioned this in the PR description. I'm currently finding the best way to do it.
Ah, sorry, I missed that, sounds good!
I re-tested the PR and changes look good to me! Let's ship!
| $args = array( | ||
| 'taxonomy' => $taxonomy, | ||
| 'hide_empty' => false, | ||
| 'fields' => 'all', |
There was a problem hiding this comment.
In the PR description, I also mentioned an idea to reduce the get_terms calls. Does it sound good to you? Do you think we should do it within this PR?
Comment makes sense and totally ok to do it in separate PR!
Submission Review Guidelines
Changes proposed in this Pull Request
This PR introduces a new TaxonomyHierarchyData class that provides hierarchy mapping for hierarchical taxonomies in product filters.
Key improvements:
get_ancestors()andget_term_children()calls with bulk hierarchy operationsCurrent Implementation
This PR implements basic hierarchical sorting by returning a flat list where terms appear in proper hierarchical depth-first order. Future enhancements will add hierarchy support to filter options, including visual indentation and parent-child relationship properties in the filter UI. See #60069.
Why This Change
The current implementation performs individual WordPress function calls for each term's hierarchy relationships, leading to performance issues with large taxonomies. This optimization reduces database queries while providing correct hierarchical ordering as a foundation for enhanced hierarchy features.
Note:
The preview of the Product Filter Taxonomy block in the editor is not hierarchically sorted for now. Hierarchical ordering is applied only on the frontend display.
Future Optimization:
I believe we could further improve performance by storing additional term data—such as name, slug, and menu order—directly in the hierarchical taxonomy data. This would allow us to eliminate
get_termscalls entirely, resulting in even faster retrieval and processing of taxonomy terms. However, I think we should do it in a follow-up PR as this one is already big.Closes WOOPLUG-59960
Screenshots or screen recordings
How to test the changes in this Pull Request
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
Set up test environment with hierarchical taxonomy data:
Test Product Filter Taxonomy block ordering:
Test caching functionality:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Optimize hierarchical taxonomy ordering for Product Filter Taxonomy block with new TaxonomyHierarchyData class