From 5ed1e3756f5771eca7a4f3d67fba269a97b13c1c Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Mon, 17 Aug 2026 11:19:23 +0600 Subject: [PATCH 1/5] feat(order): lifecycle gate with ports, idempotency, and rollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make OrderStatusService the non-draft status gate: optional transition allow-list, NullOrderLifecyclePorts for #589–#591, idempotent retries, and status rollback when after-event or a port fails. --- _build/elements/settings.php | 5 + .../minishop3/lexicon/en/default.inc.php | 3 + .../minishop3/lexicon/en/setting.inc.php | 4 +- .../minishop3/lexicon/ru/default.inc.php | 3 + .../minishop3/lexicon/ru/setting.inc.php | 4 +- .../minishop3/src/ServiceRegistry.php | 25 +- .../src/ServiceRegistryFactories.php | 7 +- .../Order/ManagerOrderMutationService.php | 19 +- .../Order/NullOrderLifecyclePorts.php | 28 ++ .../Order/OrderLifecyclePortsInterface.php | 40 ++ .../src/Services/Order/OrderStatusService.php | 138 +++++-- .../Order/OrderStatusTransitionPolicy.php | 89 ++++ .../Order/OrderStatusServiceLifecycleTest.php | 380 ++++++++++++++++++ .../Order/OrderStatusTransitionPolicyTest.php | 57 +++ 14 files changed, 751 insertions(+), 51 deletions(-) create mode 100644 core/components/minishop3/src/Services/Order/NullOrderLifecyclePorts.php create mode 100644 core/components/minishop3/src/Services/Order/OrderLifecyclePortsInterface.php create mode 100644 core/components/minishop3/src/Services/Order/OrderStatusTransitionPolicy.php create mode 100644 core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php create mode 100644 core/components/minishop3/tests/Unit/Services/Order/OrderStatusTransitionPolicyTest.php diff --git a/_build/elements/settings.php b/_build/elements/settings.php index 282c79188..07aceb032 100644 --- a/_build/elements/settings.php +++ b/_build/elements/settings.php @@ -321,6 +321,11 @@ 'xtype' => 'numberfield', 'area' => 'ms3_statuses', ], + 'ms3_order_status_transitions' => [ + 'value' => '', + 'xtype' => 'textfield', + 'area' => 'ms3_statuses', + ], 'ms3_customer_cancel_allowed_statuses' => [ 'value' => '2,3', 'xtype' => 'textfield', diff --git a/core/components/minishop3/lexicon/en/default.inc.php b/core/components/minishop3/lexicon/en/default.inc.php index 30f266a0c..fd804d02a 100644 --- a/core/components/minishop3/lexicon/en/default.inc.php +++ b/core/components/minishop3/lexicon/en/default.inc.php @@ -214,6 +214,9 @@ $_lang['ms3_err_status_fixed'] = 'Fixed status is set. You cannot change it to earlier one.'; $_lang['ms3_err_status_wrong'] = 'Invalid order status.'; $_lang['ms3_err_status_same'] = 'This status is already set.'; +$_lang['ms3_err_status_transition'] = 'This status transition is not allowed.'; +$_lang['ms3_err_status_transitions_invalid'] = 'Order status transition allow-list is invalid.'; +$_lang['ms3_err_status_rollback'] = 'Failed to roll back order status after a rejected transition.'; $_lang['ms3_err_register_globals'] = 'Error: php parameter register_globals must be disabled.'; $_lang['ms3_err_link_equal'] = 'You are trying to add product link to itself'; $_lang['ms3_err_no_link'] = 'Link type not found'; diff --git a/core/components/minishop3/lexicon/en/setting.inc.php b/core/components/minishop3/lexicon/en/setting.inc.php index 316a5254b..e36d0db9e 100644 --- a/core/components/minishop3/lexicon/en/setting.inc.php +++ b/core/components/minishop3/lexicon/en/setting.inc.php @@ -142,13 +142,15 @@ $_lang['setting_ms3_status_canceled'] = 'Canceled order status ID'; $_lang['setting_ms3_status_canceled_desc'] = 'What status to set when canceling order'; $_lang['setting_ms3_status_sent'] = 'Sent order status ID'; -$_lang['setting_ms3_status_sent_desc'] = 'Order status to set when a shipment becomes shipped (if the transition is allowed).'; +$_lang['setting_ms3_status_sent_desc'] = 'Order status to set when a shipment becomes shipped (if the transition is allowed). Also used as the shipped status for order lifecycle ports (default seed: 4).'; $_lang['setting_ms3_shipment_enabled'] = 'Enable shipment lifecycle'; $_lang['setting_ms3_shipment_enabled_desc'] = 'Off (default): checkout and order statuses are unchanged. On: shipment shipped maps to ms3_status_sent via OrderStatusService, cancelled/failed maps to ms3_status_canceled. Create/setTracking still work when off. Webhook is 404 when off. Replace ms3_shipment_lifecycle to use an external WMS.'; $_lang['setting_ms3_shipment_on_delivered_status'] = 'Order status ID on delivered shipment'; $_lang['setting_ms3_shipment_on_delivered_status_desc'] = 'Optional. 0 (default) keeps order status unchanged when the shipment becomes delivered. Seed sent is final, so leave 0 unless you use a non-final sent status.'; $_lang['setting_ms3_shipment_on_in_transit_status'] = 'Order status ID on in-transit shipment'; $_lang['setting_ms3_shipment_on_in_transit_status_desc'] = 'Optional. 0 (default) keeps order status unchanged when the shipment becomes in_transit.'; +$_lang['setting_ms3_order_status_transitions'] = 'Allowed order status transitions'; +$_lang['setting_ms3_order_status_transitions_desc'] = 'Optional allow-list of status edges in addition to final/fixed rules. Empty = no matrix (default final/fixed only). Format: CSV pairs from:to (e.g. 2:3,3:4,2:5) or JSON [[2,3],[3,4]].'; $_lang['setting_ms3_customer_cancel_allowed_statuses'] = 'Statuses from which customer can cancel order'; $_lang['setting_ms3_customer_cancel_allowed_statuses_desc'] = 'Comma-separated status IDs. Default: New and Paid (2,3). Empty = use ms3_status_new and ms3_status_paid.'; $_lang['setting_ms3_status_for_stat'] = 'Status IDs for statistics'; diff --git a/core/components/minishop3/lexicon/ru/default.inc.php b/core/components/minishop3/lexicon/ru/default.inc.php index 67747b555..6315e09f5 100644 --- a/core/components/minishop3/lexicon/ru/default.inc.php +++ b/core/components/minishop3/lexicon/ru/default.inc.php @@ -214,6 +214,9 @@ $_lang['ms3_err_status_fixed'] = 'Установлен фиксирующий статус. Вы не можете сменить его на более ранний.'; $_lang['ms3_err_status_wrong'] = 'Неверный статус заказа.'; $_lang['ms3_err_status_same'] = 'Этот статус уже установлен.'; +$_lang['ms3_err_status_transition'] = 'Такой переход статуса не разрешён.'; +$_lang['ms3_err_status_transitions_invalid'] = 'Некорректный allow-list переходов статусов заказа.'; +$_lang['ms3_err_status_rollback'] = 'Не удалось откатить статус заказа после отклонённого перехода.'; $_lang['ms3_err_register_globals'] = 'Ошибка: php параметр register_globals должен быть выключен.'; $_lang['ms3_err_link_equal'] = 'Вы пытаетесь добавить товару ссылку на самого себя'; $_lang['ms3_err_no_link'] = 'Тип связи не найден'; diff --git a/core/components/minishop3/lexicon/ru/setting.inc.php b/core/components/minishop3/lexicon/ru/setting.inc.php index 9b2523f1e..dd8ef0c2d 100644 --- a/core/components/minishop3/lexicon/ru/setting.inc.php +++ b/core/components/minishop3/lexicon/ru/setting.inc.php @@ -142,13 +142,15 @@ $_lang['setting_ms3_status_canceled'] = 'ID статуса отмены заказа'; $_lang['setting_ms3_status_canceled_desc'] = 'Какой статус нужно устанавливать при отмене заказа'; $_lang['setting_ms3_status_sent'] = 'ID статуса «отправлен»'; -$_lang['setting_ms3_status_sent_desc'] = 'Статус заказа при переходе отгрузки в shipped, если переход разрешён.'; +$_lang['setting_ms3_status_sent_desc'] = 'Статус заказа при переходе отгрузки в shipped, если переход разрешён. Также используется как shipped-статус для order lifecycle ports (по умолчанию seed id 4).'; $_lang['setting_ms3_shipment_enabled'] = 'Включить lifecycle отгрузки'; $_lang['setting_ms3_shipment_enabled_desc'] = 'Выкл. (по умолчанию): оформление и статусы заказа как сейчас. Вкл.: shipped ставит ms3_status_sent через OrderStatusService, cancelled/failed — ms3_status_canceled. create/setTracking работают и при выкл. Webhook при выкл. отвечает 404. Внешний WMS подменяется через ms3_shipment_lifecycle.'; $_lang['setting_ms3_shipment_on_delivered_status'] = 'ID статуса заказа при delivered'; $_lang['setting_ms3_shipment_on_delivered_status_desc'] = 'Необязательно. 0 (по умолчанию) не меняет статус заказа, когда отгрузка становится delivered. Сид sent финальный, поэтому оставьте 0, если не используете нефинальный sent.'; $_lang['setting_ms3_shipment_on_in_transit_status'] = 'ID статуса заказа при in_transit'; $_lang['setting_ms3_shipment_on_in_transit_status_desc'] = 'Необязательно. 0 (по умолчанию) не меняет статус заказа, когда отгрузка становится in_transit.'; +$_lang['setting_ms3_order_status_transitions'] = 'Разрешённые переходы статусов заказа'; +$_lang['setting_ms3_order_status_transitions_desc'] = 'Опциональный allow-list рёбер поверх правил final/fixed. Пусто — только final/fixed. Формат: CSV пары from:to (например 2:3,3:4,2:5) или JSON [[2,3],[3,4]].'; $_lang['setting_ms3_customer_cancel_allowed_statuses'] = 'Статусы, из которых покупатель может отменить заказ'; $_lang['setting_ms3_customer_cancel_allowed_statuses_desc'] = 'ID статусов через запятую. По умолчанию: «Новый» и «Оплачен» (2,3). Пусто — использовать ms3_status_new и ms3_status_paid.'; $_lang['setting_ms3_status_for_stat'] = 'ID статусов для статистики'; diff --git a/core/components/minishop3/src/ServiceRegistry.php b/core/components/minishop3/src/ServiceRegistry.php index ba0763391..5e1c9ef7b 100644 --- a/core/components/minishop3/src/ServiceRegistry.php +++ b/core/components/minishop3/src/ServiceRegistry.php @@ -108,7 +108,7 @@ class ServiceRegistry 'ms3_order_number_generator', ], 'ms3_order_finalize' => ['ms3_order_number_generator'], - 'ms3_order_status' => ['ms3_order_log'], + 'ms3_order_status' => ['ms3_order_log', 'ms3_order_lifecycle_ports'], 'ms3_payment_lifecycle' => ['ms3_order_status'], 'ms3_cart_mutation_handler' => [ 'ms3_order_draft_manager', @@ -286,6 +286,10 @@ class ServiceRegistry 'class' => \MiniShop3\Services\Order\OrderLogService::class, 'interface' => null, ], + 'ms3_order_lifecycle_ports' => [ + 'class' => \MiniShop3\Services\Order\NullOrderLifecyclePorts::class, + 'interface' => \MiniShop3\Services\Order\OrderLifecyclePortsInterface::class, + ], 'ms3_order_status' => [ 'class' => \MiniShop3\Services\Order\OrderStatusService::class, 'interface' => null, @@ -722,19 +726,16 @@ protected function validateClass( return $fallbackClass; } - if ($requiredInterface) { - $interfaces = class_implements($className); - if (!in_array($requiredInterface, $interfaces ?: [])) { - $this->modx->log( - modX::LOG_LEVEL_ERROR, - "[MiniShop3 ServiceRegistry] Class '{$className}' must implement {$requiredInterface}, " - . 'using fallback' - ); - return $fallbackClass; - } + if ($requiredInterface && !is_a($className, $requiredInterface, true)) { + $this->modx->log( + modX::LOG_LEVEL_ERROR, + "[MiniShop3 ServiceRegistry] Class '{$className}' must implement {$requiredInterface}, " + . 'using fallback' + ); + return $fallbackClass; } - if (!is_subclass_of($className, $fallbackClass)) { + if (!$requiredInterface && !is_subclass_of($className, $fallbackClass)) { $this->modx->log( modX::LOG_LEVEL_ERROR, "[MiniShop3 ServiceRegistry] Class '{$className}' must extend {$fallbackClass}, using fallback" diff --git a/core/components/minishop3/src/ServiceRegistryFactories.php b/core/components/minishop3/src/ServiceRegistryFactories.php index 64cef45d4..0134f06c4 100644 --- a/core/components/minishop3/src/ServiceRegistryFactories.php +++ b/core/components/minishop3/src/ServiceRegistryFactories.php @@ -184,11 +184,16 @@ public static function map(): array ); }, + 'ms3_order_lifecycle_ports' => static function (modX $modx, object $services, string $class): object { + return new $class(); + }, + 'ms3_order_status' => static function (modX $modx, object $services, string $class): object { return new $class( $modx, self::ms3($modx), - $services->get('ms3_order_log') + $services->get('ms3_order_log'), + $services->get('ms3_order_lifecycle_ports') ); }, diff --git a/core/components/minishop3/src/Services/Order/ManagerOrderMutationService.php b/core/components/minishop3/src/Services/Order/ManagerOrderMutationService.php index 769f417e3..f681cfaea 100644 --- a/core/components/minishop3/src/Services/Order/ManagerOrderMutationService.php +++ b/core/components/minishop3/src/Services/Order/ManagerOrderMutationService.php @@ -201,12 +201,19 @@ public function update(array $params = []): array // Store old values for logging $oldStatusId = (int)$order->get('status_id'); + $pendingStatusId = array_key_exists('status_id', $params) + ? (int) $params['status_id'] + : null; // Get editable order fields from msModelField configuration $orderFields = $this->presenter->getModelFieldNames('msOrder'); $changedOrderFields = []; foreach ($orderFields as $field) { + // Non-draft status changes go only through OrderStatusService (#592). + if ($field === 'status_id') { + continue; + } if (array_key_exists($field, $params)) { $oldValue = $order->get($field); $newValue = $params[$field]; @@ -257,8 +264,7 @@ public function update(array $params = []): array return $this->error('Failed to update order', HttpStatus::INTERNAL_SERVER_ERROR); } - // Log order field changes (excluding status_id which is logged separately) - unset($changedOrderFields['status_id']); + // Log order field changes (status_id is logged by OrderStatusService) if (!empty($changedOrderFields)) { $this->getOrderLog()->addEntry( $id, @@ -325,15 +331,10 @@ public function update(array $params = []): array } // Handle status change via OrderStatusService (sends notifications) - $newStatusId = (int)$order->get('status_id'); - if ($oldStatusId !== $newStatusId) { - // Revert status to old value - OrderStatusService will change it properly - $order->set('status_id', $oldStatusId); - $order->save(); - + if ($pendingStatusId !== null && $pendingStatusId !== $oldStatusId) { /** @var OrderStatusService $orderStatusService */ $orderStatusService = $this->modx->services->get('ms3_order_status'); - $result = $orderStatusService->change((int)$order->get('id'), $newStatusId); + $result = $orderStatusService->change((int)$order->get('id'), $pendingStatusId); if ($result !== true) { return $this->error( diff --git a/core/components/minishop3/src/Services/Order/NullOrderLifecyclePorts.php b/core/components/minishop3/src/Services/Order/NullOrderLifecyclePorts.php new file mode 100644 index 000000000..bc4df83d2 --- /dev/null +++ b/core/components/minishop3/src/Services/Order/NullOrderLifecyclePorts.php @@ -0,0 +1,28 @@ +modx = $modx; $this->ms3 = $ms3; $this->orderLog = $orderLog; + $this->lifecyclePorts = $lifecyclePorts ?? new NullOrderLifecyclePorts(); $this->modx->lexicon->load('minishop3:default'); } @@ -90,15 +104,22 @@ public function ensure(int $orderId, int $statusId, bool $skipNotifications = fa } /** - * Switch order status + * Switch order status (single gate for non-draft transitions). * * @param int $orderId The id of msOrder * @param int $statusId The id of msOrderStatus * @param bool $skipNotifications Skip sending notifications (for admin finalization) + * @param array{idempotent?: bool} $options idempotent=true → same status is success no-op * @return bool|string True on success, error message on failure */ - public function change(int $orderId, int $statusId, bool $skipNotifications = false): bool|string - { + public function change( + int $orderId, + int $statusId, + bool $skipNotifications = false, + array $options = [] + ): bool|string { + $idempotent = !empty($options['idempotent']); + /** @var msOrder|null $msOrder */ $msOrder = $this->modx->getObject(msOrder::class, ['id' => $orderId]); if (!$msOrder) { @@ -115,26 +136,26 @@ public function change(int $orderId, int $statusId, bool $skipNotifications = fa return $this->modx->lexicon('ms3_err_status_nf'); } + $storedStatusId = $msOrder->get('status_id'); + $previousStatusId = $storedStatusId !== null ? (int) $storedStatusId : null; + /** @var msOrderStatusModel|null $oldStatus */ - $oldStatus = $this->modx->getObject( - msOrderStatusModel::class, - ['id' => $msOrder->get('status_id'), 'active' => 1] - ); + $oldStatus = $previousStatusId !== null + ? $this->modx->getObject(msOrderStatusModel::class, ['id' => $previousStatusId]) + : null; - if ($oldStatus) { - $transitionError = $this->validateStatusTransition($oldStatus, $status); - if ($transitionError !== null) { - return $transitionError; - } + if ($previousStatusId === $statusId) { + return $idempotent ? true : $this->modx->lexicon('ms3_err_status_same'); } - if ($msOrder->get('status_id') == $statusId) { - return $this->modx->lexicon('ms3_err_status_same'); + $transitionError = $this->validateStatusTransition($oldStatus, $status); + if ($transitionError !== null) { + return $transitionError; } $eventParams = [ 'msOrder' => $msOrder, - 'old_status' => $oldStatus?->get('id'), + 'old_status' => $previousStatusId, 'status' => $statusId, ]; $response = $this->ms3->utils->invokeEvent('msOnBeforeChangeOrderStatus', $eventParams); @@ -154,8 +175,8 @@ public function change(int $orderId, int $statusId, bool $skipNotifications = fa if (!$status) { return $this->modx->lexicon('ms3_err_status_nf'); } - if ($msOrder->get('status_id') == $statusId) { - return $this->modx->lexicon('ms3_err_status_same'); + if ($previousStatusId === $statusId) { + return $idempotent ? true : $this->modx->lexicon('ms3_err_status_same'); } $transitionError = $this->validateStatusTransition($oldStatus, $status); @@ -165,22 +186,26 @@ public function change(int $orderId, int $statusId, bool $skipNotifications = fa } $msOrder->set('status_id', $statusId); - if (!$msOrder->save()) { return $this->modx->lexicon('ms3_err_unknown'); } - $this->orderLog->add($msOrder->get('id'), $statusId, 'status'); + $portError = $this->runLifecyclePorts($msOrder, $statusId, $previousStatusId); + if ($portError !== null) { + return $this->rollbackStatus($msOrder, $previousStatusId) ?? $portError; + } $response = $this->ms3->utils->invokeEvent('msOnChangeOrderStatus', [ 'msOrder' => $msOrder, - 'old_status' => $oldStatus?->get('id'), + 'old_status' => $previousStatusId, 'status' => $statusId, ]); if (!$response['success']) { - return $response['message']; + return $this->rollbackStatus($msOrder, $previousStatusId) ?? $response['message']; } + $this->orderLog->add($msOrder->get('id'), $statusId, 'status'); + // Send notifications via NotificationManager (unless skipped) // Use output buffering to prevent any stray output from Fenom/pdoTools if (!$skipNotifications) { @@ -193,7 +218,7 @@ public function change(int $orderId, int $statusId, bool $skipNotifications = fa } /** - * Validate transition from old status to new (final/fixed rules). + * Validate transition: final/fixed defaults + optional allow-list (ms3_order_status_transitions). */ protected function validateStatusTransition( ?msOrderStatusModel $oldStatus, @@ -211,9 +236,68 @@ protected function validateStatusTransition( return $this->modx->lexicon('ms3_err_status_fixed'); } + $edges = OrderStatusTransitionPolicy::resolve( + $this->modx->getOption('ms3_order_status_transitions', null, '') + ); + if ($edges['mode'] === OrderStatusTransitionPolicy::MODE_INVALID) { + return $this->modx->lexicon('ms3_err_status_transitions_invalid'); + } + if ( + $edges['mode'] === OrderStatusTransitionPolicy::MODE_ON + && !isset($edges['edges'][(int) $oldStatus->get('id')][(int) $newStatus->get('id')]) + ) { + return $this->modx->lexicon('ms3_err_status_transition'); + } + + return null; + } + + /** + * Invoke semantic lifecycle ports when the target matches configured status ids. + */ + protected function runLifecyclePorts(msOrder $order, int $statusId, ?int $previousStatusId): ?string + { + $paidId = (int) $this->modx->getOption('ms3_status_paid', null, 3); + $canceledId = (int) $this->modx->getOption('ms3_status_canceled', null, 5); + $sentId = (int) $this->modx->getOption('ms3_status_sent', null, 4); + + if ($statusId === $paidId) { + return $this->lifecyclePorts->onOrderBecamePaid($order, $previousStatusId); + } + if ($statusId === $canceledId) { + return $this->lifecyclePorts->onOrderCancelled($order, $previousStatusId); + } + if ($statusId === $sentId) { + return $this->lifecyclePorts->onOrderShipped($order, $previousStatusId); + } + return null; } + /** + * Restore previous status_id after failed after-event or lifecycle port. + * + * @return string|null Lexicon/error when rollback persist fails + */ + protected function rollbackStatus(msOrder $order, ?int $previousStatusId): ?string + { + $order->set('status_id', $previousStatusId); + if ($order->save()) { + return null; + } + + $this->modx->log( + modX::LOG_LEVEL_ERROR, + sprintf( + '[MiniShop3] Failed to rollback order #%s status to %s', + (string) $order->get('id'), + $previousStatusId === null ? 'null' : (string) $previousStatusId + ) + ); + + return $this->modx->lexicon('ms3_err_status_rollback'); + } + /** * Send notifications for status change * diff --git a/core/components/minishop3/src/Services/Order/OrderStatusTransitionPolicy.php b/core/components/minishop3/src/Services/Order/OrderStatusTransitionPolicy.php new file mode 100644 index 000000000..8d1eb13c0 --- /dev/null +++ b/core/components/minishop3/src/Services/Order/OrderStatusTransitionPolicy.php @@ -0,0 +1,89 @@ +>} + */ + public static function resolve(mixed $raw): array + { + if ($raw === null) { + return ['mode' => self::MODE_OFF, 'edges' => []]; + } + + if (is_array($raw)) { + return ['mode' => self::MODE_ON, 'edges' => self::fromPairList($raw)]; + } + + $value = trim((string) $raw); + if ($value === '') { + return ['mode' => self::MODE_OFF, 'edges' => []]; + } + + if (str_starts_with($value, '[')) { + $decoded = json_decode($value, true); + if (!is_array($decoded)) { + return ['mode' => self::MODE_INVALID, 'edges' => []]; + } + + return ['mode' => self::MODE_ON, 'edges' => self::fromPairList($decoded)]; + } + + $pairs = []; + foreach (array_filter(array_map('trim', explode(',', $value))) as $pair) { + $parts = array_map('trim', explode(':', $pair, 2)); + if (count($parts) !== 2) { + return ['mode' => self::MODE_INVALID, 'edges' => []]; + } + $pairs[] = $parts; + } + + $edges = self::fromPairList($pairs); + if ($edges === []) { + return ['mode' => self::MODE_INVALID, 'edges' => []]; + } + + return ['mode' => self::MODE_ON, 'edges' => $edges]; + } + + /** + * @param array $pairs + * @return array> + */ + private static function fromPairList(array $pairs): array + { + $edges = []; + foreach ($pairs as $pair) { + if (!is_array($pair) || count($pair) < 2) { + continue; + } + $from = (int) $pair[0]; + $to = (int) $pair[1]; + if ($from < 1 || $to < 1) { + continue; + } + $edges[$from][$to] = true; + } + + return $edges; + } +} diff --git a/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php new file mode 100644 index 000000000..67077110e --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php @@ -0,0 +1,380 @@ +makeHarness(statusId: 2); + $log = $this->recordingLog(); + $events = []; + + $service = $this->makeService($harness, $log, new NullOrderLifecyclePorts(), $events); + $result = $service->change(10, 2, true, ['idempotent' => true]); + + self::assertTrue($result); + self::assertSame([], $log->entries); + self::assertSame([], $events); + } + + public function testSameStatusWithoutIdempotentReturnsError(): void + { + $harness = $this->makeHarness(statusId: 2); + $log = $this->recordingLog(); + $events = []; + + $service = $this->makeService($harness, $log, new NullOrderLifecyclePorts(), $events); + $result = $service->change(10, 2); + + self::assertSame('ms3_err_status_same', $result); + self::assertSame([], $log->entries); + } + + public function testAllowListRejectsUnknownEdgeWithoutSave(): void + { + $harness = $this->makeHarness(statusId: 2, options: [ + 'ms3_order_status_transitions' => '2:3', + ]); + $log = $this->recordingLog(); + $events = []; + + $service = $this->makeService($harness, $log, new NullOrderLifecyclePorts(), $events); + $result = $service->change(10, 4); + + self::assertSame('ms3_err_status_transition', $result); + self::assertSame(2, $harness['order']->get('status_id')); + self::assertSame([], $log->entries); + self::assertSame([], $events); + } + + public function testInvalidAllowListConfigRejectedWithoutSave(): void + { + $harness = $this->makeHarness(statusId: 2, options: [ + 'ms3_order_status_transitions' => '[broken', + ]); + $log = $this->recordingLog(); + $events = []; + + $service = $this->makeService($harness, $log, new NullOrderLifecyclePorts(), $events); + $result = $service->change(10, 3); + + self::assertSame('ms3_err_status_transitions_invalid', $result); + self::assertSame(2, $harness['order']->get('status_id')); + self::assertSame([], $log->entries); + } + + public function testCancelAndShippedPortsAreInvoked(): void + { + $ports = new class implements OrderLifecyclePortsInterface { + public int $cancelCalls = 0; + public int $shipCalls = 0; + + public function onOrderBecamePaid(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + + public function onOrderCancelled(msOrder $order, ?int $previousStatusId): ?string + { + ++$this->cancelCalls; + + return null; + } + + public function onOrderShipped(msOrder $order, ?int $previousStatusId): ?string + { + ++$this->shipCalls; + + return null; + } + }; + + $cancelHarness = $this->makeHarness(statusId: 2); + $cancelLog = $this->recordingLog(); + $cancelEvents = []; + $cancelService = $this->makeService($cancelHarness, $cancelLog, $ports, $cancelEvents); + self::assertTrue($cancelService->change(10, 5, true)); + self::assertSame(1, $ports->cancelCalls); + + $shipHarness = $this->makeHarness(statusId: 2); + $shipLog = $this->recordingLog(); + $shipEvents = []; + $shipService = $this->makeService($shipHarness, $shipLog, $ports, $shipEvents); + self::assertTrue($shipService->change(10, 4, true)); + self::assertSame(1, $ports->shipCalls); + } + + public function testAfterEventFailureRollsBackStatusAndSkipsLog(): void + { + $harness = $this->makeHarness(statusId: 2); + $log = $this->recordingLog(); + $events = []; + + $service = $this->makeService( + $harness, + $log, + new NullOrderLifecyclePorts(), + $events, + afterFail: true + ); + $result = $service->change(10, 3, true); + + self::assertSame('after failed', $result); + self::assertSame(2, $harness['order']->get('status_id')); + self::assertSame([], $log->entries); + self::assertSame(['msOnBeforeChangeOrderStatus', 'msOnChangeOrderStatus'], $events); + } + + public function testPaidPortRunsAndFailureRollsBack(): void + { + $harness = $this->makeHarness(statusId: 2); + $log = $this->recordingLog(); + $events = []; + $ports = new class implements OrderLifecyclePortsInterface { + public int $paidCalls = 0; + + public function onOrderBecamePaid(msOrder $order, ?int $previousStatusId): ?string + { + ++$this->paidCalls; + + return 'port failed'; + } + + public function onOrderCancelled(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + + public function onOrderShipped(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + }; + + $service = $this->makeService($harness, $log, $ports, $events); + $result = $service->change(10, 3, true); + + self::assertSame('port failed', $result); + self::assertSame(1, $ports->paidCalls); + self::assertSame(2, $harness['order']->get('status_id')); + self::assertSame([], $log->entries); + self::assertSame(['msOnBeforeChangeOrderStatus'], $events); + } + + public function testSuccessfulPaidTransitionLogsAndInvokesPort(): void + { + $harness = $this->makeHarness(statusId: 2); + $log = $this->recordingLog(); + $events = []; + $ports = new class implements OrderLifecyclePortsInterface { + public int $paidCalls = 0; + + public function onOrderBecamePaid(msOrder $order, ?int $previousStatusId): ?string + { + ++$this->paidCalls; + + return null; + } + + public function onOrderCancelled(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + + public function onOrderShipped(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + }; + + $service = $this->makeService($harness, $log, $ports, $events); + $result = $service->change(10, 3, true); + + self::assertTrue($result); + self::assertSame(1, $ports->paidCalls); + self::assertSame(3, $harness['order']->get('status_id')); + self::assertSame([[10, 3, 'status']], $log->entries); + self::assertSame(['msOnBeforeChangeOrderStatus', 'msOnChangeOrderStatus'], $events); + } + + /** + * @param array $options + * @return array{order: RecordingMsOrder, statuses: array, options: array} + */ + private function makeHarness(int $statusId, array $options = []): array + { + $order = new RecordingMsOrder([ + 'id' => 10, + 'status_id' => $statusId, + 'context' => 'web', + ]); + + $statuses = [ + 2 => $this->makeStatus(2, final: false, fixed: false, position: 2), + 3 => $this->makeStatus(3, final: false, fixed: true, position: 3), + 4 => $this->makeStatus(4, final: true, fixed: true, position: 4), + 5 => $this->makeStatus(5, final: true, fixed: false, position: 5), + ]; + + return [ + 'order' => $order, + 'statuses' => $statuses, + 'options' => array_merge([ + 'ms3_status_paid' => 3, + 'ms3_status_canceled' => 5, + 'ms3_status_sent' => 4, + 'ms3_order_status_transitions' => '', + ], $options), + ]; + } + + private function makeStatus(int $id, bool $final, bool $fixed, int $position): object + { + return new class($id, $final, $fixed, $position) extends msOrderStatus { + public function __construct( + private int $statusId, + private bool $isFinal, + private bool $isFixed, + private int $pos + ) { + } + + public function get($k, $format = null, $formatType = '') + { + return match ($k) { + 'id' => $this->statusId, + 'final' => $this->isFinal ? 1 : 0, + 'fixed' => $this->isFixed ? 1 : 0, + 'position' => $this->pos, + default => null, + }; + } + }; + } + + /** + * @return OrderLogService&object{entries: list} + */ + private function recordingLog(): OrderLogService + { + return new class extends OrderLogService { + /** @var list */ + public array $entries = []; + + public function __construct() + { + } + + public function add(int $order_id, mixed $entry, string $action, bool $visible = true): bool + { + $this->entries[] = [$order_id, $entry, $action]; + + return true; + } + }; + } + + /** + * @param array{order: RecordingMsOrder, statuses: array, options: array} $harness + * @param list $events + */ + private function makeService( + array $harness, + OrderLogService $log, + OrderLifecyclePortsInterface $ports, + array &$events, + bool $afterFail = false + ): OrderStatusService { + $modx = new class($harness, $afterFail) extends modX { + /** @var array{order: RecordingMsOrder, statuses: array, options: array} */ + private array $harness; + private bool $afterFail; + + public function __construct(array $harness, bool $afterFail) + { + parent::__construct(); + $this->harness = $harness; + $this->afterFail = $afterFail; + } + + public function getOption(string $key, $options = null, $default = null) + { + return $this->harness['options'][$key] ?? $default; + } + + public function switchContext($contextKey, $force = false) + { + return true; + } + + public function getObject($className, $criteria = null, $cacheFlag = true) + { + if ($className === msOrder::class || $className === RecordingMsOrder::class) { + $id = is_array($criteria) ? (int) ($criteria['id'] ?? 0) : (int) $criteria; + return $id === (int) $this->harness['order']->get('id') + ? $this->harness['order'] + : null; + } + + if ($className === msOrderStatus::class || is_a($className, msOrderStatus::class, true)) { + $id = is_array($criteria) ? (int) ($criteria['id'] ?? 0) : (int) $criteria; + return $this->harness['statuses'][$id] ?? null; + } + + return null; + } + }; + + $ms3 = $this->createMock(MiniShop3::class); + $ms3->method('initialize')->willReturn(true); + + $utils = new class($events, $afterFail) { + /** @var list */ + private array $events; + private bool $afterFail; + + public function __construct(array &$events, bool $afterFail) + { + $this->events = &$events; + $this->afterFail = $afterFail; + } + + public function invokeEvent(string $eventName, array $params = [], $glue = '
'): array + { + $this->events[] = $eventName; + if ($eventName === 'msOnChangeOrderStatus' && $this->afterFail) { + return ['success' => false, 'message' => 'after failed', 'data' => []]; + } + + return ['success' => true, 'message' => '', 'data' => $params]; + } + }; + $ms3->utils = $utils; + + return new OrderStatusService($modx, $ms3, $log, $ports); + } +} diff --git a/core/components/minishop3/tests/Unit/Services/Order/OrderStatusTransitionPolicyTest.php b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusTransitionPolicyTest.php new file mode 100644 index 000000000..4f511ee3b --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusTransitionPolicyTest.php @@ -0,0 +1,57 @@ + Date: Thu, 17 Sep 2026 19:27:28 +0600 Subject: [PATCH 2/5] refactor(order): align status change with in-TX ports, no post-commit rollback Ports run before status_id persist inside an optional DB transaction. msOnChangeOrderStatus failure no longer compensates with a second save, so status and future inventory stay consistent (#596 / #603 contract). --- .../minishop3/lexicon/en/default.inc.php | 1 - .../minishop3/lexicon/ru/default.inc.php | 1 - .../Order/OrderLifecyclePortsInterface.php | 14 +-- .../src/Services/Order/OrderStatusService.php | 108 ++++++++++++------ .../Order/OrderStatusServiceLifecycleTest.php | 8 +- 5 files changed, 81 insertions(+), 51 deletions(-) diff --git a/core/components/minishop3/lexicon/en/default.inc.php b/core/components/minishop3/lexicon/en/default.inc.php index fd804d02a..96bc9a6d8 100644 --- a/core/components/minishop3/lexicon/en/default.inc.php +++ b/core/components/minishop3/lexicon/en/default.inc.php @@ -216,7 +216,6 @@ $_lang['ms3_err_status_same'] = 'This status is already set.'; $_lang['ms3_err_status_transition'] = 'This status transition is not allowed.'; $_lang['ms3_err_status_transitions_invalid'] = 'Order status transition allow-list is invalid.'; -$_lang['ms3_err_status_rollback'] = 'Failed to roll back order status after a rejected transition.'; $_lang['ms3_err_register_globals'] = 'Error: php parameter register_globals must be disabled.'; $_lang['ms3_err_link_equal'] = 'You are trying to add product link to itself'; $_lang['ms3_err_no_link'] = 'Link type not found'; diff --git a/core/components/minishop3/lexicon/ru/default.inc.php b/core/components/minishop3/lexicon/ru/default.inc.php index 6315e09f5..e08811053 100644 --- a/core/components/minishop3/lexicon/ru/default.inc.php +++ b/core/components/minishop3/lexicon/ru/default.inc.php @@ -216,7 +216,6 @@ $_lang['ms3_err_status_same'] = 'Этот статус уже установлен.'; $_lang['ms3_err_status_transition'] = 'Такой переход статуса не разрешён.'; $_lang['ms3_err_status_transitions_invalid'] = 'Некорректный allow-list переходов статусов заказа.'; -$_lang['ms3_err_status_rollback'] = 'Не удалось откатить статус заказа после отклонённого перехода.'; $_lang['ms3_err_register_globals'] = 'Ошибка: php параметр register_globals должен быть выключен.'; $_lang['ms3_err_link_equal'] = 'Вы пытаетесь добавить товару ссылку на самого себя'; $_lang['ms3_err_no_link'] = 'Тип связи не найден'; diff --git a/core/components/minishop3/src/Services/Order/OrderLifecyclePortsInterface.php b/core/components/minishop3/src/Services/Order/OrderLifecyclePortsInterface.php index e03b52f83..a64616ea7 100644 --- a/core/components/minishop3/src/Services/Order/OrderLifecyclePortsInterface.php +++ b/core/components/minishop3/src/Services/Order/OrderLifecyclePortsInterface.php @@ -7,32 +7,32 @@ use MiniShop3\Model\msOrder; /** - * Domain ports invoked by order status lifecycle after a successful transition gate. + * In-TX domain ports invoked by {@see OrderStatusService} before status_id is persisted. * - * Implementations land with inventory (#589), payment (#590), and shipment (#591). + * Implementations land with inventory (#589 / #603), payment (#590), and shipment (#591). * Core ships {@see NullOrderLifecyclePorts} until those domains exist. * - * Ports must be safe to retry (idempotent) because after-event failure rolls back - * status while a port may already have run. + * A non-null return aborts the transition (transaction rollback); status_id is not saved. + * Ports run inside the same DB transaction as the status write when the connection supports it. */ interface OrderLifecyclePortsInterface { /** - * Order reached the configured "paid" status (ms3_status_paid). + * Order is transitioning to the configured "paid" status (ms3_status_paid). * * @return string|null Lexicon/error message on failure; null on success */ public function onOrderBecamePaid(msOrder $order, ?int $previousStatusId): ?string; /** - * Order reached the configured canceled status (ms3_status_canceled). + * Order is transitioning to the configured canceled status (ms3_status_canceled). * * @return string|null Lexicon/error message on failure; null on success */ public function onOrderCancelled(msOrder $order, ?int $previousStatusId): ?string; /** - * Order reached the configured shipped/sent status (ms3_status_sent, default 4). + * Order is transitioning to the configured shipped/sent status (ms3_status_sent, default 4). * * @return string|null Lexicon/error message on failure; null on success */ diff --git a/core/components/minishop3/src/Services/Order/OrderStatusService.php b/core/components/minishop3/src/Services/Order/OrderStatusService.php index caafcc0aa..69902b697 100644 --- a/core/components/minishop3/src/Services/Order/OrderStatusService.php +++ b/core/components/minishop3/src/Services/Order/OrderStatusService.php @@ -20,13 +20,17 @@ * Do not write `status_id` directly for paid / cancel / sent — call {@see change()}. * Draft creation may still set draft status_id without going through this service. * - * After a successful gate: lifecycle ports (#589–#591) → msOnChangeOrderStatus → log → notify. - * If after-event or a port fails, status is rolled back (no log / notify). - * msOrderLog is written only after a successful after-event (plugins must not rely on a fresh - * log row inside msOnChangeOrderStatus). + * Flow (contract with #603, see PR #596): + * 1. Validate + msOnBeforeChangeOrderStatus (may abort before any persist). + * 2. DB transaction: in-TX lifecycle ports (may deny) → persist status_id → commit. + * Rollback is only the DB transaction — no compensating status save after commit. + * 3. After commit: msOnChangeOrderStatus → log → notify. + * Plugin failure after commit returns an error but does **not** revert status_id + * (status and future inventory stay consistent). * * Options for {@see change()}: * - idempotent=true: already-in-status returns true without events/notify (integrations). + * Prefer {@see ensure()} when callers only need "end up in this status". */ class OrderStatusService implements OrderStatusChanger { @@ -185,14 +189,9 @@ public function change( } } - $msOrder->set('status_id', $statusId); - if (!$msOrder->save()) { - return $this->modx->lexicon('ms3_err_unknown'); - } - - $portError = $this->runLifecyclePorts($msOrder, $statusId, $previousStatusId); - if ($portError !== null) { - return $this->rollbackStatus($msOrder, $previousStatusId) ?? $portError; + $persistError = $this->persistStatusWithPorts($msOrder, $statusId, $previousStatusId); + if ($persistError !== null) { + return $persistError; } $response = $this->ms3->utils->invokeEvent('msOnChangeOrderStatus', [ @@ -201,7 +200,8 @@ public function change( 'status' => $statusId, ]); if (!$response['success']) { - return $this->rollbackStatus($msOrder, $previousStatusId) ?? $response['message']; + // Status (and in-TX domain work) already committed — do not compensate. + return $response['message']; } $this->orderLog->add($msOrder->get('id'), $statusId, 'status'); @@ -217,6 +217,61 @@ public function change( return true; } + /** + * In-TX domain hooks then persist status_id. On failure rolls back the transaction only. + */ + protected function persistStatusWithPorts( + msOrder $msOrder, + int $statusId, + ?int $previousStatusId + ): ?string { + $useTx = is_callable([$this->modx, 'beginTransaction']) + && is_callable([$this->modx, 'commit']) + && is_callable([$this->modx, 'rollback']); + + if ($useTx) { + $this->modx->beginTransaction(); + } + + try { + $portError = $this->runLifecyclePorts($msOrder, $statusId, $previousStatusId); + if ($portError !== null) { + if ($useTx) { + $this->modx->rollback(); + } + + return $portError; + } + + $msOrder->set('status_id', $statusId); + if (!$msOrder->save()) { + if ($useTx) { + $this->modx->rollback(); + } + $msOrder->set('status_id', $previousStatusId); + + return $this->modx->lexicon('ms3_err_unknown'); + } + + if ($useTx) { + $this->modx->commit(); + } + } catch (\Throwable $e) { + if ($useTx) { + $this->modx->rollback(); + } + $msOrder->set('status_id', $previousStatusId); + $this->modx->log( + modX::LOG_LEVEL_ERROR, + '[OrderStatusService] persistStatusWithPorts: ' . $e->getMessage() + ); + + return $this->modx->lexicon('ms3_err_unknown'); + } + + return null; + } + /** * Validate transition: final/fixed defaults + optional allow-list (ms3_order_status_transitions). */ @@ -253,7 +308,8 @@ protected function validateStatusTransition( } /** - * Invoke semantic lifecycle ports when the target matches configured status ids. + * Invoke in-TX semantic lifecycle ports when the target matches configured status ids. + * May deny the transition before status_id is persisted (#589–#591 / #603). */ protected function runLifecyclePorts(msOrder $order, int $statusId, ?int $previousStatusId): ?string { @@ -274,30 +330,6 @@ protected function runLifecyclePorts(msOrder $order, int $statusId, ?int $previo return null; } - /** - * Restore previous status_id after failed after-event or lifecycle port. - * - * @return string|null Lexicon/error when rollback persist fails - */ - protected function rollbackStatus(msOrder $order, ?int $previousStatusId): ?string - { - $order->set('status_id', $previousStatusId); - if ($order->save()) { - return null; - } - - $this->modx->log( - modX::LOG_LEVEL_ERROR, - sprintf( - '[MiniShop3] Failed to rollback order #%s status to %s', - (string) $order->get('id'), - $previousStatusId === null ? 'null' : (string) $previousStatusId - ) - ); - - return $this->modx->lexicon('ms3_err_status_rollback'); - } - /** * Send notifications for status change * diff --git a/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php index 67077110e..b32740b3b 100644 --- a/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php +++ b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php @@ -16,7 +16,7 @@ use PHPUnit\Framework\TestCase; /** - * Level-2: OrderStatusService gate — idempotency, allow-list, after-event rollback, ports. + * Level-2: OrderStatusService gate — idempotency, allow-list, after-event, in-TX ports. */ final class OrderStatusServiceLifecycleTest extends TestCase { @@ -128,7 +128,7 @@ public function onOrderShipped(msOrder $order, ?int $previousStatusId): ?string self::assertSame(1, $ports->shipCalls); } - public function testAfterEventFailureRollsBackStatusAndSkipsLog(): void + public function testAfterEventFailureKeepsCommittedStatusAndSkipsLog(): void { $harness = $this->makeHarness(statusId: 2); $log = $this->recordingLog(); @@ -144,12 +144,12 @@ public function testAfterEventFailureRollsBackStatusAndSkipsLog(): void $result = $service->change(10, 3, true); self::assertSame('after failed', $result); - self::assertSame(2, $harness['order']->get('status_id')); + self::assertSame(3, $harness['order']->get('status_id')); self::assertSame([], $log->entries); self::assertSame(['msOnBeforeChangeOrderStatus', 'msOnChangeOrderStatus'], $events); } - public function testPaidPortRunsAndFailureRollsBack(): void + public function testPaidPortFailureAbortsBeforeSave(): void { $harness = $this->makeHarness(statusId: 2); $log = $this->recordingLog(); From 88d8671f77de1c98fe297ce2510935eebfc1aea2 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Mon, 21 Sep 2026 14:39:03 +0600 Subject: [PATCH 3/5] fix(order): join open transactions and log status before the after-event Nested beginTransaction() threw when change() ran inside an existing PDO transaction. The status log is written after commit and before msOnChangeOrderStatus, and shipment sync uses ensure() so a webhook retry is not rejected as already set. --- .../src/Services/Order/OrderStatusService.php | 75 +++++++---- .../Shipment/ShipmentLifecycleService.php | 2 +- .../Order/OrderStatusServiceLifecycleTest.php | 120 +++++++++++++++++- .../Shipment/ShipmentLifecycleServiceTest.php | 6 +- 4 files changed, 171 insertions(+), 32 deletions(-) diff --git a/core/components/minishop3/src/Services/Order/OrderStatusService.php b/core/components/minishop3/src/Services/Order/OrderStatusService.php index 69902b697..7de29d8e1 100644 --- a/core/components/minishop3/src/Services/Order/OrderStatusService.php +++ b/core/components/minishop3/src/Services/Order/OrderStatusService.php @@ -194,6 +194,10 @@ public function change( return $persistError; } + // Log before the after-event: status is already committed, and a plugin + // error must not erase the history of that transition. + $this->orderLog->add($msOrder->get('id'), $statusId, 'status'); + $response = $this->ms3->utils->invokeEvent('msOnChangeOrderStatus', [ 'msOrder' => $msOrder, 'old_status' => $previousStatusId, @@ -204,8 +208,6 @@ public function change( return $response['message']; } - $this->orderLog->add($msOrder->get('id'), $statusId, 'status'); - // Send notifications via NotificationManager (unless skipped) // Use output buffering to prevent any stray output from Fenom/pdoTools if (!$skipNotifications) { @@ -218,48 +220,36 @@ public function change( } /** - * In-TX domain hooks then persist status_id. On failure rolls back the transaction only. + * In-TX domain hooks then persist status_id. + * Joins an already open transaction instead of starting a nested one. + * On failure rolls back only the transaction this method opened. */ protected function persistStatusWithPorts( msOrder $msOrder, int $statusId, ?int $previousStatusId ): ?string { - $useTx = is_callable([$this->modx, 'beginTransaction']) - && is_callable([$this->modx, 'commit']) - && is_callable([$this->modx, 'rollback']); - - if ($useTx) { - $this->modx->beginTransaction(); - } + $ownsTx = $this->beginOwnedTransaction(); try { $portError = $this->runLifecyclePorts($msOrder, $statusId, $previousStatusId); if ($portError !== null) { - if ($useTx) { - $this->modx->rollback(); - } + $this->rollbackOwnedTransaction($ownsTx); return $portError; } $msOrder->set('status_id', $statusId); if (!$msOrder->save()) { - if ($useTx) { - $this->modx->rollback(); - } + $this->rollbackOwnedTransaction($ownsTx); $msOrder->set('status_id', $previousStatusId); return $this->modx->lexicon('ms3_err_unknown'); } - if ($useTx) { - $this->modx->commit(); - } + $this->commitOwnedTransaction($ownsTx); } catch (\Throwable $e) { - if ($useTx) { - $this->modx->rollback(); - } + $this->rollbackOwnedTransaction($ownsTx); $msOrder->set('status_id', $previousStatusId); $this->modx->log( modX::LOG_LEVEL_ERROR, @@ -272,6 +262,47 @@ protected function persistStatusWithPorts( return null; } + /** + * Start a transaction only when none is active. + * PDO::beginTransaction() throws if one is already open. + */ + private function beginOwnedTransaction(): bool + { + if ( + !is_callable([$this->modx, 'inTransaction']) + || !is_callable([$this->modx, 'beginTransaction']) + || !is_callable([$this->modx, 'commit']) + || !is_callable([$this->modx, 'rollback']) + ) { + return false; + } + if ($this->modx->inTransaction()) { + return false; + } + + $this->modx->beginTransaction(); + + return true; + } + + private function commitOwnedTransaction(bool $ownsTx): void + { + if ($ownsTx) { + $this->modx->commit(); + } + } + + private function rollbackOwnedTransaction(bool $ownsTx): void + { + if (!$ownsTx) { + return; + } + if (is_callable([$this->modx, 'inTransaction']) && !$this->modx->inTransaction()) { + return; + } + $this->modx->rollback(); + } + /** * Validate transition: final/fixed defaults + optional allow-list (ms3_order_status_transitions). */ diff --git a/core/components/minishop3/src/Services/Shipment/ShipmentLifecycleService.php b/core/components/minishop3/src/Services/Shipment/ShipmentLifecycleService.php index 6fae83bf6..2ffe05bd6 100644 --- a/core/components/minishop3/src/Services/Shipment/ShipmentLifecycleService.php +++ b/core/components/minishop3/src/Services/Shipment/ShipmentLifecycleService.php @@ -352,7 +352,7 @@ private function syncOrderStatus(int $orderId, string $shipmentStatus): void if ((int) $order->get('status_id') === $statusId) { return; } - $result = $this->orderStatus->change($orderId, $statusId); + $result = $this->orderStatus->ensure($orderId, $statusId); if ($result !== true) { $this->modx->log( modX::LOG_LEVEL_ERROR, diff --git a/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php index b32740b3b..ee5b350bf 100644 --- a/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php +++ b/core/components/minishop3/tests/Unit/Services/Order/OrderStatusServiceLifecycleTest.php @@ -128,7 +128,7 @@ public function onOrderShipped(msOrder $order, ?int $previousStatusId): ?string self::assertSame(1, $ports->shipCalls); } - public function testAfterEventFailureKeepsCommittedStatusAndSkipsLog(): void + public function testAfterEventFailureKeepsCommittedStatusAndStillLogs(): void { $harness = $this->makeHarness(statusId: 2); $log = $this->recordingLog(); @@ -145,8 +145,69 @@ public function testAfterEventFailureKeepsCommittedStatusAndSkipsLog(): void self::assertSame('after failed', $result); self::assertSame(3, $harness['order']->get('status_id')); - self::assertSame([], $log->entries); - self::assertSame(['msOnBeforeChangeOrderStatus', 'msOnChangeOrderStatus'], $events); + self::assertSame([[10, 3, 'status']], $log->entries); + self::assertSame( + ['msOnBeforeChangeOrderStatus', 'log-present', 'msOnChangeOrderStatus'], + $events + ); + } + + public function testJoinsOuterTransactionWithoutBeginOrCommit(): void + { + $harness = $this->makeHarness(statusId: 2); + $harness['tx'] = (object) [ + 'outer' => true, + 'open' => false, + 'begins' => 0, + 'commits' => 0, + 'rollbacks' => 0, + ]; + $log = $this->recordingLog(); + $events = []; + $service = $this->makeService($harness, $log, new NullOrderLifecyclePorts(), $events); + + self::assertTrue($service->change(10, 3, true)); + self::assertSame(3, $harness['order']->get('status_id')); + self::assertSame(0, $harness['tx']->begins); + self::assertSame(0, $harness['tx']->commits); + self::assertSame(0, $harness['tx']->rollbacks); + } + + public function testOwnedTransactionRollsBackWhenPortDenies(): void + { + $harness = $this->makeHarness(statusId: 2); + $harness['tx'] = (object) [ + 'outer' => false, + 'open' => false, + 'begins' => 0, + 'commits' => 0, + 'rollbacks' => 0, + ]; + $log = $this->recordingLog(); + $events = []; + $ports = new class implements OrderLifecyclePortsInterface { + public function onOrderBecamePaid(msOrder $order, ?int $previousStatusId): ?string + { + return 'port failed'; + } + + public function onOrderCancelled(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + + public function onOrderShipped(msOrder $order, ?int $previousStatusId): ?string + { + return null; + } + }; + $service = $this->makeService($harness, $log, $ports, $events); + + self::assertSame('port failed', $service->change(10, 3, true)); + self::assertSame(2, $harness['order']->get('status_id')); + self::assertSame(1, $harness['tx']->begins); + self::assertSame(0, $harness['tx']->commits); + self::assertSame(1, $harness['tx']->rollbacks); } public function testPaidPortFailureAbortsBeforeSave(): void @@ -310,7 +371,7 @@ private function makeService( bool $afterFail = false ): OrderStatusService { $modx = new class($harness, $afterFail) extends modX { - /** @var array{order: RecordingMsOrder, statuses: array, options: array} */ + /** @var array{order: RecordingMsOrder, statuses: array, options: array, tx?: object} */ private array $harness; private bool $afterFail; @@ -319,6 +380,47 @@ public function __construct(array $harness, bool $afterFail) parent::__construct(); $this->harness = $harness; $this->afterFail = $afterFail; + if (!isset($this->harness['tx'])) { + $this->harness['tx'] = (object) [ + 'outer' => false, + 'open' => false, + 'begins' => 0, + 'commits' => 0, + 'rollbacks' => 0, + ]; + } + } + + public function inTransaction(): bool + { + return $this->harness['tx']->outer || $this->harness['tx']->open; + } + + public function beginTransaction() + { + if ($this->inTransaction()) { + throw new \PDOException('There is already an active transaction'); + } + $this->harness['tx']->begins++; + $this->harness['tx']->open = true; + + return true; + } + + public function commit() + { + $this->harness['tx']->commits++; + $this->harness['tx']->open = false; + + return true; + } + + public function rollback() + { + $this->harness['tx']->rollbacks++; + $this->harness['tx']->open = false; + + return true; } public function getOption(string $key, $options = null, $default = null) @@ -352,19 +454,25 @@ public function getObject($className, $criteria = null, $cacheFlag = true) $ms3 = $this->createMock(MiniShop3::class); $ms3->method('initialize')->willReturn(true); - $utils = new class($events, $afterFail) { + $utils = new class($events, $afterFail, $log) { /** @var list */ private array $events; private bool $afterFail; + private OrderLogService $log; - public function __construct(array &$events, bool $afterFail) + public function __construct(array &$events, bool $afterFail, OrderLogService $log) { $this->events = &$events; $this->afterFail = $afterFail; + $this->log = $log; } public function invokeEvent(string $eventName, array $params = [], $glue = '
'): array { + if ($eventName === 'msOnChangeOrderStatus' && $this->afterFail) { + $entries = $this->log->entries ?? []; + $this->events[] = $entries === [] ? 'log-missing' : 'log-present'; + } $this->events[] = $eventName; if ($eventName === 'msOnChangeOrderStatus' && $this->afterFail) { return ['success' => false, 'message' => 'after failed', 'data' => []]; diff --git a/core/components/minishop3/tests/Unit/Services/Shipment/ShipmentLifecycleServiceTest.php b/core/components/minishop3/tests/Unit/Services/Shipment/ShipmentLifecycleServiceTest.php index 53d82b90e..44ff9a074 100644 --- a/core/components/minishop3/tests/Unit/Services/Shipment/ShipmentLifecycleServiceTest.php +++ b/core/components/minishop3/tests/Unit/Services/Shipment/ShipmentLifecycleServiceTest.php @@ -123,7 +123,7 @@ public function testOrderStatusFailureDoesNotRollBackShipment(): void $order = new StubMsOrder(['id' => 10, 'delivery_id' => 7, 'status_id' => 3]); $modx = $this->modx($order, true); $orderStatus = $this->createMock(OrderStatusService::class); - $orderStatus->method('change')->willReturn('ms3_err_status_final'); + $orderStatus->method('ensure')->willReturn('ms3_err_status_final'); $service = new ShipmentLifecycleService($store, $modx, $orderStatus); $row = $service->create(10); $updated = $service->transition($row['id'], ShipmentStatus::SHIPPED, 'evt-1'); @@ -350,7 +350,7 @@ public function testWebhookDeliveryMismatchDoesNotCreateShipment(): void $store = new InMemoryShipmentStore(); $order = new StubMsOrder(['id' => 10, 'delivery_id' => 5, 'status_id' => 3]); $orderStatus = $this->createMock(OrderStatusService::class); - $orderStatus->method('change')->willReturnCallback( + $orderStatus->method('ensure')->willReturnCallback( function (int $orderId, int $statusId): bool { $this->statusChanges[] = [$orderId, $statusId]; @@ -377,7 +377,7 @@ private function service(InMemoryShipmentStore $store, bool $enabled = false): S { $order = new StubMsOrder(['id' => 10, 'delivery_id' => 7, 'status_id' => 3]); $orderStatus = $this->createMock(OrderStatusService::class); - $orderStatus->method('change')->willReturnCallback( + $orderStatus->method('ensure')->willReturnCallback( function (int $orderId, int $statusId): bool { $this->statusChanges[] = [$orderId, $statusId]; From 03915ad78590e82c66f3c8763c8b51079eafe5b6 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Mon, 21 Sep 2026 14:42:21 +0600 Subject: [PATCH 4/5] fix(order): detect an open transaction on xPDO's PDO handle xPDO has no inTransaction() method, so the previous check never started a transaction on MODX. Use $modx->pdo->inTransaction() instead. --- .../src/Services/Order/OrderStatusService.php | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/core/components/minishop3/src/Services/Order/OrderStatusService.php b/core/components/minishop3/src/Services/Order/OrderStatusService.php index 7de29d8e1..799653c28 100644 --- a/core/components/minishop3/src/Services/Order/OrderStatusService.php +++ b/core/components/minishop3/src/Services/Order/OrderStatusService.php @@ -265,18 +265,19 @@ protected function persistStatusWithPorts( /** * Start a transaction only when none is active. * PDO::beginTransaction() throws if one is already open. + * + * xPDO exposes the connection as public $pdo and has no inTransaction(). */ private function beginOwnedTransaction(): bool { if ( - !is_callable([$this->modx, 'inTransaction']) - || !is_callable([$this->modx, 'beginTransaction']) + !is_callable([$this->modx, 'beginTransaction']) || !is_callable([$this->modx, 'commit']) - || !is_callable([$this->modx, 'rollback']) + || !is_callable([$this->modx, 'rollBack']) ) { return false; } - if ($this->modx->inTransaction()) { + if ($this->hasOpenTransaction()) { return false; } @@ -285,6 +286,21 @@ private function beginOwnedTransaction(): bool return true; } + /** + * Test doubles may define inTransaction() on the modX subclass. + * Production xPDO does not: the check is $modx->pdo->inTransaction(). + */ + private function hasOpenTransaction(): bool + { + if (method_exists($this->modx, 'inTransaction')) { + return (bool) $this->modx->inTransaction(); + } + + $pdo = $this->modx->pdo ?? null; + + return $pdo instanceof \PDO && $pdo->inTransaction(); + } + private function commitOwnedTransaction(bool $ownsTx): void { if ($ownsTx) { @@ -297,7 +313,7 @@ private function rollbackOwnedTransaction(bool $ownsTx): void if (!$ownsTx) { return; } - if (is_callable([$this->modx, 'inTransaction']) && !$this->modx->inTransaction()) { + if (!$this->hasOpenTransaction()) { return; } $this->modx->rollback(); From 444d579396701db3d5c528cb5b97a3f7c23bae79 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Mon, 21 Sep 2026 14:46:22 +0600 Subject: [PATCH 5/5] fix(order): align shipment status mocks with ensure() and PHPStan Webhook tests still stubbed change() after sync moved to ensure(), so CI saw no status update. Drop the null coalesce on xPDO's pdo property. --- .../minishop3/src/Services/Order/OrderStatusService.php | 7 +++++-- .../Api/Manager/OrderShipmentControllerTest.php | 2 +- .../Controllers/Api/Web/DeliveryWebhookControllerTest.php | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/core/components/minishop3/src/Services/Order/OrderStatusService.php b/core/components/minishop3/src/Services/Order/OrderStatusService.php index 799653c28..246d249c4 100644 --- a/core/components/minishop3/src/Services/Order/OrderStatusService.php +++ b/core/components/minishop3/src/Services/Order/OrderStatusService.php @@ -296,9 +296,12 @@ private function hasOpenTransaction(): bool return (bool) $this->modx->inTransaction(); } - $pdo = $this->modx->pdo ?? null; + $pdo = $this->modx->pdo; + if (!$pdo instanceof \PDO) { + return false; + } - return $pdo instanceof \PDO && $pdo->inTransaction(); + return $pdo->inTransaction(); } private function commitOwnedTransaction(bool $ownsTx): void diff --git a/core/components/minishop3/tests/Unit/Controllers/Api/Manager/OrderShipmentControllerTest.php b/core/components/minishop3/tests/Unit/Controllers/Api/Manager/OrderShipmentControllerTest.php index c5714b21e..466da0da4 100644 --- a/core/components/minishop3/tests/Unit/Controllers/Api/Manager/OrderShipmentControllerTest.php +++ b/core/components/minishop3/tests/Unit/Controllers/Api/Manager/OrderShipmentControllerTest.php @@ -141,7 +141,7 @@ private function modxWithLifecycle(): modX $store = new InMemoryShipmentStore(); $order = new StubMsOrder(['id' => 10, 'delivery_id' => 7, 'status_id' => 3]); $orderStatus = $this->createMock(OrderStatusService::class); - $orderStatus->method('change')->willReturn(true); + $orderStatus->method('ensure')->willReturn(true); $lifecycle = new ShipmentLifecycleService($store, $this->modx($order), $orderStatus); return $this->modx($order, ['ms3_shipment_lifecycle' => $lifecycle]); diff --git a/core/components/minishop3/tests/Unit/Controllers/Api/Web/DeliveryWebhookControllerTest.php b/core/components/minishop3/tests/Unit/Controllers/Api/Web/DeliveryWebhookControllerTest.php index fae0ce451..e2f0659d0 100644 --- a/core/components/minishop3/tests/Unit/Controllers/Api/Web/DeliveryWebhookControllerTest.php +++ b/core/components/minishop3/tests/Unit/Controllers/Api/Web/DeliveryWebhookControllerTest.php @@ -212,7 +212,7 @@ protected function readRawRequestBody(): string private function lifecycle(InMemoryShipmentStore $store, msOrder $order): ShipmentLifecycleService { $orderStatus = $this->createMock(OrderStatusService::class); - $orderStatus->method('change')->willReturnCallback( + $orderStatus->method('ensure')->willReturnCallback( function (int $orderId, int $statusId): bool { $this->statusChanges[] = [$orderId, $statusId];