Introduce Factory for all resources #3613
Conversation
pjedrzejewski
added
BC Break
|
👍 looks sensible to me, although difficult to see the bigger picture It took you 8 - 12 hours to do find/replace? |
|
@peteward I wish it was as simple as find replace. You need to update specs etc. but sometimes it does not mean only replacing Repository with a Factory. Sometimes you need both etc. I think 8-12 hours is too much, was just a shot to give some idea how difficult upgrade is. |
|
I was joking ;) good work |
|
Green! 🎉 |
|
Good job 👍 Related PR: #3480 :) |
| /** | ||
| * @author Paweł Jędrzejewski <pawel@sylius.org> | ||
| */ | ||
| class TranslatableFactory extends Factory implements TranslatableFactoryInterface |
There was a problem hiding this comment.
Can we make this a decorator for FactoryInterface? Doing so, we can write specs for setting both current locale and fallback locale.
There was a problem hiding this comment.
Anyway, if we rewrite this class as decorator, it is possible to make some vendor class translatable even without changing its factory code.
There was a problem hiding this comment.
I know there was a reason why I have not used a decoration here, can't recall now. Will try to do this and see how it works out, should not be that hard to change.
There was a problem hiding this comment.
If there is nothing against going for decorator pattern, we should pick this direction, because it definitely makes sense here.
|
@pjedrzejewski great job! 👍 Small step that starts the big improvement, looking forward to see more! |
|
Applied all changes. Good to go! :) Woohoo! |
Introduce Factory for all resources
|
@pjedrzejewski one more thing left, global upgrade and changelog update :) |
This is first step extracted from #2255. The original PR became too big and unmergeable and very hard to maintain for me. It was not the best move from my side to make it that big, but it gave me a good insight into how I want SyliusResourceBundle to look like.
This is the first step, which will effectively allow us to fix #480, #3480 and probably much more bugs. Also, it makes everything much cleaner and we can add more methods to our Factories. For now, I kept only createNew by default (except InventoryUnitFactory, which required changes) because I want to keep this PR as slim as possible.
Main thing here is that we can easily inject stuff into Factories, which was not possible/simple/clean with Repository taking care of object creation. I updated all tests, specs and also tracked down
createNewcalls in stuff that has no coverage of specs/tests/scenarios.This is a BC break, pretty big one, but this PR took me maybe 8-12 hours to update whole Sylius, so it should be a pretty straightforward upgrade. :)
Review appreciated!
QA: