[Fixtures] Brand new fixtures bundle #5219
Conversation
pamil
force-pushed
the
fixtures-bundle
branch
2 times, most recently
from
85318c7
to
fa7f116
Compare
| $locale = $this->localeFactory->createNew(); | ||
|
|
||
| $locale->setCode($localeCode); | ||
| $locale->setEnabled(true); |
There was a problem hiding this comment.
If it's hardcoded, maybe use $locale->enable()?
There was a problem hiding this comment.
It won't in the near future
|
This seems very lovely but please don't forget to show some love to docs and write a decent cookbook before replacing simple and straightforward code with something more complex. In the meantime i'll try to do something about the docs myself :) |
| * @param ObjectManager $currencyManager | ||
| * @param string $baseCurrencyCode | ||
| */ | ||
| public function __construct(FactoryInterface $currencyFactory, ObjectManager $currencyManager, $baseCurrencyCode) |
There was a problem hiding this comment.
Is the ObjectManager really a currency manager? would call it simply $objectManager
There was a problem hiding this comment.
ObjectManager is so broad that it actually covers nothing. ObjectManager is just a common interface for all persistence layers in Sylius.
There was a problem hiding this comment.
well, currency manager implies that it manages currency. what it acutally is is an entity/document manager abstracted as ObjectManager, which can manage any entity/document.
There was a problem hiding this comment.
Nevertheless we shouldn't use it (sylius.manager.currency) to manage something else than currency.
pamil
force-pushed
the
fixtures-bundle
branch
2 times, most recently
from
ed270f1
to
c099a47
Compare
| */ | ||
| protected function configureOptionsNode(ArrayNodeDefinition $optionsNode) | ||
| { | ||
| // no options |
There was a problem hiding this comment.
We could allow configuring some specific country codes and load all if none provided. WDYT?
There was a problem hiding this comment.
There's still an issue if some country codes would be used by Province / Zone fixtures while these countries wouldn't exist in the database.
There was a problem hiding this comment.
That's why I think we could really have this geographical_fixture, which defines zones like this:
zones:
USA:
countries: US
EU:
countries: PL, DE, FR
And drop the country fixture entirely or keep it.
pamil
force-pushed
the
fixtures-bundle
branch
2 times, most recently
from
634cd97
to
ef67e28
Compare
pamil
force-pushed
the
fixtures-bundle
branch
5 times, most recently
from
0499f8e
to
277bb42
Compare
| /** | ||
| * @var FixtureLoaderInterface | ||
| */ | ||
| private $baseFixtureLoader; |
pamil
changed the title
|
Boom! 💥 Nice work Kamil! Thanks! |
|
My god, such blazing speed. Great work! |
It's really a work in progress, but feel free to review 🎉 It will be a standalone bundle, not depending on any of Sylius components or bundles.