doctrine / data-fixtures

Doctrine2 ORM Data Fixtures Extensions
http://www.doctrine-project.org
MIT License
2.76k stars 224 forks source link

Add singleTransaction option for ORMExecutor #388

Closed BafS closed 2 years ago

BafS commented 2 years ago

Reworked https://github.com/doctrine/data-fixtures/pull/209 to target 1.6.x and use func_get_args() to avoid BC use the constructor to avoid BC

stof commented 2 years ago

This PR needs tests covering the case of not using a single transaction

BafS commented 2 years ago

This PR needs tests covering the case of not using a single transaction

Thanks, I added some tests, let me know if it's enough.

stof commented 2 years ago

The test is exactly the same than the test not using multiple transactions. So to me, this means that the test has no value. If I revert all changes in the ORMExecutor::execute method, your test would still be passing.

BafS commented 2 years ago

I rebased my PR to use $this->em->wrapInTransaction() instead of $this->em->getConnection()->transactional() and added some tests to count the number of "transactional" calls

greg0ire commented 2 years ago

Please add documentation about the new feature.

greg0ire commented 2 years ago

@derrabus If you prefer to have MultipleTransactionORMExecutor extends AbstracExecutor, that's fine by me. I think there is a need for a design change here, but it might best be done outside this PR, and go further than what has been done here. Let us know what you think.

BafS commented 2 years ago

I updated the PR to make MultipleTransactionORMExecutor extends ORMExecutor, which is probably the easiest way to go but not the nicest (because inheritance). If you prefer I can extends the AbstracExecutor and share the logic with trait (which is not so nice either IMO). Not trivial to come up with a nice design without BC.

derrabus commented 2 years ago

Let me explore that path with the shared trait. I'll get back to you. And sorry for the back-and-forth on this PR. 😓

BafS commented 2 years ago

@derrabus Okay 👍 no problem, it's great to have feedback and to see things moving

derrabus commented 2 years ago

Here's my take: #396

derrabus commented 2 years ago

Closing in favor of #396.