-
Notifications
You must be signed in to change notification settings - Fork 89
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
Fixing double carts #454
Fixing double carts #454
Conversation
@@ -71,8 +71,13 @@ | |||
</service> | |||
|
|||
<!-- Removing the create cart context from composite context (see: https://github.com/Sylius/Sylius/issues/10192) --> | |||
<service id="sylius.context.cart.new" class="Sylius\Component\Order\Context\CartContext"> | |||
<argument type="service" id="sylius.factory.order" /> | |||
<service id="sylius.shop_api_plugin.context.cart" class="Sylius\Component\Order\Context\CompositeCartContext" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should work without these services. May you test it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we need it. The compiler pass doesn't create the service but only uses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the service is created by another compiler pass, shouldn't it be available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No compiler pass creates it. The compiler pass I wrote just uses this and adds the other context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant \Sylius\Bundle\OrderBundle\DependencyInjection\Compiler\RegisterCartContextsPass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be that I could use this class to create the service as well. I just took the logic of the PrioritizedCompositeServicePass
and applied it here. This requires the service to exist though.
@@ -71,8 +71,13 @@ | |||
</service> | |||
|
|||
<!-- Removing the create cart context from composite context (see: https://github.com/Sylius/Sylius/issues/10192) --> | |||
<service id="sylius.context.cart.new" class="Sylius\Component\Order\Context\CartContext"> | |||
<argument type="service" id="sylius.factory.order" /> | |||
<service id="sylius.shop_api_plugin.context.cart" class="Sylius\Component\Order\Context\CompositeCartContext" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the service is created by another compiler pass, shouldn't it be available?
fa5ce57
to
8227ca7
Compare
Fixed in #490, thanks to Max for your work anyway :) |
In the last pull request #401 the cart provider was overridden however this fix is much more specific in that it replaces the cart context only in a specific point (the cart blamer) The cart blamer is used on login so it is also the point where the cart creation shouldn't occur. For a more indepth view on if this is the right part of where the override should happen, the Sylius Core team would need to give feedback. This however should prevent the Sylius frontend from crashing when using the ShopApi