Add transactional helper to DocumentManager #2605
Conversation
|
All this conditionality ("would") in your comment makes me think that the implementation is not ideal. |
| public function transactional(Closure $closure) | ||
| { | ||
| $return = $closure(); | ||
| $this->flush(['withTransaction' => true]); |
There was a problem hiding this comment.
Given the simplicity of this implementation, I don't see what the point of a transactional() helper is instead of just having users opt into transactions when calling flush().
There was a problem hiding this comment.
The only benefit would be for discoverability.
There was a problem hiding this comment.
And being on par with ORM :) 👍 from me
There was a problem hiding this comment.
To start, I see nothing nothing fundamentally wrong with this PR (the code seems very simple and correct).
The requirement to nest everything in a closure seems more cumbersome than a call to flush() (or even a new flushUsingTransaction() method) that can otherwise be done in any context.
Per the PR description, it seems like this API is needed in ORM because the SQL transaction must be started before any operations are executed. I assume that allows ORM to avoid the conflict potential that @alcaeus alluded to in #2606 (re: still needing a locking mechanism to avoid data loss). Presumably, ORM could start/commit the transaction during the flush since that's where all of the SQL is executed, but it would invite the same issue we have with ODM due to MongoDB's transaction API.
But if this is unavoidable in ODM, I don't see what benefit this closure API has for the user. If discoverability is the main concern, I think there are better APIs for that. And I'd argue consistency with ORM's API when the implementation and behavior are very different ultimately makes it easier for users to shoot themselves in the foot (I'd wonder how many people are going to consider the caveats mentioned in #2606).
There was a problem hiding this comment.
After discussing this with the rest of the team, we've decided not to add the helper at this time.
While parity with the ORM would be good, this parity is not possible with the way transactions work in ODM. The transactional helper would always have to do things differently - either by invoking the callback multiple times (so the transaction can be restarted in case of errors), or by invoking it once and flushing afterwards which we've done in this PR. Of course, the callback itself would have to be different as well, as the session needs to be explicitly passed to any operation that is meant to be part of the transaction, which is not the case in ORM as the transaction will affect the entire connection.
This PR adds a
transactionalmethod to theDocumentManagerclass, similar to the one present in ORM'sEntityManager. However, the transaction is not started before closure execution, but only during theflushoperation. This is due to MongoDB's different handling of transactions, which would entail invoking the closure multiple times (which does not happen in ORM). It would also require us to add detection logic in UnitOfWork to not start a new transaction is already in progress (which can lead to issues in other places), and it would also require a different closure signature, as the transaction is not bound to the connection but rather to a session, which would then have to be passed to all operations that happen within a transaction.