Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cart][Core] Rework the logic behind the cart #5565

Conversation

Arminek
Copy link
Contributor

@Arminek Arminek commented Jul 21, 2016

Q A
Bug fix? yes
New feature? yes
BC breaks? yes
Related tickets #5197
License MIT

try {
$cart = $this->cartContext->getCart();
} catch (CartNotFoundException $exception) {
$cart = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return?

@Arminek Arminek force-pushed the experimental/cart-providing-refactoring branch from 01e2f5f to 874ca1c Compare July 21, 2016 11:38
@Mr-Negative
Copy link

Mr-Negative commented Jul 21, 2016

To have it more visible. Any chances / maybe it is worked on multiple orders per cart? #5566

@Arminek
Copy link
Contributor Author

Arminek commented Jul 22, 2016

@Mr-Negative IMO we should rework basic cart logic I don't think that multiple orders are our goal now.

@Arminek Arminek force-pushed the experimental/cart-providing-refactoring branch from 24e671b to e2aef46 Compare July 22, 2016 09:28
@@ -57,4 +63,5 @@
</call>
</service>
</services>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant

@Arminek Arminek force-pushed the experimental/cart-providing-refactoring branch from 37567b3 to 1d987fa Compare July 22, 2016 12:18
@Arminek Arminek force-pushed the experimental/cart-providing-refactoring branch 12 times, most recently from 6314dcc to ac2ec89 Compare July 26, 2016 14:01
@@ -58,19 +58,19 @@ protected function getCartSummaryRoute()
protected function getCurrentCart()
{
return $this
->getProvider()
->getContext()
->getCart()
;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can make it an one-liner.

@michalmarcinkowski michalmarcinkowski merged commit c29c050 into Sylius:master Jul 28, 2016
@michalmarcinkowski
Copy link
Contributor

Great job! Thanks @pjedrzejewski and @Arminek! 👍

@michalmarcinkowski
Copy link
Contributor

@Arminek please apply all comments in a separate PR.

@@ -36,39 +40,40 @@ function it_is_initializable()
}

function it_recalculates_cart_for_logged_in_user(
CartProviderInterface $cartProvider,
CartContextInterface $cartContext,
Event $event,
OrderInterface $order,
OrderRecalculatorInterface $orderRecalculator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be the second one (it's the collaborator injected into constructor).

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…ng-refactoring

[Cart][Core] Rework the logic behind the cart
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants