Allow integrations to have custom parameters for queries #26
Conversation
WalkthroughAdds ProductLoader and injects MemoryManager throughout; refactors ProductWalker to a private-constructor factory using injected ProductLoader, MemoryManager, and query args; makes MemoryManager instance-based; introduces per-args option_key handling in async flows; tweaks JsonFileFeed filename logic; removes typed Container property; and adds multiple unit tests and DI test helpers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Comment |
…ate. Use the current time in the file name for uniqueness, but also `wp_hash()` (salt) to make it impossible to guess the last part of the filename.
There was a problem hiding this comment.
LGTM! Thanks for refactoring and enhancing the support for the POS catalog integration.
On the testing side:
- I tried
wp product-feed generate --integration=openai --silent, though opening the file in a JSON viewer app (Dadroit JSON Viewer) said that it's not valid JSON 🤔 - Tested integration in the core branch. Tried passing different
fieldsparameters there, which means different args for the generator, and the status is associated with each parameter 👍
| $hash_data = $this->base_name . gmdate( 'r' ); | ||
| $file_name = $this->base_name . '-' . time() . '-' . wp_hash( $hash_data ) . '.json'; |
There was a problem hiding this comment.
I would prefer this change in another PR, and then have a proper test for it.
There was a problem hiding this comment.
The approach looks good with introducing the interface method get_product_feed_query_args! Please keep finishing it, especially with tests, and move changes related to filename (#26 (review)) to another PR
RadoslavGeorgiev
force-pushed
the
wooptp-41-product-feed-allow-custom-pos-parameters
branch
from
d6ad43b to
5de6e1b
Compare
|
From the POS side, would just like to hear your thoughts on the fields support p1761787711991059/1761032826.289079-slack-C09EWAXSYD9 - feel free to respond here or in the thread for more visibility 🙂 |
I have a concept in my mind, but it's not backed by immediate knowledge of how exactly the core REST API works. I'll try to close this PR today so I can open the next one, showcasing my idea. |
- Memory management is not tested yet. - Add a ProductLoader that can be mocked - Fix a bug in the walker where a round number of procucts (ex. 100 products and batch size 10) would require a useless additional iteration.
|
@htdat I'm marking this one as ready for review. I'm yet to add tests for settings, but I think those will mostly be incosequential. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Core/Plugin.php (1)
31-31: Verify the necessity of removing type safety.The type declaration was removed from the
$containerproperty, reducing type safety. While the enriched summary mentions "more flexible runtime typing in tests," consider whether a better pattern exists (e.g., using an interface or abstract base class that both production and test containers implement) rather than sacrificing type safety.If the test container implements a common interface with the production Container, you could type the property as that interface instead:
private ContainerInterface $container;This would maintain type safety while allowing test container substitution. Can you confirm whether this pattern was considered, or if there's a specific technical constraint requiring the untyped property?
src/Feed/ProductLoader.php (1)
19-31: Consider adding parameter type hint for stricter typing.The wrapper pattern is appropriate for testability, though the
$argsparameter lacks a type hint. Consider declaring it asarray $argsfor better type safety, unless there's a specific reason to accept other types.Apply this diff if you want stricter typing:
- public function get_products( $args ) { + public function get_products( array $args ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/CLI/Command.php(3 hunks)src/Core/Plugin.php(1 hunks)src/Feed/ProductLoader.php(1 hunks)src/Feed/ProductWalker.php(7 hunks)src/Integrations/IntegrationInterface.php(1 hunks)src/Integrations/OpenAi/OpenAiIntegration.php(4 hunks)src/Integrations/OpenAi/ScheduledActionManager.php(1 hunks)src/Integrations/POSCatalog/ApiController.php(1 hunks)src/Integrations/POSCatalog/AsyncGenerator.php(7 hunks)src/Integrations/POSCatalog/POSIntegration.php(2 hunks)src/Storage/JsonFileFeed.php(1 hunks)src/Utils/MemoryManager.php(3 hunks)tests/unit/ProductFeed/Feed/ProductWalkerTest.php(1 hunks)tests/unit/ProductFeed/ProductFeedTestCase.php(1 hunks)tests/unit/ProductFeed/Storage/JsonFileFeedTest.php(1 hunks)tests/unit/ProductFeed/TestContainer.php(1 hunks)tests/unit/ProductFeed/Utils/TestUtilsTest.php(1 hunks)tests/unit/bootstrap.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursorrules)
When suggesting with Tab or writing code, always obey the WordPress Coding Standards (project PHPCS config in phpcs.xml.dist)
Files:
src/Integrations/OpenAi/OpenAiIntegration.phpsrc/Integrations/POSCatalog/ApiController.phpsrc/Feed/ProductLoader.phpsrc/Utils/MemoryManager.phpsrc/Integrations/IntegrationInterface.phptests/unit/ProductFeed/Storage/JsonFileFeedTest.phptests/unit/ProductFeed/TestContainer.phpsrc/Storage/JsonFileFeed.phpsrc/CLI/Command.phpsrc/Core/Plugin.phptests/unit/ProductFeed/ProductFeedTestCase.phptests/unit/ProductFeed/Utils/TestUtilsTest.phpsrc/Integrations/POSCatalog/AsyncGenerator.phpsrc/Integrations/OpenAi/ScheduledActionManager.phptests/unit/bootstrap.phptests/unit/ProductFeed/Feed/ProductWalkerTest.phpsrc/Integrations/POSCatalog/POSIntegration.phpsrc/Feed/ProductWalker.php
🧠 Learnings (4)
📓 Common learnings
Learnt from: RadoslavGeorgiev
PR: woocommerce/OpenAI-Product-Feed#21
File: src/Utils/MemoryManager.php:35-60
Timestamp: 2025-10-24T17:54:03.161Z
Learning: In the woocommerce/OpenAI-Product-Feed repository, the MemoryManager::flush_caches() method uses aggressive cache clearing including direct $wp_object_cache property manipulation. This is an intentional, tried & tested pattern copied from another project that is necessary for generating very large product feeds (70k-200k products). Without this aggressive approach, large exports fail. The cache clearing happens after feed processing is complete, before shipping the file to another server, not during normal page execution.
📚 Learning: 2025-10-22T15:49:57.065Z
Learnt from: RadoslavGeorgiev
PR: woocommerce/OpenAI-Product-Feed#13
File: src/API/Controllers/ApiController.php:44-50
Timestamp: 2025-10-22T15:49:57.065Z
Learning: The ApiController in src/API/Controllers/ApiController.php is planned to be moved and refactored to be OpenAI platform-specific, which is why it currently requires concrete ProductMapper and FeedValidator implementations from the OpenAI platform rather than interface types.
Applied to files:
src/Integrations/OpenAi/OpenAiIntegration.phpsrc/Integrations/OpenAi/ScheduledActionManager.phpsrc/Feed/ProductWalker.php
📚 Learning: 2025-10-24T17:54:03.161Z
Learnt from: RadoslavGeorgiev
PR: woocommerce/OpenAI-Product-Feed#21
File: src/Utils/MemoryManager.php:35-60
Timestamp: 2025-10-24T17:54:03.161Z
Learning: In the woocommerce/OpenAI-Product-Feed repository, the MemoryManager::flush_caches() method uses aggressive cache clearing including direct $wp_object_cache property manipulation. This is an intentional, tried & tested pattern copied from another project that is necessary for generating very large product feeds (70k-200k products). Without this aggressive approach, large exports fail. The cache clearing happens after feed processing is complete, before shipping the file to another server, not during normal page execution.
Applied to files:
src/Feed/ProductLoader.phpsrc/Utils/MemoryManager.phpsrc/CLI/Command.phpsrc/Integrations/POSCatalog/AsyncGenerator.phpsrc/Feed/ProductWalker.php
📚 Learning: 2025-10-22T06:06:47.231Z
Learnt from: RadoslavGeorgiev
PR: woocommerce/OpenAI-Product-Feed#13
File: src/Platforms/OpenAI/ProductMapper.php:99-107
Timestamp: 2025-10-22T06:06:47.231Z
Learning: In the woocommerce/OpenAI-Product-Feed repository, exception messages should use escaping functions (like esc_html()) to sanitize translations before throwing, as translations may contain HTML. Do not suggest removing escaping from exception messages in this codebase.
Applied to files:
src/Integrations/POSCatalog/AsyncGenerator.phpsrc/Feed/ProductWalker.php
🧬 Code graph analysis (13)
src/Integrations/OpenAi/OpenAiIntegration.php (6)
src/Integrations/IntegrationInterface.php (5)
get_id(29-29)get_product_feed_query_args(66-66)create_feed(73-73)get_product_mapper(80-80)get_feed_validator(87-87)src/Integrations/POSCatalog/POSIntegration.php (5)
get_id(44-46)get_product_feed_query_args(51-55)create_feed(92-94)get_product_mapper(99-101)get_feed_validator(106-108)src/Integrations/POSCatalog/ProductMapper.php (2)
get_id(64-66)ProductMapper(22-339)src/Storage/JsonFileFeed.php (1)
JsonFileFeed(24-186)src/Core/DependencyManagement/Container.php (1)
get(53-55)src/Integrations/POSCatalog/FeedValidator.php (1)
FeedValidator(21-32)
src/Integrations/POSCatalog/ApiController.php (1)
src/Integrations/POSCatalog/AsyncGenerator.php (2)
force_regeneration(191-218)get_status(87-136)
src/Integrations/IntegrationInterface.php (3)
src/Core/Plugin.php (2)
activate(99-113)deactivate(118-122)src/Integrations/OpenAi/OpenAiIntegration.php (3)
activate(71-75)deactivate(80-85)get_product_feed_query_args(97-101)src/Integrations/POSCatalog/POSIntegration.php (3)
activate(78-80)deactivate(85-87)get_product_feed_query_args(51-55)
tests/unit/ProductFeed/Storage/JsonFileFeedTest.php (1)
src/Storage/JsonFileFeed.php (6)
JsonFileFeed(24-186)start(82-133)end(156-163)get_file_path(170-172)get_file_url(179-185)add_entry(141-149)
tests/unit/ProductFeed/TestContainer.php (1)
src/Core/DependencyManagement/RuntimeContainer.php (1)
RuntimeContainer(15-207)
src/CLI/Command.php (2)
src/Utils/MemoryManager.php (2)
MemoryManager(19-103)get_available_memory(25-33)src/Feed/ProductWalker.php (2)
ProductWalker(22-265)from_integration(117-156)
tests/unit/ProductFeed/ProductFeedTestCase.php (2)
tests/unit/ProductFeed/TestContainer.php (2)
TestContainer(12-29)reset_all_replacements(26-28)openai-product-feed-for-woo.php (1)
wpfoai_plugin(70-72)
tests/unit/ProductFeed/Utils/TestUtilsTest.php (6)
src/Core/DependencyManagement/ContainerException.php (1)
ContainerException(12-23)src/Integrations/OpenAi/OpenAiIntegration.php (1)
OpenAiIntegration(29-132)tests/unit/ProductFeed/ProductFeedTestCase.php (2)
ProductFeedTestCase(14-103)createMock(49-51)src/Utils/StringHelper.php (2)
StringHelper(19-45)bool_string(27-30)tests/unit/ProductFeed/TestContainer.php (2)
replace_with_concrete(19-21)reset_all_replacements(26-28)openai-product-feed-for-woo.php (1)
wpfoai_get_service(80-82)
src/Integrations/POSCatalog/AsyncGenerator.php (4)
src/Integrations/IntegrationInterface.php (2)
create_feed(73-73)get_id(29-29)src/Feed/ProductWalker.php (2)
ProductWalker(22-265)from_integration(117-156)src/Storage/JsonFileFeed.php (2)
get_file_url(179-185)get_file_path(170-172)src/Feed/FeedInterface.php (2)
get_file_url(48-48)get_file_path(41-41)
src/Integrations/OpenAi/ScheduledActionManager.php (1)
src/Feed/ProductWalker.php (2)
ProductWalker(22-265)from_integration(117-156)
tests/unit/ProductFeed/Feed/ProductWalkerTest.php (6)
tests/unit/ProductFeed/ProductFeedTestCase.php (2)
ProductFeedTestCase(14-103)createMock(49-51)src/Utils/MemoryManager.php (1)
MemoryManager(19-103)src/Feed/ProductLoader.php (1)
ProductLoader(19-32)tests/unit/ProductFeed/TestContainer.php (1)
replace_with_concrete(19-21)src/Feed/WalkerProgress.php (1)
WalkerProgress(15-60)src/Feed/ProductWalker.php (4)
ProductWalker(22-265)from_integration(117-156)set_batch_size(164-167)walk(186-231)
src/Integrations/POSCatalog/POSIntegration.php (5)
src/Integrations/IntegrationInterface.php (6)
get_id(29-29)get_product_feed_query_args(66-66)activate(47-47)deactivate(58-58)create_feed(73-73)get_product_mapper(80-80)src/Integrations/OpenAi/OpenAiIntegration.php (6)
get_id(90-92)get_product_feed_query_args(97-101)activate(71-75)deactivate(80-85)create_feed(106-108)get_product_mapper(113-116)src/Core/Plugin.php (3)
activate(99-113)deactivate(118-122)get(142-144)src/Storage/JsonFileFeed.php (1)
JsonFileFeed(24-186)src/Core/DependencyManagement/Container.php (1)
get(53-55)
src/Feed/ProductWalker.php (6)
src/Feed/ProductLoader.php (2)
ProductLoader(19-32)get_products(29-31)src/Utils/MemoryManager.php (3)
MemoryManager(19-103)get_available_memory(25-33)flush_caches(38-60)src/Integrations/IntegrationInterface.php (3)
get_product_feed_query_args(66-66)get_product_mapper(80-80)get_feed_validator(87-87)src/Integrations/OpenAi/OpenAiIntegration.php (3)
get_product_feed_query_args(97-101)get_product_mapper(113-116)get_feed_validator(121-124)src/Integrations/POSCatalog/POSIntegration.php (3)
get_product_feed_query_args(51-55)get_product_mapper(99-101)get_feed_validator(106-108)openai-product-feed-for-woo.php (1)
wpfoai_get_service(80-82)
🔇 Additional comments (19)
tests/unit/bootstrap.php (1)
106-110: LGTM!The test bootstrap correctly loads ProductFeed test utilities after dependency injection initialization, enabling standardized test helpers across the test suite.
tests/unit/ProductFeed/Storage/JsonFileFeedTest.php (1)
1-126: LGTM!Comprehensive test coverage for JsonFileFeed including file creation, entry handling, error scenarios, and proper test isolation with cleanup helpers.
src/Integrations/POSCatalog/ApiController.php (1)
82-93: LGTM!Initializes parameter support for future extensibility and appropriately removes sensitive fields (action_id, path) from the API response before returning to the client.
src/Integrations/IntegrationInterface.php (1)
60-66: LGTM!The new
get_product_feed_query_args()method appropriately extends the integration contract, enabling each integration to specify custom product query arguments. The@seereference towc_get_products()helpfully clarifies the expected format.src/Storage/JsonFileFeed.php (1)
104-115: LGTM!The new deterministic file naming scheme (base_name + timestamp + hash) is well-documented with a reference to the GitHub discussion. The prior review concern about testing is now addressed with comprehensive test coverage in JsonFileFeedTest.php.
src/CLI/Command.php (2)
35-53: LGTM!Properly applies dependency injection for MemoryManager, enabling instance-based memory management and improving testability.
126-126: LGTM!The ProductWalker factory method
from_integration()centralizes walker initialization and ensures consistent dependency injection across integration contexts.src/Integrations/OpenAi/OpenAiIntegration.php (2)
56-56: LGTM!Adopting
{@inheritdoc}reduces documentation duplication and aligns with the feedback from past reviews, improving maintainability.Also applies to: 69-69, 78-78, 88-88, 103-103, 111-111, 119-119, 127-127
94-101: LGTM!The implementation correctly specifies query arguments for the OpenAI feed (simple and variation product types), fulfilling the IntegrationInterface contract and supporting the centralized ProductWalker initialization flow.
src/Integrations/POSCatalog/POSIntegration.php (1)
48-55: LGTM! POS integration correctly includes variable products.The POS integration appropriately includes
'variable'products in the feed query args, which distinguishes it from the OpenAI integration that only includes'simple'and'variation'types. This aligns with the PR objectives stating that POS requires parent products while OpenAI treats variations as purchasable products.src/Utils/MemoryManager.php (2)
25-33: Conversion to instance method enables dependency injection.The refactor from static to instance method supports the broader architectural shift toward dependency injection, allowing MemoryManager to be injected into ProductWalker and tested with mocks.
38-60: Instance method conversion completed correctly.The flush_caches method and its dependency on collect_garbage have been properly updated to use instance-based calls (
$this->), and collect_garbage visibility has been appropriately reduced to private.tests/unit/ProductFeed/TestContainer.php (1)
12-28: LGTM! Test utility provides clean DI replacement.The TestContainer appropriately extends RuntimeContainer to provide test-specific mock replacement and reset capabilities. The direct manipulation of protected
resolved_cacheis valid within the subclass and enables flexible test setup.src/Integrations/OpenAi/ScheduledActionManager.php (1)
69-71: Factory pattern simplifies walker instantiation.The migration to
ProductWalker::from_integration()successfully centralizes dependency wiring and simplifies the caller code. The integration provides the necessary components (mapper, validator, query args) while the feed remains externally provided as intended.tests/unit/ProductFeed/Utils/TestUtilsTest.php (1)
15-53: Comprehensive test coverage for container replacement.The test effectively validates container replacement functionality including class substitution, interface binding, mock injection, and reset behavior. The test structure is clear and assertions are appropriate.
tests/unit/ProductFeed/Feed/ProductWalkerTest.php (1)
60-246: Excellent comprehensive test coverage for ProductWalker.This test thoroughly validates the refactored ProductWalker including:
- Pagination with various batch sizes and product counts
- Memory management sequencing (availability checks and cache flushes)
- Validation error handling (11th product rejection)
- Filter application (wpfoai_product_feed_args)
- Progress callback reporting
- Feed lifecycle (start/add_entry/end)
The test structure is well-organized despite its complexity, with clear sections for mock setup, expectations, and execution.
tests/unit/ProductFeed/ProductFeedTestCase.php (3)
22-35: Test lifecycle management is well-structured.The setUp and tearDown methods properly initialize and clean up the test container, ensuring test isolation through replacement resets.
53-88: Container replacement logic handles test isolation correctly.The
get_replacement_containermethod appropriately manages the static test container lifecycle, ensuring the plugin uses a testable container while preserving the original resolved cache. The reflection-based approach is necessary for modifying internal plugin state.
90-102: Standard reflection pattern for test access.The
make_property_accessiblehelper follows the standard PHPUnit pattern for accessing private/protected properties during testing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Integrations/POSCatalog/AsyncGenerator.php (1)
238-249: Key ordering normalization is still missing from the past comment's suggestion.While
$args ?? []correctly handles the null case, arrays with identical keys but different order (e.g.,['foo' => 1, 'bar' => 2]vs.['bar' => 2, 'foo' => 1]) will serialize differently and produce distinct hashes. This can lead to duplicate option keys, redundant generation jobs, and orphaned status options/files—the core issue flagged in the earlier review.Apply this diff to add stable key sorting before hashing:
private function get_option_key( ?array $args = null ): string { + $normalized_args = $args ?? []; + + if ( ! empty( $normalized_args ) ) { + ksort( $normalized_args ); + } + return 'feed_status_' . md5( // WPCS dislikes serialize for security reasons, but it will be hashed immediately. // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize serialize( [ 'integration' => $this->integration->get_id(), - 'args' => $args ?? [], + 'args' => $normalized_args, ] ) ); }If
$argscan contain nested arrays, consider a recursive sort or usejson_encode()withJSON_FORCE_OBJECTfor canonical representation.
🧹 Nitpick comments (2)
tests/unit/ProductFeed/ProductFeedTestCase.php (2)
22-29: Consider simplifying the test container initialization.The ternary
isset( $this->test_container ) ? $this->test_container : nullchecks if the property is set before it's assigned. In standard PHPUnit usage,setUp()runs on a fresh instance per test, so$this->test_containerwould not typically be set. The pattern might accommodate subclass scenarios, but it adds cognitive overhead.If subclass flexibility isn't needed, simplify to:
- $this->test_container = $this->get_replacement_container( - isset( $this->test_container ) ? $this->test_container : null - ); + $this->test_container = $this->get_replacement_container();And update the method signature to make the parameter truly optional with a default value.
78-81: Deep reflection chain couples tests to container implementation.The code accesses
plugin->container->container->initial_resolved_cachethrough nested reflection calls. This is fragile and tightly couples the test infrastructure to specific container implementation details.Consider whether the container architecture could expose
initial_resolved_cachethrough a test-friendly accessor method to reduce reflection depth and improve maintainability. If the nested structure is temporary or specific to the current implementation, document why this depth is necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Integrations/POSCatalog/AsyncGenerator.php(7 hunks)tests/unit/ProductFeed/ProductFeedTestCase.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursorrules)
When suggesting with Tab or writing code, always obey the WordPress Coding Standards (project PHPCS config in phpcs.xml.dist)
Files:
tests/unit/ProductFeed/ProductFeedTestCase.phpsrc/Integrations/POSCatalog/AsyncGenerator.php
🧠 Learnings (2)
📚 Learning: 2025-10-24T17:54:03.161Z
Learnt from: RadoslavGeorgiev
PR: woocommerce/OpenAI-Product-Feed#21
File: src/Utils/MemoryManager.php:35-60
Timestamp: 2025-10-24T17:54:03.161Z
Learning: In the woocommerce/OpenAI-Product-Feed repository, the MemoryManager::flush_caches() method uses aggressive cache clearing including direct $wp_object_cache property manipulation. This is an intentional, tried & tested pattern copied from another project that is necessary for generating very large product feeds (70k-200k products). Without this aggressive approach, large exports fail. The cache clearing happens after feed processing is complete, before shipping the file to another server, not during normal page execution.
Applied to files:
src/Integrations/POSCatalog/AsyncGenerator.php
📚 Learning: 2025-10-22T06:06:47.231Z
Learnt from: RadoslavGeorgiev
PR: woocommerce/OpenAI-Product-Feed#13
File: src/Platforms/OpenAI/ProductMapper.php:99-107
Timestamp: 2025-10-22T06:06:47.231Z
Learning: In the woocommerce/OpenAI-Product-Feed repository, exception messages should use escaping functions (like esc_html()) to sanitize translations before throwing, as translations may contain HTML. Do not suggest removing escaping from exception messages in this codebase.
Applied to files:
src/Integrations/POSCatalog/AsyncGenerator.php
🧬 Code graph analysis (2)
tests/unit/ProductFeed/ProductFeedTestCase.php (2)
tests/unit/ProductFeed/TestContainer.php (2)
TestContainer(12-29)reset_all_replacements(26-28)openai-product-feed-for-woo.php (1)
wpfoai_plugin(70-72)
src/Integrations/POSCatalog/AsyncGenerator.php (7)
src/Integrations/IntegrationInterface.php (2)
create_feed(73-73)get_id(29-29)src/Integrations/POSCatalog/POSIntegration.php (2)
create_feed(92-94)get_id(44-46)src/Integrations/OpenAi/OpenAiIntegration.php (2)
create_feed(106-108)get_id(90-92)src/Feed/ProductWalker.php (2)
ProductWalker(22-265)from_integration(117-156)src/Feed/WalkerProgress.php (1)
WalkerProgress(15-60)src/Storage/JsonFileFeed.php (2)
get_file_url(179-185)get_file_path(170-172)src/Feed/FeedInterface.php (2)
get_file_url(48-48)get_file_path(41-41)
🔇 Additional comments (5)
tests/unit/ProductFeed/ProductFeedTestCase.php (3)
37-51: LGTM! Good use of generics for type safety.This override provides better IDE support and static analysis by combining
MockObjectwith the mocked class type. The runtime behavior is unchanged, and the phpcs:ignore is appropriate since the linter cannot recognize the static analysis benefit.
59-88: Verify that static test container sharing across all test classes is safe.The static
$test_containeron line 61 is shared across all tests in all test classes that extendProductFeedTestCase. The comment confirms this is intentional, but it could lead to test pollution if one test class modifies the container in ways that affect subsequent test classes.Confirm that:
- All test classes properly reset replacements in
tearDown()(they do via line 34).- The
initial_resolved_cachecaptured at container creation is sufficient to restore clean state.- Test execution order won't cause cross-contamination between different test suites.
If test isolation issues arise, consider using a static property per test class rather than a globally shared static variable.
90-102: LGTM! Clean reflection helper.Standard pattern for accessing private/protected properties in tests. The implementation is straightforward and well-documented.
src/Integrations/POSCatalog/AsyncGenerator.php (2)
276-304: Solid defensive validation logic.The implementation correctly guards against two edge cases: completed feeds with missing files (manual deletion or external interference) and scheduled jobs stuck beyond 10 minutes (Action Scheduler throttling or failure). Returning
falsetriggers regeneration viaget_status(), which is the appropriate recovery path.
156-156: Good use of factory pattern for walker instantiation.The
ProductWalker::from_integration()factory correctly centralizes initialization logic, allowing each integration to provide custom query args, mappers, and validators. This aligns well with the PR's goal of supporting multi-integration usage with per-integration parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Integrations/POSCatalog/AsyncGenerator.php (2)
87-100: Add type guard before callingvalidate_status().If the option is corrupted (e.g., manually modified to a non-array value),
get_option()can return a value that passes thefalse !== $statuscheck but will trigger aTypeErrorwhen passed tovalidate_status(array $status). For consistency with line 147, add anis_array()guard here.Apply this diff:
- if ( false !== $status && ! $this->validate_status( $status ) ) { + if ( false !== $status && ( ! is_array( $status ) || ! $this->validate_status( $status ) ) ) { $status = false; }
191-198: Add type guard inforce_regeneration()as well.The same type-safety issue exists here: if the option is corrupted to a non-array,
validate_status()will throw aTypeError. Add anis_array()check before callingvalidate_status().Apply this diff:
- if ( false === $status || ! $this->validate_status( $status ) ) { + if ( false === $status || ! is_array( $status ) || ! $this->validate_status( $status ) ) { return $this->get_status( $args ); }
🧹 Nitpick comments (1)
src/Integrations/POSCatalog/AsyncGenerator.php (1)
238-254: Consider recursive normalization for nested arrays.The current normalization uses
ksort(), which only sorts top-level keys. If$argscontains nested arrays, their key order won't be normalized, and semantically identical argument sets could produce different hashes (e.g.,['a' => ['z' => 1, 'y' => 2]]vs.['a' => ['y' => 2, 'z' => 1]]). If nested parameters are expected in the future, consider a recursive sort to ensure canonical representation.Example recursive normalization helper:
private function normalize_args( $args ) { if ( ! is_array( $args ) ) { return $args; } ksort( $args ); foreach ( $args as $key => $value ) { $args[ $key ] = $this->normalize_args( $value ); } return $args; }Then use it in
get_option_key():private function get_option_key( ?array $args = null ): string { - $normalized_args = $args ?? []; - if ( ! empty( $normalized_args ) ) { - ksort( $normalized_args ); - } + $normalized_args = $this->normalize_args( $args ?? [] ); return 'feed_status_' . md5(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Integrations/POSCatalog/AsyncGenerator.php(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursorrules)
When suggesting with Tab or writing code, always obey the WordPress Coding Standards (project PHPCS config in phpcs.xml.dist)
Files:
src/Integrations/POSCatalog/AsyncGenerator.php
🧠 Learnings (2)
📚 Learning: 2025-10-24T17:54:03.161Z
Learnt from: RadoslavGeorgiev
PR: woocommerce/OpenAI-Product-Feed#21
File: src/Utils/MemoryManager.php:35-60
Timestamp: 2025-10-24T17:54:03.161Z
Learning: In the woocommerce/OpenAI-Product-Feed repository, the MemoryManager::flush_caches() method uses aggressive cache clearing including direct $wp_object_cache property manipulation. This is an intentional, tried & tested pattern copied from another project that is necessary for generating very large product feeds (70k-200k products). Without this aggressive approach, large exports fail. The cache clearing happens after feed processing is complete, before shipping the file to another server, not during normal page execution.
Applied to files:
src/Integrations/POSCatalog/AsyncGenerator.php
📚 Learning: 2025-10-22T06:06:47.231Z
Learnt from: RadoslavGeorgiev
PR: woocommerce/OpenAI-Product-Feed#13
File: src/Platforms/OpenAI/ProductMapper.php:99-107
Timestamp: 2025-10-22T06:06:47.231Z
Learning: In the woocommerce/OpenAI-Product-Feed repository, exception messages should use escaping functions (like esc_html()) to sanitize translations before throwing, as translations may contain HTML. Do not suggest removing escaping from exception messages in this codebase.
Applied to files:
src/Integrations/POSCatalog/AsyncGenerator.php
🧬 Code graph analysis (1)
src/Integrations/POSCatalog/AsyncGenerator.php (7)
src/Integrations/IntegrationInterface.php (2)
create_feed(73-73)get_id(29-29)src/Integrations/OpenAi/OpenAiIntegration.php (2)
create_feed(106-108)get_id(90-92)src/Integrations/POSCatalog/POSIntegration.php (2)
create_feed(92-94)get_id(44-46)src/Feed/ProductWalker.php (2)
ProductWalker(22-265)from_integration(117-156)src/Feed/WalkerProgress.php (1)
WalkerProgress(15-60)src/Storage/JsonFileFeed.php (2)
get_file_url(179-185)get_file_path(170-172)src/Feed/FeedInterface.php (2)
get_file_url(48-48)get_file_path(41-41)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/unit/ProductFeed/Integrations/OpenAi/SettingsTest.php (1)
70-75: Semantically incorrect default value.The default value
'https://example.com/'is a URL, butseller_nameshould be a name string. While this doesn't affect the test (sinceget_bloginfo('name')always returns a value), it's semantically confusing.Consider using a more appropriate default:
$this->assertEquals( get_bloginfo( 'name' ), - $this->sut->get( 'seller_name', 'https://example.com/' ) + $this->sut->get( 'seller_name', 'Default Store Name' ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Integrations/OpenAi/Settings.php(2 hunks)tests/unit/ProductFeed/Integrations/OpenAi/SettingsTest.php(1 hunks)tests/unit/bootstrap.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/bootstrap.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursorrules)
When suggesting with Tab or writing code, always obey the WordPress Coding Standards (project PHPCS config in phpcs.xml.dist)
Files:
tests/unit/ProductFeed/Integrations/OpenAi/SettingsTest.phpsrc/Integrations/OpenAi/Settings.php
🧬 Code graph analysis (1)
tests/unit/ProductFeed/Integrations/OpenAi/SettingsTest.php (2)
tests/unit/ProductFeed/ProductFeedTestCase.php (1)
ProductFeedTestCase(14-103)src/Integrations/OpenAi/Settings.php (5)
Settings(19-174)get(39-70)get_endpoint_url(77-90)extend_providers(99-135)save_settings(143-163)
🔇 Additional comments (9)
src/Integrations/OpenAi/Settings.php (4)
46-47: LGTM!The simplified privacy URL retrieval with fallback is clear and correct.
48-51: LGTM!The terms and returns URL retrieval correctly chains WooCommerce functions with a safe fallback.
54-56: LGTM!The shop URL retrieval with fallback is implemented correctly.
144-146: LGTM!The conditional nonce check correctly bypasses verification during unit tests when the
PRODUCT_FEED_UNIT_TESTSconstant is defined and true, while maintaining security checks in production.tests/unit/ProductFeed/Integrations/OpenAi/SettingsTest.php (5)
28-43: LGTM!The test correctly validates both fallback and configured privacy URL scenarios. Using a product as a test fixture is unconventional but works fine for permalink testing.
45-68: LGTM!The test comprehensively validates both
tos_urlandreturns_urlretrieval with proper fallback handling.
77-142: LGTM!These tests provide comprehensive coverage of:
- Registry option retrieval with proper namespace precedence
- Endpoint URL behavior (configured, null, and filtered scenarios)
All logic is correct and assertions are appropriate.
144-169: LGTM!The
extend_providerstest correctly validates that new fields are added while preserving existing fields.
171-192: LGTM!The
save_settingstest correctly validates the settings persistence flow, including proper nonce setup and verification of the updated registry values. The nonce configuration properly supports the conditional check in the Settings class.
Description
This PR centralizes
ProductWalkerinitialization, delegates query argument preparation to integrations, and allows theAsyncGeneratorto work with custom parameters, treating each set of integration + parameters as an individual entity.The changes are being implemented for multiple reasons:
AsyncGenerator's abstraction to work with multiple integrations.Walker initialization
Rather than instantiating the Walker directly in multiple places, that is now done through a centralized static factory method of the Walker. The thinking behind the method is as follows:
FeedInterface) as an external parameter. Even though all feeds areJsonFileFeedcurrently, that might easily change based not only on the provider but also on context (CLI vs REST, etc.)wpfoai_product_feed_argsfilter to include the integration as an argument for integration-specific hooks.Allow individual arguments in the async generator
The generator moves away from the single option that was previously used to a dynamic option key, based on the integration and additional external parameters.
External parameters can be provided by other actors like the POS
ApiController. At this stage, they are always an empty array:OpenAI-Product-Feed/src/Integrations/POSCatalog/ApiController.php
Lines 82 to 85 in f20480d
After some discussions in p1761032826289079-slack-C09EWAXSYD9 tomorrow, this will likely change. But even if it does not, the functionality will be available for when needed.
Tests
I've added tests for most files that were not covered with tests. Below are the exceptions. There are quite a few of them, but they seem... reasonable. Please don't hesitate to disagree with me.
PluginCLI\CommandDeliveryMethods\PushFilecheck_setup()and some failures. I can't properly test curl in unit testsUtils\MemoryManagerIntegrations\POSCatalog\*Integrations\OpenAi\DevHelpersIntegrations\OpenAi\ProductFieldsControllerIntegrations\OpenAi\FeedSchemais_field_required, but it is tested through theFeedValidator.Integrations\OpenAi\OpenAiIntegrationIntegrations\OpenAi\SettingsTesting instructions
This PR is focusing on abstraction/refactoring, bringing nearly no functional changes. The easiest way that covers the most ground would be to generate both an OpenAI feed through CLI and a POS one through the REST API:
The first step is to make sure that both files get generated. Once they are generated, compare both. The POS catalog should contain more entries, as it includes variable products.
Once the REST controller's parameters are defined and implemented, their outcome should be tested as well.
Checklist
npm run test:phpdoes not return errors.npm run lint:phpdoes not return errors.SettingsSummary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests