Added Platform Registry Class, Shopify Platform Base, a new List command #59778
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a PlatformRegistry service to the WooCommerce CLI Migrator, transitioning from hardcoded platform data to a proper dependency injection pattern.
- Adds
PlatformRegistryclass with platform discovery via WordPress filters and dynamic instantiation of fetcher/mapper classes - Updates
BaseCommandto inject bothCredentialManagerandPlatformRegistrydependencies - Refactors
ListCommandto use real platform registry data instead of hardcoded array
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| PlatformRegistry.php | New service class providing platform discovery, validation, and factory methods for fetcher/mapper instances |
| BaseCommand.php | Extended init method to accept PlatformRegistry dependency injection |
| ListCommand.php | Replaced hardcoded platform data with dynamic registry lookup and formatting |
Comments suppressed due to low confidence (1)
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php:34
- [nitpick] The variable name 'formatted_items' could be more descriptive. Consider renaming to 'platform_items' or 'table_rows' to better indicate its purpose.
$formatted_items = array();
plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php
Outdated
Show resolved
Hide resolved
Testing GuidelinesHi @xristos3490 , 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. |
|
Hey @xristos3490, thanks for testing, I've updated the instructions. The hook function was not correct, please test now. |
ae3205c to
dd3725d
Compare
📝 WalkthroughWalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WP_CLI
participant Command
participant PlatformRegistry
participant CredentialManager
User->>WP_CLI: Run migrate command (list/setup/reset/products)
WP_CLI->>Command: __invoke(args, assoc_args)
Command->>PlatformRegistry: resolve_platform(assoc_args)
PlatformRegistry-->>Command: platform_slug
alt Credential setup required
Command->>PlatformRegistry: get_platform_credential_fields(platform_slug)
PlatformRegistry-->>Command: required_fields
Command->>CredentialManager: setup_credentials(platform_slug, required_fields)
CredentialManager-->>Command: credentials saved
end
Command->>WP_CLI: Output results or messages
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. ✨ 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: 3
♻️ Duplicate comments (1)
plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php (1)
49-49: Filter name inconsistency with PR documentation.The filter name 'wc_migrator_register_platform' doesn't match the 'woocommerce_migrator_platforms' mentioned in the PR testing instructions. This inconsistency could cause confusion for developers trying to register platforms.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugins/woocommerce/changelog/59778-feat-platform-registry(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Commands/BaseCommand.php(2 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Runner.php(0 hunks)
📓 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/CLI/Migrator/Commands/BaseCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.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/CLI/Migrator/Commands/BaseCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php
🧠 Learnings (3)
📓 Common learnings
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: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/generate-pr-description.mdc:0-0
Timestamp: 2025-07-21T05:22:46.417Z
Learning: Provide clear, step-by-step instructions for how to test the changes in the PR description.
plugins/woocommerce/changelog/59778-feat-platform-registry (2)
Learnt from: jorgeatorres
PR: #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: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/woo-build.mdc:0-0
Timestamp: 2025-07-18T14:55:02.778Z
Learning: To watch for changes during development, use 'pnpm --filter=@woocommerce/plugin-woocommerce watch:build' to ensure experimental features are active.
plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php (3)
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.
Learnt from: vladolaru
PR: #58784
File: plugins/woocommerce/src/Internal/Admin/Settings/Payments.php:431-439
Timestamp: 2025-06-17T14:19:30.933Z
Learning: In plugins/woocommerce/src/Internal/Admin/Settings/Payments.php, the process_payment_provider_states() method intentionally filters out payment providers that don't have a _suggestion_id. This is by design to only track state changes for gateways from partner extensions, not core WooCommerce gateways or other installed gateways.
Learnt from: samueljseay
PR: #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.
💤 Files with no reviewable changes (1)
- plugins/woocommerce/src/Internal/CLI/Migrator/Runner.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/src/Internal/CLI/Migrator/Commands/BaseCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.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/CLI/Migrator/Commands/BaseCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php
🧠 Learnings (3)
📓 Common learnings
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: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/generate-pr-description.mdc:0-0
Timestamp: 2025-07-21T05:22:46.417Z
Learning: Provide clear, step-by-step instructions for how to test the changes in the PR description.
plugins/woocommerce/changelog/59778-feat-platform-registry (2)
Learnt from: jorgeatorres
PR: #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: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/woo-build.mdc:0-0
Timestamp: 2025-07-18T14:55:02.778Z
Learning: To watch for changes during development, use 'pnpm --filter=@woocommerce/plugin-woocommerce watch:build' to ensure experimental features are active.
plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php (3)
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.
Learnt from: vladolaru
PR: #58784
File: plugins/woocommerce/src/Internal/Admin/Settings/Payments.php:431-439
Timestamp: 2025-06-17T14:19:30.933Z
Learning: In plugins/woocommerce/src/Internal/Admin/Settings/Payments.php, the process_payment_provider_states() method intentionally filters out payment providers that don't have a _suggestion_id. This is by design to only track state changes for gateways from partner extensions, not core WooCommerce gateways or other installed gateways.
Learnt from: samueljseay
PR: #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.
🔇 Additional comments (2)
plugins/woocommerce/changelog/59778-feat-platform-registry (1)
1-4: Changelog entry follows the correct format and location.The changelog entry correctly uses the WooCommerce changelog directory structure and format.
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/BaseCommand.php (1)
8-8: Dependency injection implementation looks good.The PlatformRegistry integration follows the existing pattern for CredentialManager and maintains proper type safety with the final init() method.
Also applies to: 23-28, 38-40
| $formatted_items = array(); | ||
| foreach ( $platforms as $id => $details ) { | ||
| $formatted_items[] = array( | ||
| 'id' => $id, | ||
| 'name' => $details['name'] ?? '', | ||
| 'fetcher' => $details['fetcher'] ?? '', | ||
| 'mapper' => $details['mapper'] ?? '', | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider validating required platform fields before display.
The current implementation uses null coalescing to provide empty strings for missing fields, which could hide improperly registered platforms. Consider validating that all required fields (name, fetcher, mapper) are present and skip platforms with missing data or show a warning.
$formatted_items = array();
foreach ( $platforms as $id => $details ) {
+ // Skip platforms missing required fields
+ if ( empty( $details['name'] ) || empty( $details['fetcher'] ) || empty( $details['mapper'] ) ) {
+ WP_CLI::warning( sprintf( 'Platform "%s" is missing required fields and will be skipped.', $id ) );
+ continue;
+ }
$formatted_items[] = array(
'id' => $id,
- 'name' => $details['name'] ?? '',
- 'fetcher' => $details['fetcher'] ?? '',
- 'mapper' => $details['mapper'] ?? '',
+ 'name' => $details['name'],
+ 'fetcher' => $details['fetcher'],
+ 'mapper' => $details['mapper'],
);
}🤖 Prompt for AI Agents
In plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php around
lines 34 to 42, the code currently uses null coalescing to default missing
platform fields to empty strings, which can mask improperly registered
platforms. Update the loop to validate that each platform has all required
fields (name, fetcher, mapper) present and non-empty before adding to the
formatted_items array. If any required field is missing, either skip that
platform or log a warning message to notify about the incomplete registration.
| foreach ( $platforms as $platform_id => $config ) { | ||
| if ( isset( $config['fetcher'], $config['mapper'] ) ) { | ||
| $this->platforms[ $platform_id ] = $config; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen platform configuration validation.
The current validation only checks for fetcher and mapper existence but doesn't validate the configuration structure or required fields.
foreach ( $platforms as $platform_id => $config ) {
+ // Validate platform configuration structure
+ if ( ! is_array( $config ) ) {
+ continue;
+ }
+
+ // Ensure all required fields are present
+ if ( ! isset( $config['name'], $config['fetcher'], $config['mapper'] ) ) {
+ continue;
+ }
+
+ // Validate field types
+ if ( ! is_string( $config['name'] ) || ! is_string( $config['fetcher'] ) || ! is_string( $config['mapper'] ) ) {
+ continue;
+ }
+
- if ( isset( $config['fetcher'], $config['mapper'] ) ) {
$this->platforms[ $platform_id ] = $config;
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| foreach ( $platforms as $platform_id => $config ) { | |
| if ( isset( $config['fetcher'], $config['mapper'] ) ) { | |
| $this->platforms[ $platform_id ] = $config; | |
| } | |
| } | |
| foreach ( $platforms as $platform_id => $config ) { | |
| // Validate platform configuration structure | |
| if ( ! is_array( $config ) ) { | |
| continue; | |
| } | |
| // Ensure all required fields are present | |
| if ( ! isset( $config['name'], $config['fetcher'], $config['mapper'] ) ) { | |
| continue; | |
| } | |
| // Validate field types | |
| if ( ! is_string( $config['name'] ) || ! is_string( $config['fetcher'] ) || ! is_string( $config['mapper'] ) ) { | |
| continue; | |
| } | |
| $this->platforms[ $platform_id ] = $config; | |
| } |
🤖 Prompt for AI Agents
In plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php
around lines 55 to 59, the current code only checks if 'fetcher' and 'mapper'
keys exist in the platform configuration but does not validate the structure or
required fields. Enhance the validation by verifying that the configuration
array contains all necessary keys with valid types or formats before assigning
it to $this->platforms. Implement explicit checks for each required field and
ensure the configuration meets expected criteria to prevent invalid platform
setups.
| ); | ||
| } | ||
|
|
||
| return new $fetcher_class(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider handling constructor parameters for fetcher and mapper classes.
The current implementation assumes fetcher and mapper classes have parameterless constructors. This could cause fatal errors if the classes require constructor arguments.
Consider either:
- Document that fetcher/mapper classes must have parameterless constructors
- Add support for constructor parameters through the platform configuration
- Wrap instantiation in try-catch blocks to handle constructor failures gracefully
Also applies to: 153-153
🏁 Script executed:
#!/bin/bash
# Check if there are any existing fetcher/mapper implementations that might have constructor parameters
# This will help determine if this is a real concern or theoretical
# Search for classes implementing PlatformFetcherInterface
ast-grep --pattern 'class $_ implements $_PlatformFetcherInterface$_ {
$$$
}'
# Search for classes implementing PlatformMapperInterface
ast-grep --pattern 'class $_ implements $_PlatformMapperInterface$_ {
$$$
}'
# Also search for any existing fetcher/mapper classes by common naming patterns
fd -e php -x grep -l "class.*Fetcher" {} \; | head -20
fd -e php -x grep -l "class.*Mapper" {} \; | head -20Length of output: 429
PlatformRegistry fetcher/mapper instantiation assumes parameterless constructors
Verified that there are currently no PlatformFetcherInterface or PlatformMapperInterface implementations in the codebase defining constructors with parameters—so the risk is theoretical today. To prevent future breakage if anyone adds a fetcher or mapper with required dependencies, please:
- Document in your developer or plugin docs (and/or in the PHPDoc for PlatformRegistry) that all fetcher and mapper classes must provide parameterless constructors.
- Optionally, consider refactoring
plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php(lines 116 and 153) to accept constructor arguments via configuration or a factory/DI container in the future.
🤖 Prompt for AI Agents
In plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php at
line 116, the code instantiates fetcher and mapper classes assuming they have
parameterless constructors, which could cause issues if future implementations
require constructor parameters. To fix this, add clear PHPDoc comments in
PlatformRegistry and relevant developer documentation stating that all fetcher
and mapper classes must have parameterless constructors. Additionally, consider
planning a refactor to allow passing constructor arguments via configuration or
a factory/DI container for better flexibility in the future.
|
Thank you, Naman! I now have my own migrators, but I still can't find the Shopify one. Am I missing something? 🤔 P.S. Not commenting on validation since we discussed this one to be fixed in the following PR. |
I've removed Shopify from testing instrucitons after you comment, as it is in this PR: #59780 |
* Added platform list command and Platform interfaces * lint issues * Added platform registry class * Added Shopify internal platform scaffolding * Add changefile(s) from automation for the following project(s): woocommerce * updated list command formating * lint issues * lint issues * Improve separation of concerns and eliminate unnecessary base class coupling. (#59803) * Added platform list command and Platform interfaces * lint issues * Moved credital setup logic to credential manager * Add changefile(s) from automation for the following project(s): woocommerce --------- Co-authored-by: github-actions <github-actions@github.com> --------- Co-authored-by: github-actions <github-actions@github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php (1)
57-63: Consider validating required platform fields before display.The current implementation uses null coalescing to provide empty strings for missing fields, which could hide improperly registered platforms. Consider validating that all required fields (name, fetcher, mapper) are present and skip platforms with missing data or show a warning.
foreach ( $platforms as $id => $details ) { + // Skip platforms missing required fields + if ( empty( $details['name'] ) || empty( $details['fetcher'] ) || empty( $details['mapper'] ) ) { + WP_CLI::warning( sprintf( 'Platform "%s" is missing required fields and will be skipped.', $id ) ); + continue; + } $formatted_items[] = array( 'id' => $id, - 'name' => $details['name'] ?? '', - 'fetcher' => $details['fetcher'] ?? '', - 'mapper' => $details['mapper'] ?? '', + 'name' => $details['name'], + 'fetcher' => $details['fetcher'], + 'mapper' => $details['mapper'], );plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.php (1)
68-70: Verify error handling consistency with SetupCommand.Similar to SetupCommand, this code calls
WP_CLI::error()followed by areturnstatement. Ensure consistent error handling behavior across commands.
🧹 Nitpick comments (1)
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.php (1)
64-64: Remove trailing whitespace.Line 64 contains trailing whitespace that should be removed.
- WP_CLI::log( "Credentials for '{$platform}' not found. Let's set them up." ); - + WP_CLI::log( "Credentials for '{$platform}' not found. Let's set them up." ); +
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
plugins/woocommerce/changelog/59780-feat-shopify-platform(1 hunks)plugins/woocommerce/changelog/59803-feedback-59741(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Commands/BaseCommand.php(0 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.php(2 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.php(2 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.php(2 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Core/CredentialManager.php(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/README.md(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.php(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.php(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatform.php(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Runner.php(3 hunks)
💤 Files with no reviewable changes (1)
- plugins/woocommerce/src/Internal/CLI/Migrator/Commands/BaseCommand.php
✅ Files skipped from review due to trivial changes (2)
- plugins/woocommerce/changelog/59803-feedback-59741
- plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/woocommerce/src/Internal/CLI/Migrator/Runner.php
- plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.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/src/Internal/CLI/Migrator/Core/CredentialManager.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatform.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.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/CLI/Migrator/Core/CredentialManager.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatform.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.php
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/generate-pr-description.mdc:0-0
Timestamp: 2025-07-21T05:22:46.426Z
Learning: Provide clear, step-by-step instructions for how to test the changes in the PR description.
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.
plugins/woocommerce/changelog/59780-feat-shopify-platform (2)
Learnt from: jorgeatorres
PR: #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: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/woo-build.mdc:0-0
Timestamp: 2025-07-18T14:55:02.778Z
Learning: To watch for changes during development, use 'pnpm --filter=@woocommerce/plugin-woocommerce watch:build' to ensure experimental features are active.
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.php (1)
Learnt from: tpaksu
PR: #59172
File: plugins/woocommerce/src/Internal/Fulfillments/Providers/MPLShippingProvider.php:14-21
Timestamp: 2025-06-26T10:47:27.212Z
Learning: In the WooCommerce fulfillments system, only the UPS shipping provider is currently fully implemented. All other shipping provider classes in plugins/woocommerce/src/Internal/Fulfillments/Providers/ are placeholders for future implementations, so empty shipping country arrays and stub methods in these classes are intentional and should not be flagged as issues.
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.php (1)
Learnt from: tpaksu
PR: #59172
File: plugins/woocommerce/src/Internal/Fulfillments/Providers/MPLShippingProvider.php:14-21
Timestamp: 2025-06-26T10:47:27.212Z
Learning: In the WooCommerce fulfillments system, only the UPS shipping provider is currently fully implemented. All other shipping provider classes in plugins/woocommerce/src/Internal/Fulfillments/Providers/ are placeholders for future implementations, so empty shipping country arrays and stub methods in these classes are intentional and should not be flagged as issues.
🪛 PHPMD (2.15.0)
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.php
35-35: Avoid unused parameters such as '$args'. (Unused Code Rules)
(UnusedFormalParameter)
51-51: Avoid unused parameters such as '$args'. (Unused Code Rules)
(UnusedFormalParameter)
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php
45-45: Avoid unused parameters such as '$args'. (Unused Code Rules)
(UnusedFormalParameter)
45-45: Avoid unused parameters such as '$assoc_args'. (Unused Code Rules)
(UnusedFormalParameter)
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.php
33-33: Avoid unused parameters such as '$platform_data'. (Unused Code Rules)
(UnusedFormalParameter)
🔇 Additional comments (9)
plugins/woocommerce/changelog/59780-feat-shopify-platform (1)
1-4: LGTM! Properly formatted changelog entry.The changelog entry correctly follows WooCommerce's format and accurately describes the Shopify platform addition to the CLI Migrator.
plugins/woocommerce/src/Internal/CLI/Migrator/Core/CredentialManager.php (1)
82-100: LGTM! Well-structured credential setup method.The
setup_credentialsmethod follows good practices:
- Proper input validation with early return
- Clear error messaging via WP_CLI::error
- Effective delegation to existing methods
- Descriptive logging for user feedback
The implementation integrates well with the existing credential management workflow.
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.php (1)
24-36: LGTM! Proper stub implementation for future development.The
ShopifyMapperclass correctly implements thePlatformMapperInterfacewith appropriate stub methods. The documentation clearly indicates this is a placeholder for future implementation, which aligns with the phased development approach.The unused parameter warning from static analysis is expected and acceptable for stub implementations.
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatform.php (1)
20-47: LGTM! Clean platform registration implementation.The
ShopifyPlatformclass properly implements the platform registration pattern:
- Correct use of WordPress filter system for extensibility
- Appropriate static method pattern for initialization
- Well-structured platform metadata with required fields
- Safe class references using
::classconstantsThe implementation integrates seamlessly with the new platform registry system.
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.php (1)
23-54: LGTM! Well-structured stub implementation with correct return formats.The
ShopifyFetcherclass properly implements thePlatformFetcherInterfacewith appropriate stub methods:
fetch_batchreturns the correct data structure withitems,cursor, andhasNextPagefieldsfetch_total_countreturns appropriate placeholder value- Documentation clearly indicates future implementation plans
The unused parameter warnings from static analysis are expected and acceptable for stub implementations that will be replaced with actual Shopify GraphQL API logic.
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.php (1)
14-41: Well-structured dependency injection implementation.The refactoring from inheritance to composition with explicit dependency injection is clean and follows best practices. The typed properties and init() method align with WooCommerce's DI container patterns.
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.php (1)
14-41: Consistent DI implementation across commands.The dependency injection pattern is implemented consistently with other refactored commands, maintaining good architectural cohesion.
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php (1)
13-31: Clean dependency injection with appropriate service selection.Good design choice to only inject
PlatformRegistrysince this command doesn't need credential management functionality.plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.php (1)
14-41: Good use of final keyword and consistent DI pattern.Making the class
finalis a good practice when inheritance is not intended. The dependency injection implementation maintains consistency with other commands.
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php (3)
55-59: Strengthen platform configuration validation to guard against unexpected inputs.The current validation only checks for fetcher and mapper existence but doesn't validate the configuration structure or required fields. This could lead to runtime errors with malformed platform configurations.
Apply enhanced validation to ensure robust input handling:
foreach ( $platforms as $platform_id => $config ) { + // Validate platform configuration structure + if ( ! is_array( $config ) ) { + continue; + } + + // Ensure all required fields are present + if ( ! isset( $config['name'], $config['fetcher'], $config['mapper'] ) ) { + continue; + } + + // Validate field types + if ( ! is_string( $config['name'] ) || ! is_string( $config['fetcher'] ) || ! is_string( $config['mapper'] ) ) { + continue; + } + - if ( isset( $config['fetcher'], $config['mapper'] ) ) { $this->platforms[ $platform_id ] = $config; - } }
116-116: Document parameterless constructor requirement for fetcher classes.The instantiation assumes fetcher classes have parameterless constructors, which could cause fatal errors if future implementations require constructor arguments.
As verified in previous reviews, this is currently not an issue but should be documented to prevent future breakage. Add PHPDoc comments stating that all fetcher classes must provide parameterless constructors, or consider supporting constructor parameters via configuration in the future.
153-153: Document parameterless constructor requirement for mapper classes.Similar to the fetcher method, this instantiation assumes mapper classes have parameterless constructors. This could cause issues if future implementations require constructor parameters.
Ensure documentation clearly states that all mapper classes must provide parameterless constructors to prevent future breakage.
🧹 Nitpick comments (1)
plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php (1)
206-214: Consider making credential fields configurable per platform.The current hardcoded approach works for the initial implementation, but consider making credential fields configurable through the platform configuration array to improve extensibility.
This would allow platforms to define their own credential requirements:
// In platform configuration 'credentials' => array( 'api_key' => 'Enter your API Access Token:', 'shop_url' => 'Enter your store URL:', ) // In method return $platform_config['credentials'] ?? array();
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.php(2 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.php(2 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.php(2 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php(1 hunks)plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.php
- plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.php
- plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php
- plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.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/src/Internal/CLI/Migrator/Core/PlatformRegistry.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/CLI/Migrator/Core/PlatformRegistry.php
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/generate-pr-description.mdc:0-0
Timestamp: 2025-07-21T05:22:46.426Z
Learning: Provide clear, step-by-step instructions for how to test the changes in the PR description.
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.
plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php (3)
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.
Learnt from: vladolaru
PR: #58784
File: plugins/woocommerce/src/Internal/Admin/Settings/Payments.php:431-439
Timestamp: 2025-06-17T14:19:30.933Z
Learning: In plugins/woocommerce/src/Internal/Admin/Settings/Payments.php, the process_payment_provider_states() method intentionally filters out payment providers that don't have a _suggestion_id. This is by design to only track state changes for gateways from partner extensions, not core WooCommerce gateways or other installed gateways.
Learnt from: samueljseay
PR: #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.
⏰ 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). (14)
- GitHub Check: Core API tests - @woocommerce/plugin-woocommerce [api]
- GitHub Check: Core e2e tests 4/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 3/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Core e2e tests 1/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 5/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: PHP: 7.4 WP: latest - 1 [WP 6.7.2] 2/2 - @woocommerce/plugin-woocommerce [unit:php]
- GitHub Check: Core e2e tests 2/6 - @woocommerce/plugin-woocommerce [e2e]
- GitHub Check: Lint - @woocommerce/plugin-woocommerce
- GitHub Check: build
🔇 Additional comments (4)
plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php (4)
1-30: LGTM: Clean class structure with proper dependency management.The file structure follows PHP best practices with strict typing, appropriate imports, and a clean constructor that delegates platform loading. The dependency injection approach aligns with WooCommerce's DI standards as mentioned in the PR objectives.
67-69: LGTM: Simple and correct getter implementation.
78-80: LGTM: Safe platform retrieval with proper null handling.
164-189: LGTM: Robust platform resolution with comprehensive error handling.Excellent implementation that handles multiple edge cases:
- Missing platform argument with default fallback
- Invalid platform with helpful error messages listing available platforms
- No registered platforms with clear guidance
- Proper use of WP_CLI methods for CLI context
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR introduces the
PlatformRegistryservice to the WooCommerce CLI Migrator and integrates it with the existing command infrastructure.Key Changes:
PlatformRegistry.php: Migrated from standalone plugin singleton pattern to a proper dependency injection serviceBaseCommand.php: Extendedinit()method to injectPlatformRegistryalongsideCredentialManagerListCommand.php: Replaced hardcoded platform data with realPlatformRegistryintegrationTechnical Details:
woocommerce_migrator_platforms) for extensibilityHow to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
define( 'WOOCOMMERCE_MIGRATOR_ENABLED', true );towp-config.phpwp wc migrator listcommand via WP-CLIwp wc migrator listagain and verify "Custom Platform" appears in the table alongside ShopifyChangelog entry
Changelog Entry Details
Significance
Type
Message
Add PlatformRegistry service to CLI Migrator with dependency injection integration