From 91ce41a4860f62af997096d44bbfdcb56af46597 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 29 Jul 2026 09:42:51 +0600 Subject: [PATCH 1/2] refactor(di): register option loader/sync and manager cost recalculator Wire OptionSyncService, OptionLoaderService, and ManagerOrderCostRecalculator through ServiceRegistry so ms3.services.php overrides work. OptionService receives loader/sync from the container; OrdersController resolves the cost recalculator via DI. Add ServiceRegistryDiTest smoke coverage. Closes #363 --- .../minishop3/config/ms3.services.example.php | 10 ++- .../Api/Manager/OrdersController.php | 5 +- .../minishop3/src/ServiceRegistry.php | 24 ++++++ .../src/Services/Option/OptionService.php | 8 +- .../minishop3/tests/ServiceRegistryDiTest.php | 83 +++++++++++++++++++ 5 files changed, 121 insertions(+), 9 deletions(-) create mode 100644 core/components/minishop3/tests/ServiceRegistryDiTest.php diff --git a/core/components/minishop3/config/ms3.services.example.php b/core/components/minishop3/config/ms3.services.example.php index 71e402ff..c07d7275 100644 --- a/core/components/minishop3/config/ms3.services.example.php +++ b/core/components/minishop3/config/ms3.services.example.php @@ -193,9 +193,15 @@ * 'ms3_category_service' - Category operations * 'ms3_category_option_service' - Category options * - * Product Options: + * Product Options (override via ms3.services.php / ms3.services.d/): * -------------- - * 'ms3_option_service' - EAV options system + * 'ms3_option_service' - EAV options facade + * 'ms3_option_loader' - load option values / admin fields + * 'ms3_option_sync' - save/sync product option values + * + * Order manager cost: + * ------------------- + * 'ms3_manager_order_cost_recalculator' - manager order totals recalc * * Utilities: * -------- diff --git a/core/components/minishop3/src/Controllers/Api/Manager/OrdersController.php b/core/components/minishop3/src/Controllers/Api/Manager/OrdersController.php index b91ace53..dc27a3e1 100644 --- a/core/components/minishop3/src/Controllers/Api/Manager/OrdersController.php +++ b/core/components/minishop3/src/Controllers/Api/Manager/OrdersController.php @@ -330,9 +330,6 @@ public function recalculateCost(array $params = []): array return Response::error('Order not found', HttpStatus::NOT_FOUND)->getData(); } - /** @var MiniShop3 $ms3 */ - $ms3 = $this->modx->services->get('ms3'); - $modeIn = strtolower(trim((string)($params['mode'] ?? ManagerOrderCostRecalculator::MODE_AUTO))); $allowedModes = [ ManagerOrderCostRecalculator::MODE_AUTO, @@ -348,7 +345,7 @@ public function recalculateCost(array $params = []): array $options['manual_delivery_cost'] = $params['manual_delivery_cost']; } - $recalculator = new ManagerOrderCostRecalculator($this->modx, $ms3); + $recalculator = $this->modx->services->get('ms3_manager_order_cost_recalculator'); $result = $recalculator->recalculate($order, $options); if (empty($result['success'])) { diff --git a/core/components/minishop3/src/ServiceRegistry.php b/core/components/minishop3/src/ServiceRegistry.php index 04980615..4b4a6f1d 100644 --- a/core/components/minishop3/src/ServiceRegistry.php +++ b/core/components/minishop3/src/ServiceRegistry.php @@ -93,6 +93,10 @@ class ServiceRegistry 'class' => \MiniShop3\Services\Order\OrderCostCalculator::class, 'interface' => null, ], + 'ms3_manager_order_cost_recalculator' => [ + 'class' => \MiniShop3\Services\Order\ManagerOrderCostRecalculator::class, + 'interface' => null, + ], 'ms3_order_field_manager' => [ 'class' => \MiniShop3\Services\Order\OrderFieldManager::class, 'interface' => null, @@ -150,6 +154,14 @@ class ServiceRegistry 'class' => \MiniShop3\Services\Option\OptionService::class, 'interface' => null, ], + 'ms3_option_loader' => [ + 'class' => \MiniShop3\Services\Option\OptionLoaderService::class, + 'interface' => null, + ], + 'ms3_option_sync' => [ + 'class' => \MiniShop3\Services\Option\OptionSyncService::class, + 'interface' => null, + ], 'ms3_cart' => [ 'class' => \MiniShop3\Controllers\Cart\Cart::class, 'interface' => null, @@ -453,6 +465,7 @@ protected function registerService(string $serviceKey, array $config): bool $servicesWithModxAndMs3 = [ 'ms3_order_draft_manager', 'ms3_order_cost_calculator', + 'ms3_manager_order_cost_recalculator', 'ms3_order_user_resolver', 'ms3_order_log', 'ms3_cart_item_manager', @@ -461,6 +474,7 @@ protected function registerService(string $serviceKey, array $config): bool // Services with complex dependencies (resolved via DI) $servicesWithDependencies = [ + 'ms3_option_service', 'ms3_order_field_manager', 'ms3_order_address_manager', 'ms3_order_submit_handler', @@ -505,6 +519,16 @@ protected function registerServiceWithDependencies(string $serviceKey, string $v $modx = $this->modx; switch ($serviceKey) { + case 'ms3_option_service': + // OptionService(xPDO, OptionLoaderService, OptionSyncService) + $this->modx->services->add($serviceKey, function () use ($validatedClass, $modx) { + $loader = $modx->services->get('ms3_option_loader'); + $sync = $modx->services->get('ms3_option_sync'); + + return new $validatedClass($modx, $loader, $sync); + }); + break; + case 'ms3_order_field_manager': // OrderFieldManager(modX, MiniShop3, OrderDraftManager) $this->modx->services->add($serviceKey, function () use ($validatedClass, $modx) { diff --git a/core/components/minishop3/src/Services/Option/OptionService.php b/core/components/minishop3/src/Services/Option/OptionService.php index 501416a9..c27f59ba 100644 --- a/core/components/minishop3/src/Services/Option/OptionService.php +++ b/core/components/minishop3/src/Services/Option/OptionService.php @@ -34,12 +34,14 @@ class OptionService /** * @param xPDO $xpdo + * @param OptionLoaderService $loader + * @param OptionSyncService $sync */ - public function __construct(xPDO $xpdo) + public function __construct(xPDO $xpdo, OptionLoaderService $loader, OptionSyncService $sync) { $this->xpdo = $xpdo; - $this->loader = new OptionLoaderService($xpdo); - $this->sync = new OptionSyncService($xpdo); + $this->loader = $loader; + $this->sync = $sync; $this->category = new OptionCategoryService($xpdo); } diff --git a/core/components/minishop3/tests/ServiceRegistryDiTest.php b/core/components/minishop3/tests/ServiceRegistryDiTest.php new file mode 100644 index 00000000..e02ca9e5 --- /dev/null +++ b/core/components/minishop3/tests/ServiceRegistryDiTest.php @@ -0,0 +1,83 @@ +get('ms3_manager_order_cost_recalculator')")) { + $fail('OrdersController must resolve manager cost recalculator from DI'); +} + +$example = file_get_contents(__DIR__ . '/../config/ms3.services.example.php'); +if ($example === false) { + $fail('unable to read ms3.services.example.php'); +} +foreach (['ms3_option_loader', 'ms3_option_sync', 'ms3_manager_order_cost_recalculator'] as $key) { + if (!str_contains($example, "'{$key}'")) { + $fail("ms3.services.example.php must document {$key} override"); + } +} + +fwrite(STDOUT, "OK ServiceRegistryDiTest\n"); +exit(0); From 190a51220d79afa7869cef96247dcfb6f53dcc71 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 29 Jul 2026 23:18:44 +0600 Subject: [PATCH 2/2] refactor(di): inject OptionCategoryService into OptionService Address #363 review findings: - OptionService now receives OptionCategoryService via its constructor instead of instantiating it internally; ServiceRegistry resolves ms3_category_option_service from DI for the ms3_option_service factory - Extract ServiceRegistry factory maps (CONTROLLERS_WITH_MS3_ONLY, SERVICES_WITH_MODX_AND_MS3, SERVICES_WITH_DEPENDENCIES) and a new SERVICE_DEPENDENCIES map into public constants so they are inspectable - ServiceRegistryDiTest now asserts behaviorally that every factory-map key and every declared dependency is a registered service key, and that ms3_option_service depends on ms3_category_option_service --- .../minishop3/src/ServiceRegistry.php | 95 +++++++++++++------ .../src/Services/Option/OptionService.php | 11 ++- .../minishop3/tests/ServiceRegistryDiTest.php | 87 +++++++++++++++-- 3 files changed, 153 insertions(+), 40 deletions(-) diff --git a/core/components/minishop3/src/ServiceRegistry.php b/core/components/minishop3/src/ServiceRegistry.php index 4b4a6f1d..f2607b57 100644 --- a/core/components/minishop3/src/ServiceRegistry.php +++ b/core/components/minishop3/src/ServiceRegistry.php @@ -35,6 +35,66 @@ class ServiceRegistry /** @var modX */ protected modX $modx; + /** + * Controllers requiring only a MiniShop3 instance: __construct(MiniShop3 $ms3). + */ + public const CONTROLLERS_WITH_MS3_ONLY = [ + 'ms3_cart', + 'ms3_order', + 'ms3_customer', + ]; + + /** + * Services requiring both modX and MiniShop3: __construct(modX $modx, MiniShop3 $ms3). + */ + public const SERVICES_WITH_MODX_AND_MS3 = [ + 'ms3_order_draft_manager', + 'ms3_order_cost_calculator', + 'ms3_manager_order_cost_recalculator', + 'ms3_order_user_resolver', + 'ms3_order_log', + 'ms3_cart_item_manager', + 'ms3_customer_address_manager', + ]; + + /** + * Services with complex dependencies resolved via DI + * (see {@see registerServiceWithDependencies()}). + */ + public const SERVICES_WITH_DEPENDENCIES = [ + 'ms3_option_service', + 'ms3_order_field_manager', + 'ms3_order_address_manager', + 'ms3_order_submit_handler', + 'ms3_order_status', + 'ms3_order_finalize', + ]; + + /** + * DI keys that {@see registerServiceWithDependencies()} resolves for each + * service. Exposed so tests can assert every referenced dependency is a + * registered service key (#363). + */ + public const SERVICE_DEPENDENCIES = [ + 'ms3_option_service' => [ + 'ms3_option_loader', + 'ms3_option_sync', + 'ms3_category_option_service', + ], + 'ms3_order_field_manager' => ['ms3_order_draft_manager'], + 'ms3_order_address_manager' => ['ms3_order_draft_manager', 'ms3_order_field_manager'], + 'ms3_order_submit_handler' => [ + 'ms3_order_draft_manager', + 'ms3_order_cost_calculator', + 'ms3_order_field_manager', + 'ms3_order_address_manager', + 'ms3_order_user_resolver', + 'ms3_order_number_generator', + ], + 'ms3_order_finalize' => ['ms3_order_number_generator'], + 'ms3_order_status' => ['ms3_order_log'], + ]; + /** * Default services (built into component) * @@ -458,41 +518,17 @@ protected function registerService(string $serviceKey, array $config): bool $modx = $this->modx; - // Controllers requiring only MiniShop3 instance: __construct(MiniShop3 $ms3) - $controllersWithMs3Only = ['ms3_cart', 'ms3_order', 'ms3_customer']; - - // Services requiring both modX and MiniShop3: __construct(modX $modx, MiniShop3 $ms3) - $servicesWithModxAndMs3 = [ - 'ms3_order_draft_manager', - 'ms3_order_cost_calculator', - 'ms3_manager_order_cost_recalculator', - 'ms3_order_user_resolver', - 'ms3_order_log', - 'ms3_cart_item_manager', - 'ms3_customer_address_manager', - ]; - - // Services with complex dependencies (resolved via DI) - $servicesWithDependencies = [ - 'ms3_option_service', - 'ms3_order_field_manager', - 'ms3_order_address_manager', - 'ms3_order_submit_handler', - 'ms3_order_status', - 'ms3_order_finalize', - ]; - - if (in_array($serviceKey, $controllersWithMs3Only)) { + if (in_array($serviceKey, self::CONTROLLERS_WITH_MS3_ONLY, true)) { $this->modx->services->add($serviceKey, function () use ($validatedClass, $modx) { $ms3 = $modx->getService('MiniShop3', \MiniShop3\MiniShop3::class); return new $validatedClass($ms3); }); - } elseif (in_array($serviceKey, $servicesWithModxAndMs3)) { + } elseif (in_array($serviceKey, self::SERVICES_WITH_MODX_AND_MS3, true)) { $this->modx->services->add($serviceKey, function () use ($validatedClass, $modx) { $ms3 = $modx->getService('MiniShop3', \MiniShop3\MiniShop3::class); return new $validatedClass($modx, $ms3); }); - } elseif (in_array($serviceKey, $servicesWithDependencies)) { + } elseif (in_array($serviceKey, self::SERVICES_WITH_DEPENDENCIES, true)) { // Services with dependencies - resolve them from DI $this->registerServiceWithDependencies($serviceKey, $validatedClass); } else { @@ -520,12 +556,13 @@ protected function registerServiceWithDependencies(string $serviceKey, string $v switch ($serviceKey) { case 'ms3_option_service': - // OptionService(xPDO, OptionLoaderService, OptionSyncService) + // OptionService(xPDO, OptionLoaderService, OptionSyncService, OptionCategoryService) $this->modx->services->add($serviceKey, function () use ($validatedClass, $modx) { $loader = $modx->services->get('ms3_option_loader'); $sync = $modx->services->get('ms3_option_sync'); + $category = $modx->services->get('ms3_category_option_service'); - return new $validatedClass($modx, $loader, $sync); + return new $validatedClass($modx, $loader, $sync, $category); }); break; diff --git a/core/components/minishop3/src/Services/Option/OptionService.php b/core/components/minishop3/src/Services/Option/OptionService.php index c27f59ba..705c26aa 100644 --- a/core/components/minishop3/src/Services/Option/OptionService.php +++ b/core/components/minishop3/src/Services/Option/OptionService.php @@ -36,13 +36,18 @@ class OptionService * @param xPDO $xpdo * @param OptionLoaderService $loader * @param OptionSyncService $sync + * @param OptionCategoryService $category */ - public function __construct(xPDO $xpdo, OptionLoaderService $loader, OptionSyncService $sync) - { + public function __construct( + xPDO $xpdo, + OptionLoaderService $loader, + OptionSyncService $sync, + OptionCategoryService $category + ) { $this->xpdo = $xpdo; $this->loader = $loader; $this->sync = $sync; - $this->category = new OptionCategoryService($xpdo); + $this->category = $category; } // ========== LOADING OPERATIONS ========== diff --git a/core/components/minishop3/tests/ServiceRegistryDiTest.php b/core/components/minishop3/tests/ServiceRegistryDiTest.php index e02ca9e5..41934c54 100644 --- a/core/components/minishop3/tests/ServiceRegistryDiTest.php +++ b/core/components/minishop3/tests/ServiceRegistryDiTest.php @@ -1,13 +1,19 @@ get('ms3_category_option_service')")) { + $fail('registerServiceWithDependencies must resolve ms3_category_option_service for OptionService'); +} + +if (!str_contains($registrySrc, 'SERVICES_WITH_MODX_AND_MS3')) { + $fail('ServiceRegistry must declare SERVICES_WITH_MODX_AND_MS3 factory map'); } $optionServiceSrc = file_get_contents(__DIR__ . '/../src/Services/Option/OptionService.php'); @@ -54,8 +65,11 @@ if (preg_match('/new\s+OptionSyncService\s*\(/', $optionServiceSrc)) { $fail('OptionService must not instantiate OptionSyncService directly'); } -if (!str_contains($optionServiceSrc, 'OptionLoaderService $loader, OptionSyncService $sync')) { - $fail('OptionService constructor must accept loader/sync from DI'); +if (preg_match('/new\s+OptionCategoryService\s*\(/', $optionServiceSrc)) { + $fail('OptionService must not instantiate OptionCategoryService directly'); +} +if (!preg_match('/OptionLoaderService\s+\$loader,\s*OptionSyncService\s+\$sync,\s*OptionCategoryService\s+\$category/s', $optionServiceSrc)) { + $fail('OptionService constructor must accept loader/sync/category from DI'); } $ordersCtrlSrc = file_get_contents(__DIR__ . '/../src/Controllers/Api/Manager/OrdersController.php'); @@ -73,11 +87,68 @@ if ($example === false) { $fail('unable to read ms3.services.example.php'); } -foreach (['ms3_option_loader', 'ms3_option_sync', 'ms3_manager_order_cost_recalculator'] as $key) { +foreach ([ + 'ms3_option_loader', + 'ms3_option_sync', + 'ms3_category_option_service', + 'ms3_manager_order_cost_recalculator', +] as $key) { if (!str_contains($example, "'{$key}'")) { $fail("ms3.services.example.php must document {$key} override"); } } +// Behavior test: every factory-map key and every declared dependency must be +// a registered service key, so the DI wiring can actually resolve at runtime +// (#363 review: verify factory map keys exist). +$modx = new modX(); +$registry = new class ($modx) extends ServiceRegistry { + protected function loadCustomServices(): void + { + // skip filesystem config loading in unit test + } +}; +$registered = $registry->getRegisteredServices(); +$registeredSet = array_flip($registered); + +$assertRegistered = static function (string $key, string $map) use ($registeredSet, $fail): void { + if (!isset($registeredSet[$key])) { + $fail("{$map} references unregistered service key: {$key}"); + } +}; + +foreach (ServiceRegistry::CONTROLLERS_WITH_MS3_ONLY as $key) { + $assertRegistered($key, 'CONTROLLERS_WITH_MS3_ONLY'); +} +foreach (ServiceRegistry::SERVICES_WITH_MODX_AND_MS3 as $key) { + $assertRegistered($key, 'SERVICES_WITH_MODX_AND_MS3'); + if ($key !== 'ms3_manager_order_cost_recalculator') { + continue; + } +} +if (!in_array('ms3_manager_order_cost_recalculator', ServiceRegistry::SERVICES_WITH_MODX_AND_MS3, true)) { + $fail('ms3_manager_order_cost_recalculator must use modX+MiniShop3 factory'); +} +foreach (ServiceRegistry::SERVICES_WITH_DEPENDENCIES as $key) { + $assertRegistered($key, 'SERVICES_WITH_DEPENDENCIES'); +} +foreach (ServiceRegistry::SERVICE_DEPENDENCIES as $service => $deps) { + $assertRegistered($service, 'SERVICE_DEPENDENCIES'); + foreach ($deps as $dep) { + $assertRegistered($dep, "SERVICE_DEPENDENCIES[{$service}]"); + } +} + +$optionDeps = ServiceRegistry::SERVICE_DEPENDENCIES['ms3_option_service'] ?? []; +if (!in_array('ms3_option_loader', $optionDeps, true)) { + $fail('ms3_option_service must depend on ms3_option_loader'); +} +if (!in_array('ms3_option_sync', $optionDeps, true)) { + $fail('ms3_option_service must depend on ms3_option_sync'); +} +if (!in_array('ms3_category_option_service', $optionDeps, true)) { + $fail('ms3_option_service must depend on ms3_category_option_service'); +} + fwrite(STDOUT, "OK ServiceRegistryDiTest\n"); exit(0);