Add SAVEPOINT (Fixes #56) #111
Conversation
|
Ping @lox |
|
Hey, sorry for the delay @Jud. Reviewing this and other patches now. |
| @@ -28,12 +28,21 @@ public function execute() | |||
| $this->results = array(); | |||
|
|
|||
| try { | |||
| $this->_connection->execute('BEGIN'); | |||
| $this->_connection->execute( | |||
There was a problem hiding this comment.
It's hard to see what's going on in this method at a glance.
There was a problem hiding this comment.
Agreed -- I think I wanted the changes to be as minimal as possible, but this cuts readability. I'll refactor it a bit.
|
What would you think of actually removing responsibility for issuing SQL entirely from the Transaction object? It already fires events, we could just have the Connection object listening for those events and converting them to SQL. That's probably a cleaner separation of responsibility. Then the API for transactions could remain the same and we could have a Strategy in the Connection to switch between plain old transactions or SAVEPOINTS. |
|
@lox moved the transaction sql into the |
|
I'm pretty happy with this, @harto any thoughts before merge? |
| /** | ||
| * A Transaction Stack that keeps track of open savepoints | ||
| */ | ||
| class TransactionStack |
There was a problem hiding this comment.
Is this actually a SavePointStack, or something like that? The current name implies that we push/pop Transaction objects.
There was a problem hiding this comment.
Yep, would be better as SavePointStack
|
Looks good. Just a couple of questions about naming, etc. |
before/afterStartTransaction to before/afterTransaction Also changed the methodology behind the commit event, which should only be called when the actual 'commit' query is sent and not when the savepoint is simply released.
Allows an event to be bound and then removed.
|
@lox might leave it to you to give this the final 👍 |
|
@lox fwiw, we've been using this in production since October without issue. This was the foundation for allowing all |
|
Closing in favor of #142 |
We needed this so I hacked it in and added a test case. I ran into trouble getting Mockery to correctly mock the object, so I had to change the tests up a bit.