[RFC] [POC] [Cart] Introduce CartModifier service #5970
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lchrusciel
force-pushed
the
cart-modifier-poc
branch
from
8ab7414
to
89165d8
Compare
| * @param OrderInterface $cart | ||
| * @param CartItemInterface $cartItem | ||
| */ | ||
| public function addToCart(OrderInterface $cart, CartItemInterface $cartItem); |
lchrusciel
force-pushed
the
cart-modifier-poc
branch
2 times, most recently
from
b1736e0
to
7c0d10c
Compare
lchrusciel
changed the title
pjedrzejewski
added
Enhancement
lchrusciel
force-pushed
the
cart-modifier-poc
branch
3 times, most recently
from
d13b3e8
to
72c03a9
Compare
| * @param CartInterface $cart | ||
| * @param CartItemInterface $item | ||
| */ | ||
| private function resolveCartItem(CartInterface $cart, CartItemInterface $item) |
There was a problem hiding this comment.
Does it only resolve the cart item? Or resolves and merges with the passed one if found? It would probably need some better naming.
There was a problem hiding this comment.
As far as we won't came up with a better idea, I think it could be merged and I will create an issue with naming.
lchrusciel
force-pushed
the
cart-modifier-poc
branch
from
72c03a9
to
0d5c90f
Compare
lchrusciel
force-pushed
the
cart-modifier-poc
branch
from
0d5c90f
to
b3e879b
Compare
lchrusciel
force-pushed
the
cart-modifier-poc
branch
from
b3e879b
to
6591dba
Compare
|
Good job Łukasz! 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Concept
Small proof of concept / RFC. In this PR I have extracted logic of adding a cart item from CartItemController to a separate service. This service will be responsible for determining if the new cart item should be added or just increase number of units in some existing one, and then it will recalculate the whole cart.
Profit
Such a solution ensure us, that we have only one true way of adding new items, where we are sure that whole cart will be recalculated after each addition. It can be also useful If one would like to add
Order nowfeature.Idea
I would like to decouple Sylius logic as much as it is possible form symfony events. They are nice, but only for end users. I am pretty sure, that managing this kind of logic with regular service is much better approach.
The problem with the previous implementation is, that order recalculation is handled via Cart event
SyliusCartEvents::CART_CHANGE.TODOs for now:
SyliusCartEvents::CART_CHANGErecalculation (at least for cart add/remove actions)Ideas for next iteration
OrderRecalculationListenerCartModifier::addToCart(). By default it could take a cart from context, but if one would need it, it will be possible to change the way, how the cart is resolved.Edit
I have change names of events for add/remove actions, because removing
OrderRecalculationListenerin this PR would result in ugly diff and also could create some problems in other system parts.