Fix to allow adding products with a customer option to the cart

[shochdoerfer]

Fixes #131. I haven't yet added or fixed the tests as I wanted first to get the OK to go this route. Worked fine in my test environment.

Initially, the plan was to use the Pre Add Cart event, but somehow that event also fired too late, which meant the needed data was not available in the equals() method. So the only option I found was to implement a custom CartItemFactory and configure Sylius to use that instead of the default implementation. Sadly, the Sylius default implementation is marked with final which means, I had to c&p the default logic and customize it with the logic from the AddToCartListener. This could potentially lead to problems when Sylius decides to modify their default implementation.

[shochdoerfer]

Ideally, we could improve the equals() method a bit. I am thinking of introducing a checksum for the OrderItemOptionInterface, but I couldn't think of a reliable algorithm. That way we could simply compare the checksum of both objects without constantly iterrating over the configuration arrays.

[mamazu]

First of all thanks for your contribution. It makes sense to move this to the OrderItemFactory. There are just two things I would suggest changing:

Use the decoration

I don't know if you have heard of the concept of decorating a class. Here is the symfony documentation for it: https://symfony.com/doc/current/service_container/service_decoration.html

But in essence what you want is you want to add functionality to a class but still keep the old functionality like so:

class NewFactory {
    public function __construct(private FactoryInterface $oldFactory) {}

    public function something() {
        // Some code before
        $this->oldFactory->something();
        // Some code after
    }
} 

If you need help with that just ask again. But that could avoid code duplication.

Check where the createForProduct is used

I am not sure that this method is the best place for this as this could also be used by fixtures as well. Yes, it doesn't break but I think a method named createForProductWithCustomerOptions would probably be more fitting in naming and might even allow you to make the thing more request specific.

[shochdoerfer]

@mamazu thx for your feedback.

I've read about service decoration but assumed since the Sylius class is final it might not work. Will give it a try and report back.

createForProduct is used by the Sylius configuration for these routes:

• sylius_paypal_plugin_create_paypal_order_from_product
• sylius_admin_product_variant_create
• sylius_shop_ajax_cart_add_item
• sylius_shop_partial_cart_add_item

In code createForProduct is used in the ProductVariantGenerator class and the respective spec files.

I'd be fine doing the additional checks in the createForProduct method, if you feel it makes more sense to have a custom method, I can tackle that.

[mamazu]

Yeah in the list of usages: sylius_admin_product_variant_create doesn't really fit in there.

That's why I thought that this might not be the right place to add it. I am not sure if there is a better way to add info onto cart items.

[shochdoerfer]

Ok, then I'll add a new method in the factory and add the configuration to overwrite the factories for the sylius_shop_ajax_cart_add_item and sylius_shop_partial_cart_add_item routes. I think we can ignore sylius_paypal_plugin_create_paypal_order_from_product as this only takes the product Id as input, if I remember correctly.

[mamazu]

That sounds like a better approach to be honest. Sorry I am not much help with it since it's been a long time and we only use this bundle headless these days.

[shochdoerfer]

No worries. At least we have a plan now. Need to check when I have more time to finalize this.

[shochdoerfer]

Quick update: I still need to find the time to finish the PR. It's still on my todo list, but it will take a while until I am able to tackle this.

[shochdoerfer]

@mamazu I've updated/refactored the branch. Also, I've rebased the branch with the latest changes from master.

Decorating the default CartItemFactory was a bit tricky as I ran into a dependency cycle problem because I needed to decorate and overwrite the service at the same time (with the same id). That's why I must instanciate the default CartItemFactory in the custom CartItemFactory class I added to this project. Not ideal, but it works.

I've also updated the README with the route definitions to overwrite the Sylius ones to be able to call the createForProductWithCustomerOption factory method.

[mamazu]

Your merge request looks good. I am however a little concerned about your approach to the dependency injection. Could you explain what the issue is.
Because by default if you decorate a service then the old name will be redirected to the new service as well.

[mamazu]

Is there a reason we are not using constructor property promotion here?

[shochdoerfer]

Good question ;) Not sure why I went this way in the original PR but we could totally switch to constructor property promotion.

[mamazu]

That would be great if you could do that.

[mamazu]

We have a naming schema. brille24.factory.order_item would probably fit better.

[shochdoerfer]

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?

[seizan8]

@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.
Have you tried using the id @mamazu suggested? I think that should work just fine.

https://symfony.com/doc/6.4/service_container/service_decoration.html

[shochdoerfer]

@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.

[shochdoerfer]

Let me give you some more context why I did the things I did. The original service definition from Sylius looks like this:

<service id="sylius.custom_factory.order_item" class="Sylius\Component\Core\Factory\CartItemFactory" 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" />
</service>

I tried to decorate it like this:

<service
		class="Brille24\SyliusCustomerOptionsPlugin\Factory\CartItemFactory"
		id="sylius.custom_factory.order_item"
		decorates="sylius.custom_factory.order_item"
		decoration-priority="255"
		public="false"
>
	<argument type="service" id="sylius.custom_factory.order_item"/>
	<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>

That leads to the following error when Symfony tries to compile the container definition: An alias cannot reference itself, got a circular reference on "sylius.custom_factory.order_item". It felt the only option I have is to go the route I took but maybe I am missing something.

[mamazu]

Try this:

<service
		class="Brille24\SyliusCustomerOptionsPlugin\Factory\CartItemFactory"
                id="brille24.factory.order_item"
		decorates="sylius.custom_factory.order_item"
		decoration-priority="255"
		public="false"
>
	<argument type="service" id="brille24.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>

This will create a new service called brille24.factory.order_item and it will replace the sylius.custom_factory.order_item as well. To get access to the old service (the Sylius service) you can use brille24.factory.order_item.inner.

[shochdoerfer]

@mamazu I tried that but somehow the brille24.factory.order_item.inner reference gives me a Sylius\Component\Resource\Factory\FactoryInterface instance and not \Sylius\Component\Core\Factory\CartItemFactory. Not sure why. The sylius.custom_factory.order_item service seems to be marked with public=false - maybe that is why.

Anyway, I've pushed a few more commits incl. a test for the CartItemFactory class. Let me know what's more needed to get this PR merged.

[mamazu]

Yeah, this might be because Sylius also decorates this. But yeah, I'm happy to merge this either way now. The public=false is the default. This means that you can't access the service with $container->get(...) when the container is compiled.

[shochdoerfer]

Awesome! Thx for the good collaboration!

[t-n-y]

Thx a lot for the fix.
However, is there a reason why doctrine/dbal still conflict with version ^3.0 ?