search

Brille24 / SyliusCustomOptionsPlugin

A Sylius plugin that adds customer options
MIT License
47 stars 34 forks source link

Feat/customer option recalculator events #92

Closed seizan8 closed 2 years ago

seizan8 commented 3 years ago

So, In our project used the CustomerOptionsPlugin to add multiple options to our products. But we needed the text options to impact the price. The customers basically could order a specific length of our product and depending on the length it costed a different amount. So, I rewrote the CustomerOptionRecalculator and threw some events. For which I then created subscribers to add custom adjustments for specific CustomerOptions.

I can see why only the select options can be configured to add adjustments. However, I felt it should be easier to customize the other types too. So, I updated the CustomerOptionRecalculator, added events and whatnot. And also updated the readme. If anyone likes this Bundle, but needs custom adjustments for specific CustomerOptions this should have customizing it.

I also wanted to add a tag, so you could use Listeners. However, I don't have much experience with CompilerPasses and I couldn't get it to work in the end. That's why I used subscribers. Ideally support for listeners would be added later.

mamazu commented 3 years ago

First of all thank you for your contribution. It looks very good. The event listener would work the same but you'd have to give the full classname in the configuration file. So I think an event subscriber is better in this case as refactoring tools can better pick up the class name and namespace changes. Two things though: Could you please rebase the branch onto the current master. I have updated the tests and they now run on Github. Talking about tests. We might want to have a unit test for the new logic.

seizan8 commented 3 years ago

I haven't had many experiences with contributing to projects on github. I can fix my branch and would like to. I just don't really understand what I did wrong. You want me to rebase the feat branch to the master, right? Why do I have to do this? Did I screw up somewhere or do I always have to rebase my branch before creating a PR?

A unit test for the subscribers you mean? I thought about that too. I don't have much experience with unit tests and wasn't sure how to test it. Otherwise I would have added a simple test too.

mamazu commented 3 years ago

Sure, no problem. I think the reason you have problems with rebasing your branch is that one of your commits is a merge commit of the master. Merge and rebase donw work very well together. But you can also use merge. We don't use require rebase for our project.

About the tests, do you have any specific questions? There should be already tests with PHPUnit in the project. But if you have any questions feel free to ask.

mamazu commented 2 years ago

Many thanks for your pull request. I have salvaged what I could and made a new merge request #103 If you feel like there is still something missing feel free to open a new pull request with your changes.

seizan8 commented 2 years ago

I checked the changes and don't see anything missing. I think the 2 events should suffice. The events I had were a bit excessive in hindsight. Thx for adding the events!

mamazu commented 2 years ago

Thanks for bringing the topic up. This plugin was developed for a specific use-case and never had extensibility in mind. So, thanks for bringing this topic up.