add append fixtures feature to loadFixtures method #384
Conversation
almacbe
force-pushed
the
append-load-fixtures
branch
3 times, most recently
from
55c47ac
to
6feae49
Compare
|
Adding the option as a second parameter will break many calls to |
|
That was my thought |
|
Since this change is breaking BC, we should add this information in the upgrade guide and the changelog (see #382) |
|
Then, I am sorry but I don't know what I have to do... Do I have to wait? or do I have to modify the files? |
|
It's up to @alexislefebvre or @lsmith77 to decide if they want to merge this and do it later, or merge #382 and do it now in this same PR.. |
|
#382 is merged, you can pull from the target branch and add info in those files now! 👍 |
almacbe
force-pushed
the
append-load-fixtures
branch
from
6feae49
to
46a4943
Compare
|
I have updated the doc, changelog and update guide. I hope my English is good enough. |
|
@Jean85 Do I have to change or do something more? |
UPGRADE-2.0.md
Outdated
| @@ -8,3 +8,7 @@ This is the list of actions that you need to take when upgrading this bundle fro | |||
| composer remove --dev nelmio/alice | |||
| composer require --dev liip/functional-test-bundle "~2.0" | |||
| ``` | |||
|
|
|||
| * It had been changed the interface of `LoadFixtures` to allow append fixtures. The main difference is it had been added | |||
There was a problem hiding this comment.
just a small wording tweak
The interface of `LoadFixtures` had to be changed to allow append fixtures. The main difference is that a boolean second parameter has been added. You will have to add it and set it to `false` if you have changed the default manager, driver or purge mode.
There was a problem hiding this comment.
@lsmith77 Thanks, it is sorted.
| * @param string $omName The name of object manager to use | ||
| * @param string $registryName The service id of manager registry to use | ||
| * @param int $purgeMode Sets the ORM purge mode | ||
| * | ||
| * @return null|AbstractExecutor | ||
| */ | ||
| protected function loadFixtures(array $classNames = [], ?string $omName = null, string $registryName = 'doctrine', ?int $purgeMode = null): ?AbstractExecutor | ||
| protected function loadFixtures(array $classNames = [], bool $append = false, ?string $omName = null, string $registryName = 'doctrine', ?int $purgeMode = null): ?AbstractExecutor |
There was a problem hiding this comment.
generally this method has too many parameters already from my point of view. it would be good to think about how to refactor it to have less parameters.
There was a problem hiding this comment.
Since we are targeting 2.x and BC are allowed, it's a good moment to do that, yes! 👍
There was a problem hiding this comment.
But, does it have to be in this PR or another one?
almacbe
force-pushed
the
append-load-fixtures
branch
from
46a4943
to
3bca1f2
Compare
|
LGTM, I would approach something to simplify the signature in a different PR... Maybe I have something in mind. |
|
@alexislefebvre if this approach is ok for you, then I can live with it .. we should just immediate start this follow up PR or at least create a ticket and someone should feel somewhat responsible for taking it on. |
|
I prefer that we refactor @Jean85 You looked interested, feel free to open an issue or a PR about this refactor. 😉 |
Add an optional parameter that allows to append fixtures.
As we talked on #174 I have implemented a proposal for that feature.