[WIP] Custom factory configuration fix #18434
Conversation
WalkthroughAdds optional factory-based resource instantiation to ResourceFormComponentTrait via new initialize parameters and adjusted createResource logic. Modifies OrderInventoryOperator::cancel to treat PARTIALLY_REFUNDED like PAID/REFUNDED for inventory return, altering the control path for that payment state. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Twig Component
participant R as ResourceFormComponentTrait
participant F as Factory (optional)
T->>R: initialize(repository, formFactory, resourceClass, formClass, factory?, factoryMethod?)
Note over R: Store factory and method if provided
T->>R: createResource()
alt factory provided
R->>F: factoryMethod() -> resource
F-->>R: New Resource
else no factory
R->>R: new resourceClass()
end
R-->>T: Resource instance
sequenceDiagram
autonumber
participant S as OrderInventoryOperator
participant O as Order
participant I as Inventory
S->>O: cancel(order)
O-->>S: getPaymentState()
alt state in {PAID, REFUNDED, PARTIALLY_REFUNDED}
S->>I: giveBack(order)
I-->>S: inventory returned
else other states
S->>I: release(order)
I-->>S: inventory released
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 deployment failed on BunnyshellSee: Environment Details | Pipeline Logs Check https://github.com/Sylius/Sylius/actions/runs/18467059344 for details. Available commands:
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Sylius/Component/Core/Inventory/Operator/OrderInventoryOperator.php (1)
24-37: Prevent over-returning stock on partial refunds
In cancel(), STATE_PARTIALLY_REFUNDED still calls giveBack(), which returns all items. No per-item refund tracking exists—implement tracking (e.g., refundedQuantity on OrderItem) and adjust cancel() to return only actually refunded items.
🧹 Nitpick comments (1)
src/Sylius/Bundle/UiBundle/Twig/Component/ResourceFormComponentTrait.php (1)
96-106: Validate factory method exists before dynamic invocation.The trait’s createResource() unconditionally calls
$this->factory->$factoryMethod()which can fail at runtime if a custom$factoryMethodis invalid or requires parameters. Add amethod_exists($this->factory, $factoryMethod)check (for example in initialize()) or constrain allowed methods to guarantee safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Sylius/Bundle/UiBundle/Twig/Component/ResourceFormComponentTrait.php(5 hunks)src/Sylius/Component/Core/Inventory/Operator/OrderInventoryOperator.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/Component/Core/Inventory/Operator/OrderInventoryOperator.phpsrc/Sylius/Bundle/UiBundle/Twig/Component/ResourceFormComponentTrait.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/Component/Core/Inventory/Operator/OrderInventoryOperator.phpsrc/Sylius/Bundle/UiBundle/Twig/Component/ResourceFormComponentTrait.php
⏰ 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). (19)
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.4, Symfony ^7.3, PostgreSQL 17.5
- GitHub Check: End-to-end tests (PostgreSQL) / Non-JS, PHP 8.2, Symfony ^6.4, PostgreSQL 15.13
- GitHub Check: Frontend / NodeJS 24.x
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.4, Symfony ^7.3, MariaDB 11.4.7, State Machine Adapter symfony_workflow, ApiPlatform ~4.1.7
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.4, Symfony ^7.3, MariaDB 11.4.7, State Machine Adapter symfony_workflow
- GitHub Check: End-to-end tests (MariaDB) / Non-JS, PHP 8.2, Symfony ^6.4, MariaDB 10.11.13, State Machine Adapter winzou_state_machine
- GitHub Check: Packages / PHP 8.3, Symfony ^7.3, ORM ^3.3
- GitHub Check: Packages / PHP 8.4, Symfony ^7.3
- GitHub Check: Packages / PHP 8.2, Symfony ^6.4
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.3, Symfony ^7.3 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.3, Symfony ^7.3 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.4, Symfony ^7.3 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.4, Symfony ^7.3 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.4, Symfony ^7.3 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / Non-JS, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Chromedriver, PHP 8.2, Symfony ^6.4 (test_cached), MySQL 8.0, Twig ^3.3
- GitHub Check: End-to-end tests (MySQL) / JS with Panther, PHP 8.3, Symfony ^7.3 (test_cached), MySQL 8.4, Twig ^3.3
- GitHub Check: WIP
🔇 Additional comments (4)
src/Sylius/Bundle/UiBundle/Twig/Component/ResourceFormComponentTrait.php (3)
17-17: LGTM!The import is correctly added and necessary for the new factory-based functionality.
48-51: LGTM with minor note.The properties are well-typed with appropriate defaults. The
$factoryMethod = 'createNew'default on line 51 is redundant since it's always set viainitialize(), but this doesn't cause issues and maintains consistency.
75-89: LGTM!The signature change maintains backward compatibility with optional parameters and appropriate defaults. The PHPDoc correctly documents the generic factory type.
src/Sylius/Component/Core/Inventory/Operator/OrderInventoryOperator.php (1)
24-37: Confirm inventory return logic for partially refunded orders
InOrderInventoryOperator::cancel()(line 28), addingSTATE_PARTIALLY_REFUNDEDroutes cancellation throughgiveBack(), which restocks all items.
- Should cancelling a partially-refunded order restock every item or only the refunded ones?
- No test covers
cancel()forSTATE_PARTIALLY_REFUNDED—please add one toOrderInventoryOperatorTest.- This inventory change isn’t related to “[WIP] Custom factory configuration fix”; consider splitting it into a dedicated PR or updating the title.
Summary by CodeRabbit
Bug Fixes
Improvements