src/Sylius/Bundle/CoreBundle/tests/Doctrine/ORM/Promotion/Modifier/AtomicOrderPromotionsUsageModifierTest.php (1)

46-221: Add one lock-failure test to pin non-delegation behavior.

Current tests are all happy-path. Please add a case where locking fails and assert the decorated modifier is not called.

Proposed test addition

+    public function testDoesNotDelegateWhenLockFailsDuringIncrement(): void
+    {
+        $order = $this->createMock(OrderInterface::class);
+        $promotion = $this->createMock(PromotionInterface::class);
+
+        $order->method('getPromotions')->willReturn(new ArrayCollection([$promotion]));
+        $order->method('getPromotionCoupon')->willReturn(null);
+        $promotion->method('getVersion')->willReturn(1);
+
+        $this->entityManager->expects($this->once())->method('refresh')->with($promotion);
+        $this->entityManager
+            ->expects($this->once())
+            ->method('lock')
+            ->with($promotion, LockMode::OPTIMISTIC, 1)
+            ->willThrowException(new \RuntimeException('Lock failed'))
+        ;
+
+        $this->decoratedModifier->expects($this->never())->method('increment');
+
+        $this->expectException(\RuntimeException::class);
+        $this->atomicModifier->increment($order);
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/Sylius/Bundle/CoreBundle/tests/Doctrine/ORM/Promotion/Modifier/AtomicOrderPromotionsUsageModifierTest.php`
around lines 46 - 221, Add a negative test in
AtomicOrderPromotionsUsageModifierTest that simulates EntityManager::lock
throwing an exception and asserts the decorated modifier is not called; create
an OrderInterface mock with a Promotion (and optionally a Coupon), have the
promotion return a version, mock
$this->entityManager->expects(...)->method('refresh')->with(...) as usual and
mock ->method('lock') to throw an exception (e.g. new ORMException), then assert
$this->decoratedModifier->expects($this->never())->method('increment') (or
'decrement' for the other case) and call
$this->atomicModifier->increment($order) inside an expectException or try/catch
to verify the exception is propagated (or handled) while ensuring decorated
modifier was not invoked.

src/Sylius/Component/Core/Model/PromotionCouponInterface.php (1)

17-19: ⚠️ Potential issue | 🟠 Major

BC break: extending VersionedInterface forces third-party implementers to add version methods.

Adding VersionedInterface to the interface contract is a breaking change for any external implementations of PromotionCouponInterface. The PR description states "No BC breaks," but this change requires all implementers to add getVersion() and setVersion() methods.

Consider:

1. Documenting this as an explicit BC break in the changelog, or
2. Introducing a new interface (e.g., VersionedPromotionCouponInterface) that extends both interfaces, allowing a deprecation path

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Sylius/Component/Core/Model/PromotionCouponInterface.php` around lines 17
- 19, The change made PromotionCouponInterface to extend VersionedInterface
which introduces a BC break; revert that direct extension and instead create a
new interface VersionedPromotionCouponInterface that extends
PromotionCouponInterface and VersionedInterface (so existing implementers are
unaffected), move any internal usages that require versioning to type-hint
VersionedPromotionCouponInterface, and ensure getVersion()/setVersion() remain
only on the new interface; alternatively, if you intend to keep the change,
document it as an explicit BC break in the changelog and note implementers must
add getVersion()/setVersion() to their implementations.

src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Promotion/Modifier/AtomicOrderPromotionsUsageModifier.php (1)

70-76: Runtime assertion guards type safety for optimistic locking.

The method signature accepts PromotionCouponInterface|PromotionInterface (where PromotionInterface is from the Component namespace), but getVersion() is only guaranteed by VersionedInterface. The runtime Assert::isInstanceOf check handles this gap.

This works correctly in practice because Sylius's actual entity implementations extend the Core interfaces that include VersionedInterface. However, if third-party code provides a pure Component-level implementation without versioning, this will fail at runtime rather than compile-time.

Consider documenting this expectation or using VersionedInterface directly in the type signature if only versioned entities are supported.

Alternative: Use VersionedInterface in the type signature

-    private function refreshAndLock(PromotionCouponInterface|PromotionInterface $entity): void
+    private function refreshAndLock(VersionedInterface $entity): void
     {
         $this->entityManager->refresh($entity);
 
-        Assert::isInstanceOf($entity, VersionedInterface::class);
         $this->entityManager->lock($entity, LockMode::OPTIMISTIC, $entity->getVersion());
     }

This would require updating lockEntities() to filter or assert types before calling refreshAndLock().

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/Sylius/Bundle/CoreBundle/Doctrine/ORM/Promotion/Modifier/AtomicOrderPromotionsUsageModifier.php`
around lines 70 - 76, The method refreshAndLock currently accepts
PromotionCouponInterface|PromotionInterface but relies on
VersionedInterface::getVersion(); change refreshAndLock to require
VersionedInterface (i.e. signature accepts VersionedInterface) and update
lockEntities (the caller) to either filter the collection to only
VersionedInterface instances or assert/cast each entity before calling
refreshAndLock, so no runtime Assert is needed; alternatively add an inline
docblock on refreshAndLock stating the requirement if you prefer to keep the
broader signature.