[API] Fix HTTP error codes for cart operations (fixes #18475)#18476
[API] Fix HTTP error codes for cart operations (fixes #18475)#18476NoResponseMate merged 1 commit intoSylius:2.1from
Conversation
WalkthroughThe PR refactors exception handling in cart command handlers to throw domain-specific HTTP exceptions (UnprocessableCartException, ProductVariantUnprocessableException, NotFoundHttpException, ConflictHttpException) instead of generic InvalidArgumentException, and adds integration tests validating error responses for cart and product variant not-found scenarios. Changes
Sequence DiagramsequenceDiagram
autonumber
participant Client
participant API as API Router
participant Handler as AddItemToCartHandler
participant Repo as Cart Repository
Client->>API: POST /api/v2/shop/orders/{invalidToken}/items
API->>Handler: AddItemToCart (invalid token)
Handler->>Repo: find cart by token
Repo-->>Handler: null
rect rgb(255, 235, 205)
Note over Handler: OLD: throw InvalidArgumentException
Handler-->>API: 500 Internal Server Error
end
rect rgb(200, 255, 200)
Note over Handler: NEW: throw UnprocessableCartException
Handler-->>API: 422 Unprocessable Entity
end
API-->>Client: HTTP 422 + hydra:description
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
❌ Preview Environment deleted from BunnyshellAvailable commands:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php (1)
28-30: Remove unused import.
UnprocessableEntityHttpExceptionis imported but not used in this test file.Apply this diff:
use Sylius\Bundle\ApiBundle\Exception\CartNotFoundException; use Sylius\Bundle\ApiBundle\Exception\ProductVariantNotFoundException; -use Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php(1 hunks)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php(1 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php(3 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php(3 hunks)tests/Api/Shop/Checkout/CartTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{php,yaml,yml,xml,twig}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation in PHP, YAML, XML, and Twig files
Files:
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.phpsrc/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.phpsrc/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.phpsrc/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.phptests/Api/Shop/Checkout/CartTest.php
**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
**/*.php: Use modern PHP 8.2+ syntax and features
Declare strict_types=1 in all PHP files
Follow the Sylius Coding Standard
Do not use deprecated features from PHP, Symfony, or Sylius
Use final for all classes, except entities and repositories
Use readonly for immutable services and value objects
Add type declarations for all properties, arguments, and return values
Use camelCase for variables and method names
Use SCREAMING_SNAKE_CASE for constants
Use fast returns instead of unnecessary nesting
Use trailing commas in multi-line arrays and argument lists
Order array keys alphabetically where applicable
Use PHPDoc only when necessary (e.g., @var Collection)
Group class elements in order: constants, properties, constructor, public, protected, private methods
Group getter and setter methods for the same properties together
Suffix interfaces with Interface and traits with Trait
Use use statements for all non-global classes
Sort use imports alphabetically and group by type (classes, functions, constants)
Files:
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.phpsrc/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.phpsrc/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.phpsrc/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.phptests/Api/Shop/Checkout/CartTest.php
tests/**/*Test.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PHPUnit for unit and functional testing
Files:
tests/Api/Shop/Checkout/CartTest.php
🧬 Code graph analysis (7)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
__construct(20-26)
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
__construct(20-26)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
CartNotFoundException(18-27)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php (2)
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
CartNotFoundException(18-27)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
ProductVariantNotFoundException(18-27)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php (3)
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
CartNotFoundException(18-27)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php (1)
__invoke(45-69)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
__invoke(41-64)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php (2)
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
CartNotFoundException(18-27)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
ProductVariantNotFoundException(18-27)
tests/Api/Shop/Checkout/CartTest.php (1)
tests/Api/JsonApiTestCase.php (2)
setUpDefaultPostHeaders(95-101)requestPost(169-182)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Static checks / PHP 8.4, Symfony ^7.3
🔇 Additional comments (18)
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
1-27: LGTM! Exception class follows best practices.The implementation is clean and consistent with the existing
ProductVariantNotFoundExceptionpattern. The use of 422 Unprocessable Entity for a missing cart aligns with the PR's rationale for POST operations where the endpoint exists but the resource cannot be processed.src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php (3)
17-18: Good addition of domain-specific exception imports.
50-52: Improved error handling with domain-specific exception.The change from generic
InvalidArgumentExceptiontoProductVariantNotFoundExceptionprovides clearer semantics and appropriate HTTP 422 response.
57-59: Improved error handling with domain-specific exception.The change from generic
InvalidArgumentExceptiontoCartNotFoundExceptionprovides clearer semantics and appropriate HTTP 422 response.src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php (2)
97-97: Test correctly updated for new exception type.
116-116: Test correctly updated for new exception type.src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (5)
17-17: Appropriate import for domain-specific exception.
23-24: Good addition of HTTP-specific exception imports.
46-48: Appropriate use of NotFoundHttpException (404) for missing user.
53-55: Appropriate use of CartNotFoundException (422) for missing cart.Consistent with the new error handling pattern across cart operations.
57-59: Excellent use of ConflictHttpException (409) for cart conflicts.HTTP 409 is semantically correct when the cart already has an assigned customer.
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php (4)
26-28: Appropriate imports for updated exception types.
85-85: Test correctly updated to expect ConflictHttpException.
95-95: Test correctly updated to expect CartNotFoundException.
101-101: Test correctly updated to expect NotFoundHttpException.src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
1-27: LGTM! Exception class follows best practices.The implementation is clean and consistent with
CartNotFoundException. Both exception classes follow the same pattern and adhere to coding guidelines.tests/Api/Shop/Checkout/CartTest.php (2)
626-650: Excellent integration test for cart-not-found scenario.The test properly verifies that a 422 status is returned with the expected error message when attempting to add an item to a non-existing cart.
652-679: Integration test may not exercise handler's ProductVariantNotFoundException.The test correctly verifies that a 422 response is returned, but the comment on Line 675 reveals that API Platform's IRI validation is likely catching the invalid product variant IRI before the handler executes. This means
ProductVariantNotFoundExceptionin the handler may not be tested at this integration level.Note: The handler's
ProductVariantNotFoundExceptionis properly tested at the unit level inAddItemToCartHandlerTest::testThrowsAnExceptionIfProductIsNotFound().
b1f8ed2 to
65f7e71
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
47-59: Exception semantics look good; consider message consistency and confirm 404 vs 422 policy.
404 for missing customer, 422 for missing cart, 409 for assigned customer: OK, but please confirm the policy that user-not-found remains 404 while cart-not-found is 422 for POST-like flows to avoid mixed semantics across endpoints.
Minor copy consistency with other messages (“…has not been found.” + period). Optional polish:
-throw new NotFoundHttpException('There is currently no customer with given email'); +throw new NotFoundHttpException('Customer with the given email address has not been found.');-throw new ConflictHttpException('There is an assigned customer to this cart'); +throw new ConflictHttpException('There is already an assigned customer to this cart.');src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
18-26: LGTM; optional: centralize message as a constant for reuse.Current implementation is correct and aligns with the 422 strategy. For consistency and reuse across tests and code, consider a constant:
final class CartNotFoundException extends UnprocessableEntityHttpException { + private const MESSAGE = 'Cart with the given token has not been found.'; + public function __construct( ?\Throwable $previous = null, int $code = 0, array $headers = [], ) { - parent::__construct('Cart with given token has not been found.', $previous, $code, $headers); + parent::__construct(self::MESSAGE, $previous, $code, $headers); } }tests/Api/Shop/Checkout/CartTest.php (1)
626-650: Good negative test for missing cart; asserts 422 and useful description.Minor hardening: also assert the JSON-LD error shape (e.g., presence of 'hydra:title' and '@type' = 'hydra:Error') to avoid coupling solely to message text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php(1 hunks)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php(1 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php(3 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php(3 hunks)tests/Api/Shop/Checkout/CartTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php
- src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php
- src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{php,yaml,yml,xml,twig}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation in PHP, YAML, XML, and Twig files
Files:
tests/Api/Shop/Checkout/CartTest.phpsrc/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.phpsrc/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php
tests/**/*Test.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PHPUnit for unit and functional testing
Files:
tests/Api/Shop/Checkout/CartTest.php
**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
**/*.php: Use modern PHP 8.2+ syntax and features
Declare strict_types=1 in all PHP files
Follow the Sylius Coding Standard
Do not use deprecated features from PHP, Symfony, or Sylius
Use final for all classes, except entities and repositories
Use readonly for immutable services and value objects
Add type declarations for all properties, arguments, and return values
Use camelCase for variables and method names
Use SCREAMING_SNAKE_CASE for constants
Use fast returns instead of unnecessary nesting
Use trailing commas in multi-line arrays and argument lists
Order array keys alphabetically where applicable
Use PHPDoc only when necessary (e.g., @var Collection)
Group class elements in order: constants, properties, constructor, public, protected, private methods
Group getter and setter methods for the same properties together
Suffix interfaces with Interface and traits with Trait
Use use statements for all non-global classes
Sort use imports alphabetically and group by type (classes, functions, constants)
Files:
tests/Api/Shop/Checkout/CartTest.phpsrc/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.phpsrc/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php
🧬 Code graph analysis (4)
tests/Api/Shop/Checkout/CartTest.php (1)
tests/Api/JsonApiTestCase.php (2)
setUpDefaultPostHeaders(95-101)requestPost(169-182)
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
__construct(20-26)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
__construct(20-26)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php (1)
CartNotFoundException(18-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Static checks / PHP 8.4, Symfony ^7.3
- GitHub Check: Static checks / PHP 8.3, Symfony ^6.4
🔇 Additional comments (1)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
18-26: LGTM; verify test-message alignment to avoid brittleness.Implementation is fine. Optional constant for reuse:
final class ProductVariantNotFoundException extends UnprocessableEntityHttpException { + private const MESSAGE = 'Product variant with the given code has not been found.'; + public function __construct( ?\Throwable $previous = null, int $code = 0, array $headers = [], ) { - parent::__construct('Product variant with given code has not been found.', $previous, $code, $headers); + parent::__construct(self::MESSAGE, $previous, $code, $headers); } }The new API test expects a description containing “does not exist”; if this exception is the source, the message won’t match. Please confirm which layer produces that string and align either the message or the test for stability.
src/Sylius/Bundle/ApiBundle/Exception/CartNotFoundException.php
Outdated
Show resolved
Hide resolved
65f7e71 to
1e5acc3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/Api/Shop/Checkout/CartTest.php (1)
675-678: Message assertion brittle; align with new exception text or use a tolerant match.ProductVariantNotFoundException says “has not been found.” This “does not exist” check may fail.
Use a regex tolerant to both phrasings:
- $this->assertStringContainsString('does not exist', $response['hydra:description']); + $this->assertMatchesRegularExpression('/(does not exist|has not been found)/i', $response['hydra:description']);Alternatively, assert only the 422 code and machine fields.
🧹 Nitpick comments (4)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php (1)
28-29: Sort imports alphabetically for consistency.Place ProductVariantNotFoundException before UnprocessableCartException.
As per coding guidelines.
-use Sylius\Bundle\ApiBundle\Exception\UnprocessableCartException; -use Sylius\Bundle\ApiBundle\Exception\ProductVariantNotFoundException; +use Sylius\Bundle\ApiBundle\Exception\ProductVariantNotFoundException; +use Sylius\Bundle\ApiBundle\Exception\UnprocessableCartException;src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
17-25: Sort use imports alphabetically.Reorder for consistency.
As per coding guidelines.
-use Sylius\Bundle\ApiBundle\Command\Cart\BlameCart; -use Sylius\Bundle\ApiBundle\Exception\UnprocessableCartException; +use Sylius\Bundle\ApiBundle\Command\Cart\BlameCart; +use Sylius\Bundle\ApiBundle\Exception\UnprocessableCartException; use Sylius\Component\Core\Model\OrderInterface; use Sylius\Component\Core.Model\ShopUserInterface; use Sylius\Component\Core\Repository\OrderRepositoryInterface; use Sylius\Component\Order\Processor\OrderProcessorInterface; use Sylius\Component\User\Repository\UserRepositoryInterface; -use Symfony\Component\HttpKernel\Exception\ConflictHttpException; -use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; +use Symfony\Component\HttpKernel\Exception\ConflictHttpException; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; use Symfony\Component\Messenger\Attribute\AsMessageHandler;src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php (2)
26-28: Sort imports alphabetically.Keep Symfony exception imports ordered and grouped.
-use Sylius\Bundle\ApiBundle\Exception\UnprocessableCartException; -use Symfony\Component\HttpKernel\Exception\ConflictHttpException; -use Symfony\Component\HttpKernel\Exception\NotFoundHttpException; +use Sylius\Bundle\ApiBundle\Exception\UnprocessableCartException; +use Symfony\Component\HttpKernel\Exception\ConflictHttpException; +use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
85-101: Good: tests assert specific HTTP-friendly exceptions.Expectations for 409/422/404 paths align with handler behavior. Optionally stub null returns explicitly for readability.
public function testThrowsAnExceptionIfCartHasNotBeenFound(): void { $this->shopUserRepository->expects(self::once()) ->method('findOneByEmail') ->with('sylius@example.com') ->willReturn($this->user); + $this->orderRepository->expects(self::once()) + ->method('findCartByTokenValue') + ->with('TOKEN') + ->willReturn(null); self::expectException(UnprocessableCartException::class);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php(1 hunks)src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php(1 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php(3 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php(3 hunks)tests/Api/Shop/Checkout/CartTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{php,yaml,yml,xml,twig}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation in PHP, YAML, XML, and Twig files
Files:
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.phpsrc/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.phpsrc/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.phptests/Api/Shop/Checkout/CartTest.phpsrc/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php
**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
**/*.php: Use modern PHP 8.2+ syntax and features
Declare strict_types=1 in all PHP files
Follow the Sylius Coding Standard
Do not use deprecated features from PHP, Symfony, or Sylius
Use final for all classes, except entities and repositories
Use readonly for immutable services and value objects
Add type declarations for all properties, arguments, and return values
Use camelCase for variables and method names
Use SCREAMING_SNAKE_CASE for constants
Use fast returns instead of unnecessary nesting
Use trailing commas in multi-line arrays and argument lists
Order array keys alphabetically where applicable
Use PHPDoc only when necessary (e.g., @var Collection)
Group class elements in order: constants, properties, constructor, public, protected, private methods
Group getter and setter methods for the same properties together
Suffix interfaces with Interface and traits with Trait
Use use statements for all non-global classes
Sort use imports alphabetically and group by type (classes, functions, constants)
Files:
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.phpsrc/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.phpsrc/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.phpsrc/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.phptests/Api/Shop/Checkout/CartTest.phpsrc/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php
tests/**/*Test.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PHPUnit for unit and functional testing
Files:
tests/Api/Shop/Checkout/CartTest.php
🧬 Code graph analysis (6)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php (2)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
UnprocessableCartException(18-27)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
__invoke(41-64)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php (2)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
UnprocessableCartException(18-27)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
ProductVariantNotFoundException(18-27)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
__construct(20-26)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
UnprocessableCartException(18-27)
tests/Api/Shop/Checkout/CartTest.php (1)
tests/Api/JsonApiTestCase.php (2)
setUpDefaultPostHeaders(95-101)requestPost(169-182)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
__construct(20-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Packages / Get matrix
- GitHub Check: End-to-end tests (MySQL) / Get matrix
- GitHub Check: End-to-end tests (PostgreSQL) / Get matrix
- GitHub Check: End-to-end tests (MariaDB) / Get matrix
- GitHub Check: Frontend / Get matrix
🔇 Additional comments (6)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php (2)
104-121: Good: dedicated HTTP-friendly exception asserted.Using UnprocessableCartException aligns with 422 semantics for unprocessable POST.
89-102: Disregard this review comment; the handler checks the product variant before the cart.The test is correct as written. The
AddItemToCartHandler.__invoke()method checks the product variant at line 48, throwsProductVariantNotFoundExceptionif null at lines 50-51, and only then checks the cart at line 55. Since the variant lookup happens first, the test will consistently throw the expected exception when the variant is not found, regardless of cart state. Stubbing the cart is unnecessary and adds noise.Likely an incorrect or invalid review comment.
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php (1)
47-58: Correct HTTP semantics for error cases.404 for missing user, 422 for missing cart, and 409 for occupied cart are appropriate replacements for generic exceptions.
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
18-27: LGTM: focused 422 exception with stable message.Final class, strict types, and concise constructor look good.
tests/Api/Shop/Checkout/CartTest.php (1)
639-650: Good coverage for non-existing cart → 422.Assertion aligns with UnprocessableCartException message and status.
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php (1)
18-27: LGTM! Clean exception implementation.The exception class follows all coding guidelines and is consistent with the existing
UnprocessableCartExceptionpattern. The implementation correctly:
- Uses
finalclass modifier- Declares strict types
- Provides proper type hints for all parameters
- Uses trailing commas in multi-line parameter lists
- Returns HTTP 422 (Unprocessable Entity) via the parent class, aligning with the PR objective to replace 500 errors with appropriate 4xx codes
The fixed error message clearly identifies the issue for API consumers.
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantNotFoundException.php
Outdated
Show resolved
Hide resolved
1e5acc3 to
7fe2023
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/Api/Shop/Checkout/CartTest.php (1)
652-678: Critical mismatch between exception message and test expectation.The test expects
hydra:descriptionto contain "does not exist" (line 677), butProductVariantUnprocessableException(lines 20-26 of ProductVariantUnprocessableException.php) defines the message as "Product variant could not be processed." This mismatch will cause the test to fail.Apply this diff to align the test with the actual exception message:
- $this->assertStringContainsString('does not exist', $response['hydra:description']); + $this->assertStringContainsString('Product variant could not be processed', $response['hydra:description']);Alternatively, if "does not exist" is the intended message, update the exception:
In ProductVariantUnprocessableException.php:
- parent::__construct('Product variant could not be processed.', $previous, $code, $headers); + parent::__construct('Product variant does not exist.', $previous, $code, $headers);
🧹 Nitpick comments (1)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
20-26: Consider a more specific error message for consistency.The message "Product variant could not be processed." is less specific than the sibling exception
UnprocessableCartException, which uses "Cart with given token has not been found." For consistency and clarity, consider a more specific message.Apply this diff for better clarity and consistency with
UnprocessableCartException:- parent::__construct('Product variant could not be processed.', $previous, $code, $headers); + parent::__construct('Product variant with given code has not been found.', $previous, $code, $headers);This also resolves the test expectation mismatch flagged in tests/Api/Shop/Checkout/CartTest.php line 677.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php(2 hunks)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php(1 hunks)src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php(1 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php(3 hunks)src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php(3 hunks)tests/Api/Shop/Checkout/CartTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php
- src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/BlameCartHandlerTest.php
- src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/BlameCartHandler.php
- src/Sylius/Bundle/ApiBundle/CommandHandler/Cart/AddItemToCartHandler.php
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{php,yaml,yml,xml,twig}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation in PHP, YAML, XML, and Twig files
Files:
tests/Api/Shop/Checkout/CartTest.phpsrc/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.phpsrc/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php
tests/**/*Test.php
📄 CodeRabbit inference engine (AGENTS.md)
Use PHPUnit for unit and functional testing
Files:
tests/Api/Shop/Checkout/CartTest.php
**/*.php
📄 CodeRabbit inference engine (AGENTS.md)
**/*.php: Use modern PHP 8.2+ syntax and features
Declare strict_types=1 in all PHP files
Follow the Sylius Coding Standard
Do not use deprecated features from PHP, Symfony, or Sylius
Use final for all classes, except entities and repositories
Use readonly for immutable services and value objects
Add type declarations for all properties, arguments, and return values
Use camelCase for variables and method names
Use SCREAMING_SNAKE_CASE for constants
Use fast returns instead of unnecessary nesting
Use trailing commas in multi-line arrays and argument lists
Order array keys alphabetically where applicable
Use PHPDoc only when necessary (e.g., @var Collection)
Group class elements in order: constants, properties, constructor, public, protected, private methods
Group getter and setter methods for the same properties together
Suffix interfaces with Interface and traits with Trait
Use use statements for all non-global classes
Sort use imports alphabetically and group by type (classes, functions, constants)
Files:
tests/Api/Shop/Checkout/CartTest.phpsrc/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.phpsrc/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php
🧠 Learnings (1)
📚 Learning: 2025-09-01T12:06:56.917Z
Learnt from: CR
Repo: Sylius/Sylius PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-01T12:06:56.917Z
Learning: Applies to tests/**/*Api*.php : Use PHPUnit tests to validate API configuration and API responses
Applied to files:
tests/Api/Shop/Checkout/CartTest.php
🧬 Code graph analysis (3)
tests/Api/Shop/Checkout/CartTest.php (1)
tests/Api/JsonApiTestCase.php (2)
setUpDefaultPostHeaders(95-101)requestPost(169-182)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php (2)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
UnprocessableCartException(18-27)src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
ProductVariantUnprocessableException(18-27)
src/Sylius/Bundle/ApiBundle/Exception/ProductVariantUnprocessableException.php (1)
src/Sylius/Bundle/ApiBundle/Exception/UnprocessableCartException.php (1)
__construct(20-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Packages / Get matrix
- GitHub Check: End-to-end tests (PostgreSQL) / Get matrix
- GitHub Check: Frontend / Get matrix
- GitHub Check: End-to-end tests (MariaDB) / Get matrix
- GitHub Check: End-to-end tests (MySQL) / Get matrix
🔇 Additional comments (2)
src/Sylius/Bundle/ApiBundle/tests/CommandHandler/Cart/AddItemToCartHandlerTest.php (1)
28-29: LGTM! Exception expectations properly updated.The test expectations have been correctly updated to use the new domain-specific HTTP exceptions (
UnprocessableCartExceptionandProductVariantUnprocessableException) instead of genericInvalidArgumentException, aligning with the PR's goal of providing proper 4xx error codes.Also applies to: 96-96, 115-115
tests/Api/Shop/Checkout/CartTest.php (1)
626-650: LGTM! Properly validates 422 response for non-existent cart.This test correctly verifies that attempting to add an item to a non-existent cart returns HTTP 422 with the appropriate error message from
UnprocessableCartException.
Summary
Changes
CartNotFoundExceptionreturning HTTP 422 for non-existent cartsProductVariantNotFoundExceptionreturning HTTP 422 for invalid product variantsAddItemToCartHandlerto throw dedicated exceptions instead of InvalidArgumentExceptionBlameCartHandlerto use ConflictHttpException (409) for cart conflictsWhy 422 instead of 404?
For POST operations that cannot be processed due to missing resources, 422 Unprocessable Entity is more semantically correct than 404 Not Found, as it indicates the request cannot be processed rather than the endpoint not existing.
Fixes #18475
Summary by CodeRabbit
Bug Fixes
Tests