-
Notifications
You must be signed in to change notification settings - Fork 34
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
Fix to allow adding products with a customer option to the cart #134
Changes from 2 commits
7455e15
1bf41e1
57e8236
ae7e804
65a2766
e579bdf
934783c
ff96a3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,5 +54,19 @@ | |
<argument type="service" id="brille24.repository.customer_option_value"/> | ||
<argument type="service" id="brille24.customer_options_plugin.factory.customer_option_value_price_factory" /> | ||
</service> | ||
|
||
<service | ||
class="Brille24\SyliusCustomerOptionsPlugin\Factory\CartItemFactory" | ||
id="sylius.custom_factory.order_item" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a naming schema. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO I need the Sylius ID here to overwrite the service definition. Other services rely on that ID, that's why I picked it. Or am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shochdoerfer this is a decorator. You do not need to overwrite the service ID. The decorator gets its own id. And when the service it decorates is used, the decorator is applied automatically. Or symfony replaces the old one with the decorated one automatically? Anyway, looking at the docs and recalling my experience with decorators, I am certain you do not overwrite the service ID in the config. You just need to configure what service you intend to decorate. https://symfony.com/doc/6.4/service_container/service_decoration.html There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @seizan8 that makes sense. Can't recall why it did not work for me or it looked like it is not working. I've fixed the issue locally already. Currently checking how to best fix the tests as things changed quite a bit. |
||
decorates="sylius.factory.order_item" | ||
decoration-priority="256" | ||
public="false" | ||
> | ||
<argument type="service" id="sylius.custom_factory.order_item.inner"/> | ||
<argument type="service" id="sylius.product_variant_resolver.default"/> | ||
<argument type="service" id="request_stack"/> | ||
<argument type="service" id="brille24.factory.order_item_option"/> | ||
<argument type="service" id="brille24.repository.customer_option"/> | ||
</service> | ||
</services> | ||
</container> |
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.
Is there a reason we are not using constructor property promotion here?
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.
Good question ;) Not sure why I went this way in the original PR but we could totally switch to constructor property promotion.
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.
That would be great if you could do that.