Implement Shopify Product Count Fetching with REST API Support #59935
Conversation
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. |
|
@xristos3490 I'll resolve the merge conflict with the base PR they are related to tests, but feel free to review the PR in current state. |
xristos3490
left a comment
xristos3490
left a comment
There was a problem hiding this comment.
Looking good and tests well on my end!
Left a couple of comments before approving!
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/README.md
Outdated
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/README.md
Outdated
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/README.md
Outdated
Show resolved
Hide resolved
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/README.md
Outdated
Show resolved
Hide resolved
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/PlatformRegistryTest.php
Show resolved
Hide resolved
📝 WalkthroughWalkthroughThis change introduces a new PlatformRegistry service and refactors the WooCommerce CLI Migrator commands to use dependency injection for platform and credential management. The Shopify platform is added as a built-in migration option with dedicated fetcher, mapper, and client classes. The command namespace is updated, and comprehensive tests and documentation are included for the new architecture. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WP_CLI
participant Runner
participant PlatformRegistry
participant CredentialManager
participant ShopifyPlatform
participant ShopifyFetcher
participant ShopifyMapper
participant ShopifyClient
User->>WP_CLI: Run CLI command (e.g., wp wc migrate products)
WP_CLI->>Runner: Register commands
Runner->>PlatformRegistry: Initialize platforms (calls ShopifyPlatform::init)
Runner->>WP_CLI: Register CLI commands (inject PlatformRegistry, CredentialManager)
User->>WP_CLI: Invoke command (e.g., SetupCommand)
WP_CLI->>SetupCommand: __invoke(args, assoc_args)
SetupCommand->>PlatformRegistry: resolve_platform(assoc_args)
SetupCommand->>PlatformRegistry: get_platform_credential_fields(platform)
SetupCommand->>CredentialManager: setup_credentials(platform, fields)
User->>WP_CLI: Invoke command (e.g., ProductsCommand --count)
WP_CLI->>ProductsCommand: __invoke(args, assoc_args)
ProductsCommand->>PlatformRegistry: resolve_platform(assoc_args)
ProductsCommand->>PlatformRegistry: get_fetcher(platform)
ProductsCommand->>ShopifyFetcher: fetch_total_count(args)
ShopifyFetcher->>ShopifyClient: rest_request('/products/count.json', query_params)
ShopifyClient-->>ShopifyFetcher: Return product count
ShopifyFetcher-->>ProductsCommand: Return count
ProductsCommand-->>WP_CLI: Output count to user
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (6)
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ListCommandTest.php (2)
40-41: Use mocks for better test isolation.Similar to other command tests, consider using mocks instead of real instances to avoid potential side effects and improve test isolation.
- $this->platform_registry = new PlatformRegistry(); + $this->platform_registry = $this->createMock( PlatformRegistry::class );
77-80: Add meaningful assertions for dependency injection verification.The test should verify that the dependency was actually injected and stored properly, not just that the method call succeeded.
public function test_dependency_injection_properties() { $this->command->init( $this->platform_registry ); - $this->assertTrue( true ); + // Verify the dependency was properly injected using reflection + $reflection = new \ReflectionClass( $this->command ); + $platform_registry_prop = $reflection->getProperty( 'platform_registry' ); + $platform_registry_prop->setAccessible( true ); + $this->assertSame( $this->platform_registry, $platform_registry_prop->getValue( $this->command ) ); }plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/README.md (3)
32-33: Interface references may need correction.The interface references appear to use a different namespace pattern than what's typical for WooCommerce. Verify these are the correct interface names and namespaces.
#!/bin/bash # Verify the correct interface names and namespaces rg -A 5 "PlatformFetcherInterface|PlatformMapperInterface" --type php
89-89: Reference to "WooCommerce Migrator documentation" is unclear.The line references "WooCommerce Migrator documentation" but doesn't specify where this documentation can be found or provide a link.
-For more details, refer to the WooCommerce Migrator documentation. +For more details, refer to the main WooCommerce Migrator documentation in the parent directory or visit [specific URL if available].
53-56: Expand the dependency handling section.This section could benefit from more detailed examples of how to properly check for WooCommerce Migrator activation and handle autoloading.
Your plugin should: -- Check if WooCommerce Migrator is active -- Handle its own autoloading -- Initialize during appropriate WordPress hooks +- Check if WooCommerce Migrator is active: + ```php + if ( ! class_exists( 'Automattic\WooCommerce\Internal\CLI\Migrator\Core\PlatformRegistry' ) ) { + add_action( 'admin_notices', function() { + echo '<div class="notice notice-error"><p>Your Platform Migrator requires WooCommerce Migrator to be active.</p></div>'; + } ); + return; + } + ``` +- Handle its own autoloading via composer.json +- Initialize during the `init` or `plugins_loaded` WordPress hooksplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/PlatformRegistryTest.php (1)
17-17: Address the previous feedback about namespace validation testing.The past review comment indicates that validation tests for wrong namespaces or empty strings may have been missed during PR merging. While the current test suite covers some validation scenarios, consider adding explicit tests for malformed namespaces and empty string validation.
🧹 Nitpick comments (15)
plugins/woocommerce/src/Internal/CLI/Migrator/Runner.php (1)
26-27: Consider adding error handling for platform initialization.The platform initialization approach is sound, but consider adding error handling around
ShopifyPlatform::init()to prevent CLI command registration failures if platform initialization encounters issues.private static function init_platforms(): void { - ShopifyPlatform::init(); + try { + ShopifyPlatform::init(); + } catch ( Exception $e ) { + WP_CLI::warning( 'Failed to initialize Shopify platform: ' . $e->getMessage() ); + } }Also applies to: 70-72
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockWPCLI.php (1)
35-46: Consider adding common WP_CLI methods for comprehensive testing.The mock only implements
debug()andwarning()methods. Consider adding other commonly used WP_CLI methods likelog(),error(),success(), andline()to provide better test coverage.public static function warning( $message ): void { - self::$last_warning_message = $message; + self::$warning_messages[] = $message; + } + + /** + * Mock log method. + * + * @param string $message Log message. + */ + public static function log( $message ): void { + // Could store in a separate array if needed for testing + } + + /** + * Mock error method. + * + * @param string $message Error message. + */ + public static function error( $message ): void { + // In real WP_CLI this would exit, but for testing we just store it + // Could throw an exception or set a flag for test verification + } + + /** + * Mock success method. + * + * @param string $message Success message. + */ + public static function success( $message ): void { + // Store success messages if needed for testing }plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatform.php (1)
38-47: Add input validation for the platforms array.While the current implementation works, consider adding basic validation to ensure the
$platformsparameter is an array to prevent potential issues if the filter is misused.public static function register_platform( array $platforms ): array { + if ( ! is_array( $platforms ) ) { + return array(); + } + $platforms['shopify'] = array( 'name' => 'Shopify', 'description' => 'Import products and data from Shopify stores', 'fetcher' => ShopifyFetcher::class, 'mapper' => ShopifyMapper::class, ); return $platforms; }plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/SetupCommandTest.php (1)
48-50: Consider using mocks for better test isolation.Using real instances of
CredentialManagerandPlatformRegistryin unit tests can lead to side effects and make tests less predictable. Consider using mocks or stubs for better isolation.public function setUp(): void { parent::setUp(); - $this->credential_manager = new CredentialManager(); - $this->platform_registry = new PlatformRegistry(); + $this->credential_manager = $this->createMock( CredentialManager::class ); + $this->platform_registry = $this->createMock( PlatformRegistry::class ); $this->command = new SetupCommand(); }plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ResetCommandTest.php (1)
48-50: Consider using mocks for better test isolation.Similar to the SetupCommandTest, using real instances can cause side effects. Mock objects would provide better test isolation and control.
public function setUp(): void { parent::setUp(); - $this->credential_manager = new CredentialManager(); - $this->platform_registry = new PlatformRegistry(); + $this->credential_manager = $this->createMock( CredentialManager::class ); + $this->platform_registry = $this->createMock( PlatformRegistry::class ); $this->command = new ResetCommand(); }plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php (3)
46-48: Consider redesigning method signature instead of ignoring unused parameters.The
phpcs:ignoreand explicitunset()suggest the method signature might not be optimal for this command. Consider if the parameters are truly needed.If the parameters are required by the WP-CLI interface but not used, consider adding a comment explaining why they're required:
public function __invoke( array $args, array $assoc_args ): void { - // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed - unset( $args, $assoc_args ); + // Parameters required by WP-CLI interface but not used for listing all platforms + unset( $args, $assoc_args );
56-78: Simplify the separator row logic for better maintainability.The current implementation is complex with manual index tracking. Consider a simpler approach.
- $formatted_items = array(); - $platform_count = count( $platforms ); - $current_index = 0; - - foreach ( $platforms as $id => $details ) { - $formatted_items[] = array( - 'id' => $id, - 'name' => $details['name'] ?? '', - 'fetcher' => $details['fetcher'] ?? '', - 'mapper' => $details['mapper'] ?? '', - ); - - // Add separator row between platforms (but not after the last one). - ++$current_index; - if ( $current_index < $platform_count ) { - $formatted_items[] = array( - 'id' => str_repeat( '-', 20 ), - 'name' => str_repeat( '-', 25 ), - 'fetcher' => str_repeat( '-', 30 ), - 'mapper' => str_repeat( '-', 30 ), - ); - } - } + $formatted_items = array(); + $platform_keys = array_keys( $platforms ); + + foreach ( $platforms as $id => $details ) { + $formatted_items[] = array( + 'id' => $id, + 'name' => $details['name'] ?? '', + 'fetcher' => $details['fetcher'] ?? '', + 'mapper' => $details['mapper'] ?? '', + ); + + // Add separator except after the last platform + if ( $id !== end( $platform_keys ) ) { + $formatted_items[] = array( + 'id' => str_repeat( '-', 20 ), + 'name' => str_repeat( '-', 25 ), + 'fetcher' => str_repeat( '-', 30 ), + 'mapper' => str_repeat( '-', 30 ), + ); + } + }
61-66: Add validation for platform data structure.Consider validating the platform data structure to prevent issues if the registry contains malformed data.
foreach ( $platforms as $id => $details ) { + if ( ! is_array( $details ) ) { + continue; + } + $formatted_items[] = array( 'id' => $id, 'name' => $details['name'] ?? '', 'fetcher' => $details['fetcher'] ?? '', 'mapper' => $details['mapper'] ?? '', );plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.php (2)
58-58: Add explicit return type for consistency.For consistency with other methods and better type safety, consider adding an explicit return type.
-public function __invoke( array $args, array $assoc_args ) { +public function __invoke( array $args, array $assoc_args ): void {
59-60: Validate input before passing to resolve_platform.Consider adding basic validation for the
$assoc_argsparameter before passing it toresolve_platform()to ensure robust error handling.public function __invoke( array $args, array $assoc_args ): void { + // Unused positional arguments + unset( $args ); + // Resolve and validate the platform. $platform = $this->platform_registry->resolve_platform( $assoc_args );plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ProductsCommandTest.php (1)
48-50: Consider using mocks for better test isolation.Using real instances of
CredentialManagerandPlatformRegistryin unit tests can create unwanted side effects and dependencies. Consider using mocks or test doubles for better isolation.- $this->credential_manager = new CredentialManager(); - $this->platform_registry = new PlatformRegistry(); + $this->credential_manager = $this->createMock( CredentialManager::class ); + $this->platform_registry = $this->createMock( PlatformRegistry::class );plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.php (2)
57-64: Address unused parameter warning.The static analysis correctly identifies that the
$argsparameter is unused in the stub implementation. Consider adding a comment or parameter documentation to clarify this is intentional for the stub.- public function fetch_batch( array $args ): array { + public function fetch_batch( array $args ): array { // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter // Stub implementation - will be replaced with actual Shopify GraphQL API calls. + // The $args parameter will be used in the future GraphQL implementation. return array( 'items' => array(), 'cursor' => null, 'hasNextPage' => false, ); }
86-94: Add more specific error handling for different failure scenarios.Consider handling different types of API errors more specifically to provide better debugging information and user feedback.
if ( is_wp_error( $response ) ) { - \WP_CLI::warning( 'Could not fetch total product count from Shopify REST API: ' . $response->get_error_message() ); + $error_code = $response->get_error_code(); + $error_message = $response->get_error_message(); + + if ( 'http_request_failed' === $error_code ) { + \WP_CLI::warning( 'Network error while fetching product count: ' . $error_message ); + } elseif ( strpos( $error_code, '401' ) !== false ) { + \WP_CLI::warning( 'Authentication failed. Please check your Shopify credentials.' ); + } else { + \WP_CLI::warning( 'Could not fetch total product count from Shopify REST API: ' . $error_message ); + } return 0; }plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClient.php (1)
95-97: Consider making API version configurable.While '2025-04' may now be a valid Shopify API version, hardcoding API versions can create maintenance challenges. Consider making this configurable to allow for easier updates when new API versions are released.
class ShopifyClient { + /** + * Shopify API version to use. + */ + private const API_VERSION = '2025-04'; + private function build_rest_url( string $domain, string $path, array $query_params ): string { // Ensure the domain has the protocol. if ( ! preg_match( '~^https?://~i', $domain ) ) { $domain = 'https://' . $domain; } $shop_url = untrailingslashit( $domain ); - // Use the latest stable API version. - $api_version = '2025-04'; + $api_version = self::API_VERSION; $rest_endpoint = "{$shop_url}/admin/api/{$api_version}{$path}";plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/CredentialManagerTest.php (1)
122-132: Remove unused variable to improve code quality.The
$fieldsvariable is defined but never used in this test method. Since the test only verifies method existence and callability, the variable can be removed.- public function test_prompt_for_credentials() { - $fields = array( - 'shop_url' => 'Enter shop URL:', - 'access_token' => 'Enter access token:', - ); - - // Since prompt_for_credentials uses STDIN, we can't really test it in unit tests. + public function test_prompt_for_credentials() { + // Since prompt_for_credentials uses STDIN, we can't really test it in unit tests.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
plugins/woocommerce/changelog/59778-feat-platform-registry(1 hunks)plugins/woocommerce/changelog/59780-feat-shopify-platform(1 hunks)plugins/woocommerce/changelog/59784-migrator-tests(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/ShopifyClient.php(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)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ListCommandTest.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ProductsCommandTest.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ResetCommandTest.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/SetupCommandTest.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/CredentialManagerTest.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/PlatformRegistryTest.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockPlatformFetcher.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockPlatformMapper.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockWPCLI.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClientTest.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcherTest.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapperTest.php(1 hunks)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatformTest.php(1 hunks)
💤 Files with no reviewable changes (1)
- plugins/woocommerce/src/Internal/CLI/Migrator/Commands/BaseCommand.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/CLI/Migrator/Commands/SetupCommandTest.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClient.phpplugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcherTest.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ProductsCommandTest.phpplugins/woocommerce/src/Internal/CLI/Migrator/Core/CredentialManager.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/PlatformRegistryTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClientTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/CredentialManagerTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapperTest.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Runner.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ResetCommandTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockWPCLI.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatform.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatformTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ListCommandTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockPlatformFetcher.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockPlatformMapper.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/CLI/Migrator/Commands/SetupCommandTest.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClient.phpplugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcherTest.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ProductsCommandTest.phpplugins/woocommerce/src/Internal/CLI/Migrator/Core/CredentialManager.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/PlatformRegistryTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClientTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/CredentialManagerTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapperTest.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.phpplugins/woocommerce/src/Internal/CLI/Migrator/Runner.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ResetCommandTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockWPCLI.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatform.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatformTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ListCommandTest.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockPlatformFetcher.phpplugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockPlatformMapper.php
🧠 Learnings (12)
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/SetupCommandTest.php (1)
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.
plugins/woocommerce/changelog/59778-feat-platform-registry (1)
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.
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ProductsCommandTest.php (1)
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.
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/README.md (1)
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.
plugins/woocommerce/changelog/59780-feat-shopify-platform (1)
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.
plugins/woocommerce/changelog/59803-feedback-59741 (3)
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-28T05:05:41.091Z
Learning: To watch for changes, use 'pnpm --filter=@woocommerce/plugin-woocommerce watch:build' to ensure experimental features are active during development builds.
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.
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ResetCommandTest.php (2)
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.
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.
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockWPCLI.php (3)
Learnt from: vladolaru
PR: #59160
File: plugins/woocommerce/tests/php/src/Internal/Admin/Settings/PaymentsProviders/WooPayments/WooPaymentsServiceTest.php:360-373
Timestamp: 2025-06-25T15:39:25.166Z
Learning: Test mocks should accurately simulate the behavior of the functions they replace, including return value semantics. For WordPress functions like update_option, the mock should properly return false when the value hasn't changed and true when it has, to avoid masking logic errors in tests.
Learnt from: vladolaru
PR: #59966
File: plugins/woocommerce/tests/php/src/Internal/Admin/Settings/PaymentsProviders/WooPayments/WooPaymentsServiceTest.php:5515-5574
Timestamp: 2025-07-24T10:44:40.385Z
Learning: vladolaru prefers to keep both direct value comparison (===) and serialized string comparison (maybe_serialize) in WordPress function mocks like update_option, as they may differ in edge cases and provide more comprehensive test coverage rather than just mirroring core WordPress internal behavior.
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.
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.
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatformTest.php (1)
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.
plugins/woocommerce/changelog/59784-migrator-tests (7)
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: 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.
Learnt from: prettyboymp
PR: #59048
File: .github/workflows/cherry-pick-milestoned-prs.yml:60-83
Timestamp: 2025-06-26T12:45:40.709Z
Learning: WooCommerce uses WordPress versioning conventions where minor versions in X.Y.Z format are constrained to 0-9 (Y cannot exceed 9). This means version increment logic should reset minor to 0 and increment major when minor reaches 9, rather than allowing two-digit minor versions like 9.10 or 9.11.
Learnt from: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/woo-build.mdc:0-0
Timestamp: 2025-07-28T05:05:41.091Z
Learning: To watch for changes, use 'pnpm --filter=@woocommerce/plugin-woocommerce watch:build' to ensure experimental features are active during development builds.
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: 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: CR
PR: woocommerce/woocommerce#0
File: .cursor/rules/code-quality.mdc:0-0
Timestamp: 2025-07-18T14:54:36.232Z
Learning: Applies to **/*.{php,js,jsx,ts,tsx} : Ensure code has unit or E2E tests where applicable
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockPlatformMapper.php (1)
Learnt from: vladolaru
PR: #59160
File: plugins/woocommerce/tests/php/src/Internal/Admin/Settings/PaymentsProviders/WooPayments/WooPaymentsServiceTest.php:360-373
Timestamp: 2025-06-25T15:39:25.166Z
Learning: Test mocks should accurately simulate the behavior of the functions they replace, including return value semantics. For WordPress functions like update_option, the mock should properly return false when the value hasn't changed and true when it has, to avoid masking logic errors in tests.
🧬 Code Graph Analysis (2)
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapperTest.php (3)
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.php (1)
ShopifyMapper(24-37)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatformTest.php (1)
setUp(25-29)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockPlatformMapper.php (1)
map_product_data(24-31)
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ResetCommandTest.php (7)
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.php (1)
ResetCommand(14-71)plugins/woocommerce/src/Internal/CLI/Migrator/Core/CredentialManager.php (1)
CredentialManager(12-119)plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php (1)
PlatformRegistry(16-265)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ListCommandTest.php (1)
setUp(37-42)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ProductsCommandTest.php (1)
setUp(45-51)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/CredentialManagerTest.php (1)
setUp(29-32)plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/SetupCommandTest.php (1)
setUp(45-51)
🪛 PHPMD (2.15.0)
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.php
67-67: Avoid unused parameters such as '$args'. (Unused Code Rules)
(UnusedFormalParameter)
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.php
57-57: Avoid unused parameters such as '$args'. (Unused Code Rules)
(UnusedFormalParameter)
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClientTest.php
63-63: Avoid unused parameters such as '$preempt'. (Unused Code Rules)
(UnusedFormalParameter)
97-97: Avoid unused parameters such as '$preempt'. (Unused Code Rules)
(UnusedFormalParameter)
97-97: Avoid unused parameters such as '$parsed_args'. (Unused Code Rules)
(UnusedFormalParameter)
267-267: Avoid unused parameters such as '$preempt'. (Unused Code Rules)
(UnusedFormalParameter)
267-267: Avoid unused parameters such as '$parsed_args'. (Unused Code Rules)
(UnusedFormalParameter)
300-300: Avoid unused parameters such as '$preempt'. (Unused Code Rules)
(UnusedFormalParameter)
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/CredentialManagerTest.php
123-123: Avoid unused local variables such as '$fields'. (Unused Code Rules)
(UnusedLocalVariable)
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.php
33-33: Avoid unused parameters such as '$platform_data'. (Unused Code Rules)
(UnusedFormalParameter)
🪛 Gitleaks (8.27.2)
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/CredentialManagerTest.php
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (67)
plugins/woocommerce/changelog/59780-feat-shopify-platform (1)
1-4: LGTM! Changelog entry follows WooCommerce conventions.The changelog entry is properly formatted with appropriate significance level (minor) and type (add) for adding Shopify platform support to the CLI Migrator.
plugins/woocommerce/changelog/59778-feat-platform-registry (1)
1-4: LGTM! Changelog entry properly documents PlatformRegistry addition.The changelog entry correctly follows WooCommerce format and appropriately categorizes the addition of the PlatformRegistry service as a minor addition.
plugins/woocommerce/changelog/59784-migrator-tests (1)
1-4: LGTM! Test suite changelog entry is appropriately categorized.The changelog entry correctly uses "dev" type for test suite updates and appropriately describes the scope of changes including architecture alignment and coding standards fixes.
plugins/woocommerce/changelog/59803-feedback-59741 (1)
1-4: LGTM! Clear documentation of architectural refactoring.The changelog entry appropriately documents the refactoring work with clear explanation of the separation of concerns improvements and elimination of inheritance patterns.
plugins/woocommerce/src/Internal/CLI/Migrator/Runner.php (2)
32-32: Document the breaking change in command namespace.The command namespace has changed from
wc migratortowc migrate, which is a breaking change that will affect existing users and scripts. Ensure this is properly documented in release notes or migration guides.Also applies to: 41-41, 49-49, 57-57
11-11: LGTM! Clean dependency injection and platform initialization pattern.The addition of ShopifyPlatform import and the initialization pattern before command registration follows good practices. The use of WooCommerce's dependency injection container for command instantiation is appropriate.
Also applies to: 25-28
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.php (1)
33-36: LGTM - Stub implementation is appropriately documented.The stub implementation is clearly documented and appropriate for the current phase. The unused parameter warning from static analysis is expected for placeholder methods.
Consider adding a suppression annotation to silence the static analysis warning:
/** * Maps raw Shopify product data to a standardized array format. * * @param object $platform_data The raw product data object from Shopify (e.g., Shopify product node). + * @SuppressWarnings(PHPMD.UnusedFormalParameter) * * @return array A standardized array representing the product, understandable by the WooCommerce_Product_Importer. */ public function map_product_data( object $platform_data ): array {plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockPlatformFetcher.php (1)
24-48: LGTM - Well-structured mock implementation.The mock provides realistic data structures that match the interface contract. The fixed return values are appropriate for basic testing scenarios.
For more advanced testing scenarios, consider making the mock configurable:
class MockPlatformFetcher implements PlatformFetcherInterface { + /** + * Mock data to return from fetch_batch. + * + * @var array + */ + public static $mock_batch_data = null; + + /** + * Mock count to return from fetch_total_count. + * + * @var int + */ + public static $mock_total_count = 42; + /** * {@inheritdoc} * * @param array $args Arguments for fetching. */ public function fetch_batch( array $args ): array { - return array( + return self::$mock_batch_data ?? array(plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockPlatformMapper.php (1)
24-31: LGTM - Well-designed mock with safe property access.The mock implementation effectively combines fixed and derived values while safely handling missing properties using null coalescing. This provides realistic behavior for testing scenarios.
The use of
??operators ensures the mock doesn't break when test data is incomplete, which is excellent defensive programming for test mocks.plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatform.php (2)
27-29: LGTM! Proper initialization pattern with security consideration.The use of
finalon theinit()method is a good security practice preventing method overriding, and the static initialization pattern is appropriate for platform registration.
42-43: Resolved: Referenced Shopify classes are presentVerified that both
ShopifyFetcherandShopifyMapperclasses exist under
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.phpplugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapper.phpNo further action required.
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ListCommand.php (1)
15-31: Excellent refactoring to dependency injection pattern.The transition from inheritance to explicit dependency injection improves testability and separation of concerns. The
finalkeyword oninit()prevents method overriding, which is a good security practice.plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ResetCommand.php (2)
16-41: Excellent dependency injection implementation.The refactoring from inheritance to explicit dependency injection improves testability and follows SOLID principles. The
finalkeyword oninit()prevents method overriding, which is good for security.
62-69: LGTM! Excellent error handling and user feedback.The credential validation, error messaging, and success feedback provide a good user experience. The logic flow is clear and handles edge cases appropriately.
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.php (1)
38-41: Consider adding validation for dependency injection.The
initmethod should validate that the dependencies are not null to prevent runtime errors if called incorrectly.final public function init( CredentialManager $credential_manager, PlatformRegistry $platform_registry ): void { + if ( null === $credential_manager || null === $platform_registry ) { + throw new \InvalidArgumentException( 'Dependencies cannot be null' ); + } $this->credential_manager = $credential_manager; $this->platform_registry = $platform_registry; }Likely an incorrect or invalid review comment.
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClientTest.php (6)
44-81: LGTM! Comprehensive test coverage for successful API requests.The test properly mocks HTTP responses, verifies URL construction with correct API version, validates authorization headers, and confirms response parsing. Good use of WordPress testing patterns.
86-122: LGTM! Good coverage of query parameter handling.The test effectively validates that query parameters are properly appended to URLs and the response is correctly processed.
127-157: LGTM! Excellent error handling test coverage.The test suite comprehensively covers various error scenarios including credential issues, HTTP failures, API errors, and invalid JSON responses. Proper validation of error codes and messages ensures robust error handling.
Also applies to: 161-184, 189-220, 225-251
256-282: LGTM! Good coverage of URL protocol handling.The test ensures that the client properly handles domains without protocols by automatically prepending https://.
287-319: LGTM! Thorough POST request validation.The test properly validates HTTP method, content-type headers, JSON encoding of request body, and response parsing for POST requests.
63-63: Static analysis false positives for unused filter parameters.The PHPMD warnings about unused
$preemptand$parsed_argsparameters are false positives. These parameters are required by WordPress'spre_http_requestfilter signature but are intentionally unused in these test callbacks. This is standard WordPress testing practice.Also applies to: 97-97, 267-267, 300-300
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyMapperTest.php (6)
18-33: LGTM! Well-structured test class setup.The test class properly extends
WC_Unit_Test_Case, uses typed properties, and initializes the mapper in the setup method following WordPress testing conventions.
38-40: LGTM! Important interface compliance validation.This test ensures the ShopifyMapper properly implements the PlatformMapperInterface, which is crucial for platform interoperability.
45-89: LGTM! Comprehensive stub implementation testing.The tests properly validate the current stub behavior, returning an empty array for various input scenarios. This provides a good foundation for testing when the actual implementation is added.
94-101: LGTM! Smart use of reflection for type validation.Using reflection to validate method signatures ensures that the
map_product_datamethod maintains correct type declarations (object parameter, array return type).
106-136: LGTM! Excellent coverage of different object types.The test validates that the mapper accepts various object types (stdClass, array-cast objects, custom objects), ensuring flexibility in handling different data structures from Shopify.
141-172: LGTM! Good documentation of future enhancement expectations.This test serves as excellent documentation, showing the expected Shopify product data structure and confirming the current stub behavior. This will be valuable when implementing the actual mapping logic.
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyPlatformTest.php (6)
25-38: LGTM! Proper test lifecycle management.The setup and teardown methods properly initialize the Shopify platform and clean up filters between tests, ensuring test isolation.
43-68: LGTM! Comprehensive platform registration validation.The tests thoroughly validate that the Shopify platform is correctly registered with proper configuration including name, fetcher class, mapper class, and description.
73-87: LGTM! Good validation of initialization process.The test properly validates that
ShopifyPlatform::init()registers the platform filter and that the platform is available after initialization.
92-105: LGTM! Essential platform structure validation.The test ensures the platform configuration has all required keys and that the referenced fetcher and mapper classes actually exist, preventing runtime errors.
110-122: LGTM! Important safety test for multiple initialization.The test ensures that calling
ShopifyPlatform::init()multiple times doesn't create duplicate platform registrations, which is crucial for preventing configuration conflicts.
127-148: LGTM! Good filter integration testing.The test validates that the Shopify platform properly integrates with the WordPress filter system and coexists with other platforms, using appropriate filter priorities.
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClient.php (5)
11-33: LGTM! Well-designed dependency injection pattern.The class uses a clean dependency injection pattern with an empty constructor and separate
initmethod, allowing for proper lifecycle management and testability.
63-78: LGTM! Robust credential validation with helpful error messages.The credential handling properly validates required fields and provides clear error messages guiding users to run the setup command. The mapping from stored to expected format is also well-handled.
88-104: LGTM! Solid URL construction with proper protocol handling.The URL building logic properly handles protocol detection, normalizes URLs with
untrailingslashit, and correctly appends query parameters using WordPress'sadd_query_argfunction.
114-129: LGTM! Proper HTTP request configuration for Shopify API.The request arguments are correctly configured with appropriate headers (X-Shopify-Access-Token, Content-Type), timeout settings, and conditional JSON body encoding for POST/PUT requests.
138-162: LGTM! Comprehensive response processing with robust error handling.The response processing handles all error scenarios well: WP_Error responses, HTTP error status codes, API error responses, and JSON decoding failures. Error messages are informative and include relevant context.
plugins/woocommerce/src/Internal/CLI/Migrator/Commands/ProductsCommand.php (4)
14-41: LGTM! Excellent refactoring to dependency injection pattern.The removal of
BaseCommandinheritance in favor of explicit dependency injection makes the class more testable and follows better design principles. The typed properties and proper initialization method are well-implemented.
68-84: LGTM! Good integration with platform registry system.The platform resolution using
PlatformRegistry::resolve_platform()and dynamic credential field retrieval makes the command more flexible and maintainable across different platforms.
86-126: LGTM! Well-implemented count functionality with good user experience.The new
--countflag functionality is properly implemented with:
- Clear separation in a dedicated method
- Support for status filtering
- Good error handling and user feedback
- Proper integration with platform-specific fetchers
The user experience is enhanced with informative success messages that include filter information.
67-67: Static analysis false positive for required WP-CLI signature.The
$argsparameter is required by WP-CLI's command signature even when not used. This is a false positive from PHPMD. If desired, you could add a suppression comment:// phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.Found -- Required by WP-CLI signature public function __invoke( array $args, array $assoc_args ): void {plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/PlatformRegistryTest.php (10)
22-25: LGTM - Proper test isolation.The tearDown method correctly cleans up filters to ensure test isolation.
30-50: LGTM - Comprehensive basic registration test.The test properly verifies platform registration and retrieval functionality with all required assertions.
55-73: LGTM - Clear single platform retrieval test.The test effectively verifies single platform retrieval functionality.
88-148: LGTM - Excellent validation coverage.These tests properly validate that incomplete platform configurations are rejected, demonstrating good input validation and security practices.
285-324: LGTM - Good empty string validation.These tests properly validate that platforms with empty string fetchers or mappers are rejected, addressing part of the validation concerns mentioned in previous reviews.
329-416: LGTM - Comprehensive exception handling tests.These tests properly validate error handling for invalid class configurations, ensuring robust input validation and appropriate error reporting.
443-461: LGTM - Addresses namespace validation concerns.This test properly validates handling of malformed class names and namespaces, which addresses part of the validation concerns mentioned in previous reviews.
466-510: LGTM - Sophisticated validation testing.These tests demonstrate excellent security-conscious testing by using reflection to validate edge cases and ensure proper type validation for platform configurations.
515-527: LGTM - Proper handling of test environment limitations.The test correctly handles the WP_CLI dependency with appropriate skip logic while still validating the expected behavior.
532-551: LGTM - Good edge case coverage.This test properly validates handling of platform IDs with special characters, demonstrating good robustness testing.
plugins/woocommerce/src/Internal/CLI/Migrator/Core/PlatformRegistry.php (6)
1-30: LGTM - Well-structured class design.The class follows good design principles with proper encapsulation, strict typing, and appropriate initialization.
37-63: LGTM - Excellent input validation and security practices.This method properly validates filter inputs, checks for required fields, and ensures data types are correct. This aligns well with the coding guideline to guard against unexpected inputs and sanitize potentially dangerous data.
94-146: LGTM - Comprehensive validation and security measures.This method exemplifies excellent security practices with multiple validation layers:
- Input type validation
- Class existence checks
- Interface compliance verification
- Proper dependency injection
This fully aligns with the coding guidelines to guard against unexpected inputs and validate potentially dangerous data.
157-209: LGTM - Consistent and comprehensive validation.This method maintains the same excellent validation standards as get_fetcher, ensuring consistent security practices across the class. The parallel structure makes the code maintainable and predictable.
219-244: LGTM - User-friendly validation with good error handling.This method provides excellent user experience with clear error messages and helpful fallback behavior, while maintaining proper validation of platform existence.
253-264: LGTM - Functional implementation for current needs.The method correctly provides platform-specific credential fields. While currently hardcoded for Shopify, this approach is acceptable for the initial implementation and can be extended when additional platforms are added.
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Core/CredentialManagerTest.php (3)
29-42: LGTM - Proper test setup and cleanup.The test setup and teardown methods properly manage test isolation by cleaning up stored credentials.
47-58: LGTM - Valid credential storage test with test data.The test properly validates credential storage and retrieval. The API token flagged by static analysis is clearly test data (
shpat_test123456789) and not a real credential, so this is a false positive.
137-161: LGTM - Excellent security-conscious testing.These tests properly validate credential storage mechanisms and edge cases, demonstrating good security practices in testing credential management functionality.
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcherTest.php (6)
33-44: LGTM - Well-structured test setup with proper mocking.The test setup properly handles the test environment limitations with MockWPCLI and creates appropriate mocks for isolated testing.
49-66: LGTM - Solid basic functionality test.The test properly validates the happy path for product count fetching with appropriate mocking and assertions.
71-128: LGTM - Comprehensive filter testing.These tests effectively validate filter processing and parameter mapping, ensuring that various filter combinations are properly handled and passed to the Shopify API.
133-179: LGTM - Excellent ID-based counting logic with edge case handling.These tests properly validate the optimization for ID-based counting and handle edge cases like empty values appropriately. The expectation that no API calls are made when IDs are provided is correctly tested.
184-212: LGTM - Robust error handling testing.These tests provide excellent coverage of error scenarios including API errors and malformed responses, with proper validation of error logging. This demonstrates good defensive programming practices.
217-277: LGTM - Excellent edge case coverage and input validation.These tests properly validate edge cases including case normalization and input sanitization by ignoring unknown filter arguments. This demonstrates good security practices in handling user inputs.
| $platform = $this->platform_registry->resolve_platform( $assoc_args ); | ||
|
|
||
| // Get platform-specific credential fields and set them up. | ||
| $required_fields = $this->platform_registry->get_platform_credential_fields( $platform ); | ||
| if ( empty( $required_fields ) ) { | ||
| WP_CLI::error( "The platform '{$platform}' does not have configured credential fields." ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Validate platform input to prevent injection attacks.
The platform value should be sanitized before being used in error messages or passed to other methods to prevent potential security issues.
// Resolve and validate the platform.
- $platform = $this->platform_registry->resolve_platform( $assoc_args );
+ $platform = $this->platform_registry->resolve_platform( $assoc_args );
+ $platform = sanitize_text_field( $platform ); // Sanitize for security
// Get platform-specific credential fields and set them up.
$required_fields = $this->platform_registry->get_platform_credential_fields( $platform );
if ( empty( $required_fields ) ) {
- WP_CLI::error( "The platform '{$platform}' does not have configured credential fields." );
+ WP_CLI::error( sprintf( "The platform '%s' does not have configured credential fields.", esc_html( $platform ) ) );
return;
}🤖 Prompt for AI Agents
In plugins/woocommerce/src/Internal/CLI/Migrator/Commands/SetupCommand.php
around lines 60 to 67, the platform value used in the error message and method
calls is not sanitized, which could lead to injection vulnerabilities. Sanitize
the $platform variable before using it by applying appropriate escaping or
validation functions to ensure it contains only expected safe characters. Use
the sanitized value in the error message and when passing it to other methods to
prevent security issues.
| public function setup_credentials( string $platform_slug, array $required_fields ): void { | ||
| if ( empty( $required_fields ) ) { | ||
| WP_CLI::error( 'No credential fields specified for setup.' ); | ||
| return; | ||
| } | ||
|
|
||
| WP_CLI::log( 'Configuring credentials for ' . ucfirst( $platform_slug ) . '...' ); | ||
|
|
||
| $credentials = $this->prompt_for_credentials( $required_fields ); | ||
| $this->save_credentials( $platform_slug, $credentials ); | ||
| } |
There was a problem hiding this comment.
Add input validation for platform_slug and required_fields structure.
The method should validate input parameters to guard against unexpected inputs and ensure security.
public function setup_credentials( string $platform_slug, array $required_fields ): void {
+ if ( empty( $platform_slug ) || ! is_string( $platform_slug ) ) {
+ WP_CLI::error( 'Invalid platform slug provided.' );
+ return;
+ }
+
if ( empty( $required_fields ) ) {
WP_CLI::error( 'No credential fields specified for setup.' );
return;
}
+ // Validate required_fields structure
+ foreach ( $required_fields as $key => $prompt ) {
+ if ( ! is_string( $key ) || ! is_string( $prompt ) ) {
+ WP_CLI::error( 'Invalid credential field structure. Keys and prompts must be strings.' );
+ return;
+ }
+ }
+
- WP_CLI::log( 'Configuring credentials for ' . ucfirst( $platform_slug ) . '...' );
+ WP_CLI::log( 'Configuring credentials for ' . ucfirst( sanitize_text_field( $platform_slug ) ) . '...' );🤖 Prompt for AI Agents
In plugins/woocommerce/src/Internal/CLI/Migrator/Core/CredentialManager.php
around lines 90 to 100, the setup_credentials method lacks validation for the
platform_slug and required_fields parameters. Add checks to ensure platform_slug
is a non-empty string and required_fields is a non-empty array with expected
structure before proceeding. If validation fails, use WP_CLI::error to report
the issue and halt execution. This will prevent unexpected inputs and improve
security.
| if ( isset( $args['ids'] ) ) { | ||
| \WP_CLI::debug( 'Calculating total count based on provided product IDs.' ); | ||
| $ids = is_array( $args['ids'] ) ? $args['ids'] : explode( ',', $args['ids'] ); | ||
| return count( array_filter( $ids ) ); // Filter out empty values. | ||
| } |
There was a problem hiding this comment.
Validate and sanitize ID input to prevent injection attacks.
The IDs should be validated to ensure they're numeric and sanitized before processing to prevent potential security issues.
// Handle special case: if specific IDs are provided, count them directly.
if ( isset( $args['ids'] ) ) {
\WP_CLI::debug( 'Calculating total count based on provided product IDs.' );
$ids = is_array( $args['ids'] ) ? $args['ids'] : explode( ',', $args['ids'] );
- return count( array_filter( $ids ) ); // Filter out empty values.
+ // Validate and sanitize IDs to ensure they're numeric
+ $valid_ids = array_filter( $ids, function( $id ) {
+ return is_numeric( trim( $id ) ) && (int) $id > 0;
+ } );
+ return count( $valid_ids );
}🤖 Prompt for AI Agents
In
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.php
around lines 75 to 79, the code currently processes product IDs without
validation or sanitization, which can lead to security risks. Update the code to
validate that each ID is numeric and sanitize the input by filtering out any
non-numeric or invalid values before counting them. This ensures only safe,
expected data is processed and helps prevent injection attacks.
| private function build_count_query_params( array $args ): array { | ||
| $query_params = array(); | ||
|
|
||
| // Map standard filter args to Shopify REST count query params. | ||
| if ( isset( $args['status'] ) ) { | ||
| $query_params['status'] = strtolower( $args['status'] ); // REST uses lowercase. | ||
| } | ||
|
|
||
| if ( isset( $args['created_at_min'] ) ) { | ||
| $query_params['created_at_min'] = $args['created_at_min']; | ||
| } | ||
|
|
||
| if ( isset( $args['created_at_max'] ) ) { | ||
| $query_params['created_at_max'] = $args['created_at_max']; | ||
| } | ||
|
|
||
| if ( isset( $args['updated_at_min'] ) ) { | ||
| $query_params['updated_at_min'] = $args['updated_at_min']; | ||
| } | ||
|
|
||
| if ( isset( $args['updated_at_max'] ) ) { | ||
| $query_params['updated_at_max'] = $args['updated_at_max']; | ||
| } | ||
|
|
||
| if ( isset( $args['vendor'] ) ) { | ||
| $query_params['vendor'] = $args['vendor']; | ||
| } | ||
|
|
||
| if ( isset( $args['product_type'] ) ) { | ||
| $query_params['product_type'] = $args['product_type']; | ||
| } | ||
|
|
||
| return $query_params; | ||
| } |
There was a problem hiding this comment.
Sanitize query parameter values to prevent injection attacks.
The query parameters should be sanitized before being passed to the API client to prevent potential injection attacks or malformed requests.
private function build_count_query_params( array $args ): array {
$query_params = array();
// Map standard filter args to Shopify REST count query params.
if ( isset( $args['status'] ) ) {
- $query_params['status'] = strtolower( $args['status'] ); // REST uses lowercase.
+ $query_params['status'] = strtolower( sanitize_text_field( $args['status'] ) );
}
if ( isset( $args['created_at_min'] ) ) {
- $query_params['created_at_min'] = $args['created_at_min'];
+ $query_params['created_at_min'] = sanitize_text_field( $args['created_at_min'] );
}
if ( isset( $args['created_at_max'] ) ) {
- $query_params['created_at_max'] = $args['created_at_max'];
+ $query_params['created_at_max'] = sanitize_text_field( $args['created_at_max'] );
}
if ( isset( $args['updated_at_min'] ) ) {
- $query_params['updated_at_min'] = $args['updated_at_min'];
+ $query_params['updated_at_min'] = sanitize_text_field( $args['updated_at_min'] );
}
if ( isset( $args['updated_at_max'] ) ) {
- $query_params['updated_at_max'] = $args['updated_at_max'];
+ $query_params['updated_at_max'] = sanitize_text_field( $args['updated_at_max'] );
}
if ( isset( $args['vendor'] ) ) {
- $query_params['vendor'] = $args['vendor'];
+ $query_params['vendor'] = sanitize_text_field( $args['vendor'] );
}
if ( isset( $args['product_type'] ) ) {
- $query_params['product_type'] = $args['product_type'];
+ $query_params['product_type'] = sanitize_text_field( $args['product_type'] );
}
return $query_params;
}📝 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.
| private function build_count_query_params( array $args ): array { | |
| $query_params = array(); | |
| // Map standard filter args to Shopify REST count query params. | |
| if ( isset( $args['status'] ) ) { | |
| $query_params['status'] = strtolower( $args['status'] ); // REST uses lowercase. | |
| } | |
| if ( isset( $args['created_at_min'] ) ) { | |
| $query_params['created_at_min'] = $args['created_at_min']; | |
| } | |
| if ( isset( $args['created_at_max'] ) ) { | |
| $query_params['created_at_max'] = $args['created_at_max']; | |
| } | |
| if ( isset( $args['updated_at_min'] ) ) { | |
| $query_params['updated_at_min'] = $args['updated_at_min']; | |
| } | |
| if ( isset( $args['updated_at_max'] ) ) { | |
| $query_params['updated_at_max'] = $args['updated_at_max']; | |
| } | |
| if ( isset( $args['vendor'] ) ) { | |
| $query_params['vendor'] = $args['vendor']; | |
| } | |
| if ( isset( $args['product_type'] ) ) { | |
| $query_params['product_type'] = $args['product_type']; | |
| } | |
| return $query_params; | |
| } | |
| private function build_count_query_params( array $args ): array { | |
| $query_params = array(); | |
| // Map standard filter args to Shopify REST count query params. | |
| if ( isset( $args['status'] ) ) { | |
| $query_params['status'] = strtolower( sanitize_text_field( $args['status'] ) ); | |
| } | |
| if ( isset( $args['created_at_min'] ) ) { | |
| $query_params['created_at_min'] = sanitize_text_field( $args['created_at_min'] ); | |
| } | |
| if ( isset( $args['created_at_max'] ) ) { | |
| $query_params['created_at_max'] = sanitize_text_field( $args['created_at_max'] ); | |
| } | |
| if ( isset( $args['updated_at_min'] ) ) { | |
| $query_params['updated_at_min'] = sanitize_text_field( $args['updated_at_min'] ); | |
| } | |
| if ( isset( $args['updated_at_max'] ) ) { | |
| $query_params['updated_at_max'] = sanitize_text_field( $args['updated_at_max'] ); | |
| } | |
| if ( isset( $args['vendor'] ) ) { | |
| $query_params['vendor'] = sanitize_text_field( $args['vendor'] ); | |
| } | |
| if ( isset( $args['product_type'] ) ) { | |
| $query_params['product_type'] = sanitize_text_field( $args['product_type'] ); | |
| } | |
| return $query_params; | |
| } |
🤖 Prompt for AI Agents
In
plugins/woocommerce/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.php
between lines 105 and 138, the query parameter values are assigned directly from
the input array without sanitization, which can lead to injection attacks or
malformed requests. To fix this, sanitize each parameter value before assigning
it to the $query_params array, for example by trimming whitespace and escaping
or validating the input according to expected formats, ensuring all values are
safe before use in API calls.
| public function test_dependency_injection_properties() { | ||
| $this->command->init( $this->credential_manager, $this->platform_registry ); | ||
|
|
||
| $this->assertTrue( true ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify that dependencies are actually injected.
The test doesn't verify that the dependencies were properly stored in the command instance. Consider adding assertions to check the internal state or testing behavior that depends on the injected dependencies.
public function test_dependency_injection_properties() {
$this->command->init( $this->credential_manager, $this->platform_registry );
- $this->assertTrue( true );
+ // Test that the command can use the injected dependencies
+ // by calling a method that requires them (if available)
+ // or use reflection to verify the dependencies are stored
+ $reflection = new \ReflectionClass( $this->command );
+ $credential_manager_prop = $reflection->getProperty( 'credential_manager' );
+ $credential_manager_prop->setAccessible( true );
+ $this->assertSame( $this->credential_manager, $credential_manager_prop->getValue( $this->command ) );
}📝 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.
| public function test_dependency_injection_properties() { | |
| $this->command->init( $this->credential_manager, $this->platform_registry ); | |
| $this->assertTrue( true ); | |
| } | |
| public function test_dependency_injection_properties() { | |
| $this->command->init( $this->credential_manager, $this->platform_registry ); | |
| // Test that the command can use the injected dependencies | |
| // by calling a method that requires them (if available) | |
| // or use reflection to verify the dependencies are stored | |
| $reflection = new \ReflectionClass( $this->command ); | |
| $credential_manager_prop = $reflection->getProperty( 'credential_manager' ); | |
| $credential_manager_prop->setAccessible( true ); | |
| $this->assertSame( $this->credential_manager, $credential_manager_prop->getValue( $this->command ) ); | |
| } |
🤖 Prompt for AI Agents
In
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ProductsCommandTest.php
around lines 86 to 90, the test method test_dependency_injection_properties does
not verify that the dependencies are actually injected into the command
instance. To fix this, add assertions that check whether the command's internal
properties or state reflect the injected $credential_manager and
$platform_registry after calling init. This can be done by accessing the
relevant properties or methods on the command instance that expose these
dependencies and asserting they match the injected objects.
| try { | ||
| $this->command->init( $this->credential_manager, $this->platform_registry ); | ||
| $this->assertTrue( true ); | ||
| } catch ( \Exception $e ) { | ||
| $this->fail( 'init method should not throw exceptions: ' . $e->getMessage() ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test meaningfulness with better assertions.
The current test only verifies that init() doesn't throw an exception, but doesn't verify that dependencies are properly injected.
// Test that init method can be called without errors.
try {
$this->command->init( $this->credential_manager, $this->platform_registry );
- $this->assertTrue( true );
+ // Verify initialization was successful by checking command readiness
+ $this->assertTrue( is_callable( array( $this->command, '__invoke' ) ) );
} catch ( \Exception $e ) {
$this->fail( 'init method should not throw exceptions: ' . $e->getMessage() );
}🤖 Prompt for AI Agents
In
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ResetCommandTest.php
around lines 67 to 72, the test only checks that the init() method does not
throw exceptions but does not verify if dependencies are correctly injected.
Enhance the test by adding assertions that confirm the internal state or
properties of the command reflect the injected credential_manager and
platform_registry after init() is called, ensuring dependencies are properly
set.
| public function test_dependency_injection_properties() { | ||
| $this->command->init( $this->credential_manager, $this->platform_registry ); | ||
| $this->assertTrue( true ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add functional test for reset command behavior.
Consider adding a test that verifies the actual reset functionality, including interaction with CredentialManager and PlatformRegistry.
public function test_dependency_injection_properties() {
- $this->command->init( $this->credential_manager, $this->platform_registry );
- $this->assertTrue( true );
+ // Mock the dependencies to verify interaction
+ $this->platform_registry->expects( $this->once() )
+ ->method( 'resolve_platform' )
+ ->willReturn( 'shopify' );
+
+ $this->credential_manager->expects( $this->once() )
+ ->method( 'has_credentials' )
+ ->with( 'shopify' )
+ ->willReturn( true );
+
+ $this->credential_manager->expects( $this->once() )
+ ->method( 'delete_credentials' )
+ ->with( 'shopify' );
+
+ $this->command->init( $this->credential_manager, $this->platform_registry );
+ $this->command->__invoke( array(), array() );
}📝 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.
| public function test_dependency_injection_properties() { | |
| $this->command->init( $this->credential_manager, $this->platform_registry ); | |
| $this->assertTrue( true ); | |
| } | |
| public function test_dependency_injection_properties() { | |
| // Mock the dependencies to verify interaction | |
| $this->platform_registry->expects( $this->once() ) | |
| ->method( 'resolve_platform' ) | |
| ->willReturn( 'shopify' ); | |
| $this->credential_manager->expects( $this->once() ) | |
| ->method( 'has_credentials' ) | |
| ->with( 'shopify' ) | |
| ->willReturn( true ); | |
| $this->credential_manager->expects( $this->once() ) | |
| ->method( 'delete_credentials' ) | |
| ->with( 'shopify' ); | |
| $this->command->init( $this->credential_manager, $this->platform_registry ); | |
| $this->command->__invoke( array(), array() ); | |
| } |
🤖 Prompt for AI Agents
In
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/ResetCommandTest.php
around lines 86 to 89, the current test only checks dependency injection but
does not verify the reset command's actual behavior. Add a functional test that
calls the reset command's main method or equivalent, then assert that it
correctly interacts with CredentialManager and PlatformRegistry, such as
clearing credentials or resetting platform states, to ensure the reset
functionality works as expected.
| try { | ||
| $this->command->init( $this->credential_manager, $this->platform_registry ); | ||
| $this->assertTrue( true ); | ||
| } catch ( \Exception $e ) { | ||
| $this->fail( 'init method should not throw exceptions: ' . $e->getMessage() ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make dependency injection test more meaningful.
The current test just asserts true after calling init(). Consider verifying that the dependencies are actually injected by checking the command's state or behavior.
// Test that init method can be called without errors.
try {
$this->command->init( $this->credential_manager, $this->platform_registry );
- $this->assertTrue( true );
+ // Verify that dependencies were actually injected by testing command behavior
+ $this->assertTrue( method_exists( $this->command, '__invoke' ) );
} catch ( \Exception $e ) {
$this->fail( 'init method should not throw exceptions: ' . $e->getMessage() );
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/SetupCommandTest.php
around lines 67 to 72, the test currently asserts true after calling the init()
method, which does not verify if dependencies were properly injected. Modify the
test to check the command's internal state or behavior that reflects the
injected dependencies, such as asserting that properties related to
credential_manager and platform_registry are set or that methods relying on
these dependencies behave as expected.
| public function test_dependency_injection_properties() { | ||
| $this->command->init( $this->credential_manager, $this->platform_registry ); | ||
| $this->assertTrue( true ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add meaningful assertions for dependency injection verification.
This test method doesn't verify that dependencies are properly injected. Consider adding assertions that check the command's internal state or behavior after initialization.
public function test_dependency_injection_properties() {
$this->command->init( $this->credential_manager, $this->platform_registry );
- $this->assertTrue( true );
+
+ // Since properties are private, we can test that the command can be invoked
+ // which would fail if dependencies weren't properly injected
+ $this->assertTrue( is_callable( array( $this->command, '__invoke' ) ) );
+
+ // Or add a getter method to SetupCommand for testing purposes
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Commands/SetupCommandTest.php
around lines 86 to 89, the test method test_dependency_injection_properties
currently only asserts true without verifying that dependencies are properly
injected. Update this test to include assertions that confirm the command's
internal properties or state reflect the injected $credential_manager and
$platform_registry after calling init. This could involve checking that these
properties are set correctly or that methods relying on these dependencies
behave as expected.
| public static $last_debug_message = ''; | ||
|
|
||
| /** | ||
| * Last warning message logged. | ||
| * | ||
| * @var string | ||
| */ | ||
| public static $last_warning_message = ''; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add message history and reset functionality for better test isolation.
The current implementation only stores the last message, which can cause test interference when multiple CLI operations occur. Consider adding message history and a reset method.
class MockWPCLI {
- /**
- * Last debug message logged.
- *
- * @var string
- */
- public static $last_debug_message = '';
-
- /**
- * Last warning message logged.
- *
- * @var string
- */
- public static $last_warning_message = '';
+ /**
+ * All debug messages logged.
+ *
+ * @var array
+ */
+ public static $debug_messages = array();
+
+ /**
+ * All warning messages logged.
+ *
+ * @var array
+ */
+ public static $warning_messages = array();
+
+ /**
+ * Reset all logged messages.
+ */
+ public static function reset(): void {
+ self::$debug_messages = array();
+ self::$warning_messages = array();
+ }
+
+ /**
+ * Get the last debug message.
+ *
+ * @return string
+ */
+ public static function get_last_debug_message(): string {
+ return end( self::$debug_messages ) ?: '';
+ }
+
+ /**
+ * Get the last warning message.
+ *
+ * @return string
+ */
+ public static function get_last_warning_message(): string {
+ return end( self::$warning_messages ) ?: '';
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugins/woocommerce/tests/php/src/Internal/CLI/Migrator/Mocks/MockWPCLI.php
around lines 21 to 28, the static properties only store the last debug and
warning messages, which can cause interference between tests. Add arrays to keep
a history of all debug and warning messages logged, and implement a reset method
to clear these histories before each test to ensure better test isolation.
|
@xristos3490 I've made the changes, let me know how it looks. |
There was a problem hiding this comment.
Overall, looking good and tests as expected!
Regarding the architecture, progress is going very well; I just wanted to point out an area for improvement I identified that could help clarify things a bit more.
Currently, the Platform registry includes information for credentials. This gives the PlatformRegistry a small, secondary responsibility: knowing the configuration details of each platform.
Instead of:
// in Core/PlatformRegistry.php
public function get_platform_credential_fields( string $platform_slug ): array {
$default_fields = array(
'shopify' => array(
'shop_url' => 'Enter shop URL...',
'access_token' => 'Enter access token:',
),
);
// ...
}We could move this configuration into the platform's registration:
// in Platforms/Shopify/ShopifyPlatform.php
public static function register_platform( array $platforms ): array {
$platforms['shopify'] = array(
'name' => 'Shopify',
'fetcher' => ShopifyFetcher::class,
'mapper' => ShopifyMapper::class,
'credentials' => array( // New key
'shop_url' => 'Enter shop URL (e.g., mystore.myshopify.com):',
'access_token' => 'Enter access token:',
),
);
return $platforms;
}
// in Core/PlatformRegistry.php
public function get_platform_credential_fields( string $platform_slug ): array {
$platform = $this->get_platform( $platform_slug );
return $platform['credentials'] ?? array();
}Feel free to address this in future PRs or leave it as debt if time doesn't allow for slight refactors.
Left a minor cleanup suggestion on the Mocks, but pre-approving! Cheers!
| /** | ||
| * A mock fetcher class for testing purposes. | ||
| */ | ||
| class MockPlatformFetcher implements PlatformFetcherInterface { |
There was a problem hiding this comment.
Where are these used? Same for MockPlatformMapper? Should we clean up?
There was a problem hiding this comment.
Where are these used? Same for MockPlatformMapper? Should we clean up?
Sorry, I missed this comment. These were used as part of #59941, but not anymore. I'll clean it up in seperate PR. Thanks for the catch.
Summary
This adds the ability to fetch total product counts from Shopify stores using the REST API, which is crucial for progress indication during migrations.
What's Changed
1. ShopifyClient - REST API Communication Layer
src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClient.php2. Enhanced ShopifyFetcher - Real Product Count Implementation
src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcher.phpfetch_total_count()method using Shopify's/products/count.jsonendpointShopifyClient🔧 Enhanced Components
3. Fixed Dependency Injection in PlatformRegistry
src/Internal/CLI/Migrator/Core/PlatformRegistry.phpget_fetcher()andget_mapper()to use WooCommerce's DI container instead of direct instantiation4. Enhanced ProductsCommand with Count Feature
src/Internal/CLI/Migrator/Commands/ProductsCommand.php--countflag and status filtering supportwp wc migrate products --count- Get total product countwp wc migrate products --count --status=active- Get filtered countTests
5. ShopifyClient Tests
tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyClientTest.php6. ShopifyFetcher Tests
tests/php/src/Internal/CLI/Migrator/Platforms/Shopify/ShopifyFetcherTest.phpAPI Usage Examples
Testing Instructions:
wp wc migrate setup --platform=shopifywp wc migrate products --countwp wc migrate products --count --status=active