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

feat: trimmed text option values and removed empty inputs #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

seizan8
Copy link

@seizan8 seizan8 commented Oct 27, 2022

Inputs for text options are now trimmed.
And empty inputs do not create CustomerItemOptions anymore (inputs like " " that only contain whitespace chars)

@mamazu
Copy link
Collaborator

mamazu commented Oct 29, 2022

Thanks for your contribution. However this would change the way that this plugin works a little bit.

And looking back at the implementation I don't think that this is the smartest way to go about that. To me it would make much more sense if this would be handled in the form that accepts the request (aka. ShopCustomerOptionType). There we could just attach the correct data transformer to the field and be done with it, I think. This would also help the logic to be more modular and extensible. Since now you can't add any new logic to your own customer option types without modifying this listener class.

What do you think?

@seizan8
Copy link
Author

seizan8 commented Nov 1, 2022

I looked at the addToCart process of sylius (Sylius\Bundle\OrderBundle\Controller\OrderItemController) and the customerOptionsPlugin stuff around it. Now my head hurts.

I'll use B24 as shortcut for Brille24 and S for Sylius classes, objects events and so on.

Right now the S-AddToCartType is extended by a B24-ShopCustomerOptionType Form, which generates the options for this plugin. When the form is submitted, the form is validated. Then the S-CartActions::ADD is dispatched with the configuration and orderItem. The B24-AddToCartListener listens to that event, reads the user input from the request and generates the B24-OrderItemOption. The B24-OrderItemOption gets added to the S-OrderItem when it's created.

This seemed weird to me before when I made this PR. Because the user input is not taken from the form, and from the request instead. However, Sylius does not pass the form in the CartActions::ADD event. Which would be pretty nice for this case.

If I understand this correct @mamazu , you are suggesting a DataTransformer in the B24-AddToCartListener. Said Listener would transform the CustomerOptions values into a B24-OrderItemOption object and add it to the S-OrderItem. Hence, removing the need for the B24-AddToCartListener to do that. Correct?

Since now you can't add any new logic to your own customer option types without modifying this listener class.

What do you mean by that? Do you mean adding a new customer option type? Or changing the behavior of them? I think it doesn't matter that much. You either have to update the Listener or the Transformer most likely.

@mamazu
Copy link
Collaborator

mamazu commented Nov 2, 2022

You are correct, this plugin is a hot mess. It was our first. But I am working on refactoring it a little as well. :)

You are completely correct both with your analysis (it makes no sense to take the values directly from the request and not the form) as well as what I have planned. With the data provider we could also reuse Symfony stuff, like the DataTransformer for the date and datetime fields.

What I meant with the extensibility: In the current implementation if you want to add a new Customer Option Type you have to either decorate or extend the AddToCartListener. This could also be mitigated with the migration to the form DataTransformers in the Merge Request.

@seizan8
Copy link
Author

seizan8 commented Nov 3, 2022

Alright alright.

I don't have much time on hand right now. I will try to update my PR tho. Hopefully, it won't take too much time.

As for the extensibility. I am not sure if I agree. If we use DataTransformer instead of a Listener, it would be easier to understand. Changing/Extending the form chould still require an overwrite of the Transformer tho. Or can you register multiple DataTransformers? Don't think I've ever done that. That sounds messy. I could add some events into transformer, so one could easily hook in and adjust it. If need be. Not sure if that's a great idea tho.

@mamazu
Copy link
Collaborator

mamazu commented Nov 5, 2022

So the idea with the transformer would be to have a registry where you can easily replace services, remove and add them. This way we could also have an easy way of adding new transformers without having to override anything. If you need a hint with the implementation just let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants