search

Brille24 / SyliusCustomOptionsPlugin

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

Turned entities into mapped-superclasses and optimized factories #83

Closed seizan8 closed 3 years ago

seizan8 commented 3 years ago

Entity config changes from #72 Overriding CustomerOptionValue. Also optimized factories as they are a bit annoying to extend right now.

mamazu commented 3 years ago

First of all thanks for the pull request.

Looks good and the tests also pass. However I am not sure if the factories should be extendable by inheritance. Doesn't the Sylius default say Composition? Or do you have a use case that requires inheritance?

seizan8 commented 3 years ago

Looks good and the tests also pass. However I am not sure if the factories should be extendable by inheritance. Doesn't the Sylius default say Composition? Or do you have a use case that requires inheritance?

I extended the entity and added a new property. Then I wanted to extend my fixtures. For that to work I need to update the factory. I could just overwrite it. However, this might cause problems later on after I update the bundle. Ideally, I could extend the base factory and call the parent method. Then just add my stuff at the end.

This is standard in sylius as well, see: https://docs.sylius.com/en/1.9/customization/fixtures.html

mamazu commented 3 years ago

If you want to extend the factory you should decorate it. This has the advantage of being easier to test and being good to upgrade. And the fixture classes those should be extensible but not the factories. The factories should be final and not extendable.

see: https://docs.sylius.com/en/1.8/customization/factory.html

seizan8 commented 3 years ago

Oh alright! That makes sense, agreed. I think the PR should be fine though. I don't see a downside to the properties being protected. And more importantly the added createNew calls. Without them one would need to overwrite all the methods if the entity got extended. So I think that method should definitely be used.

mamazu commented 3 years ago

Agreed, tagging a new version and then you should be able to extend the entities at your heart's content.