2.6 add 'atomic' option to "save()" API #3516
Conversation
add a transaction context to 'save()' API in order to rollback possible modifications done in some 'Model.beforeSave' listener callback. this will allow cakephp 2.x to behave like 3.0 .
|
This needs some testcases before it can be merged. |
|
Wouldn't the operations between |
|
If a transaction is started and an uncaught exception happens, then everything would default to rollback() in MySQL - not calling commit() is the same as calling rollback() |
|
What about other SQL RDBMS? |
|
Weird, my comment got lost, it appeared twice and I deleted one of them, now they are both gone. So again, @jippi thanks for the reminder. @jhli PDO has an auto-rollback mechanism implemented that kicks in in such situations, I would think that this is independent of the DBMS functionality. I'm not sure though if it's the best idea to rely on this. |
|
I wouldn't rely on the automatic rollback personally. It will make testing gross and could cause unwanted data to be commited when there are subsequent save operations. |
|
I tried to add some tests but I'm not really used to that (I should really spend some time on that ...). |
| * test callback used in testSaveTransaction method | ||
| */ | ||
| public function testSaveTransactionCallback($event) { | ||
| $TestModel = new Article(); |
There was a problem hiding this comment.
You might not want to start this method name with test. Phpunit will try to execute all methods starting with test as test cases.
Validation occurs before the event is sent. As the validation failed, event was not sent. So test was not relevant. solution : - use the global Event Manager. - try to save a correct 'Post' data. - the first time the callback is called, try to save a correct 'Article' data. - the second time the callback is called, stop the event to rollback everything.
update (again) the test due to my stupidity. - the second time the callback is called, the 'Article' data are still not saved, so there will be no data to rollback. new test which will hopefully work : - use the 'Post' specific Event Manager to attach the callback. - when callback is called, try to save 'Article' data. - assert Article->save() succeeded. - stop event and return false to force rollback in Post->save()
testSaveTransactionNoRollback was not relevant, as validation occurs out of transaction scope. solution : - use the callback 'callbackForTestSaveTransaction' as in 'testSaveTransaction' as the event dispatching is done in transaction scope.
Rename previous save() API to protected _doSave() method. create new save() method for transaction handling.
|
sorry for the multiple commits, but i don't know how to use git for the moment, so i'm using github to edit and commit. btw, I don't know why travis is failing with my test : |
|
The docs have a section on how to run the tests. That should help with not having to push to travis on each change. Don't worry about the additional commits, I can squash them before merging. |
set a returnValue 'true' to DboSource stubbed method 'begin'
| 'body' => 'Fifth Article Body', | ||
| 'published' => 'Y' | ||
| )); | ||
| $result = $TestModel->create() && $TestModel->save($data, array('atomic' => true)); |
There was a problem hiding this comment.
Personally, I would not test create() and just move it outside of this one row up.
| 'published' => 'Y' | ||
| )); | ||
| $result = $TestModel->create() && $TestModel->save($data, array('atomic' => true)); | ||
| $this->assertFalse(empty($result)); |
There was a problem hiding this comment.
I would usually check on this "positively" - $this->assertTrue((bool)$result) for example.
as you expect a non empty value here. Using the opposite is a little bit counter intuitive.
But I guess this is this way for a lot of other tests as well.. So never mind.
|
After hours of fight to do my first phpunit run, I succeeded in fixing my errors ! |
|
There are still a few tests failing with sqlite and postgres. |
|
apparently, the error was here before my PR, but it was not detected during the test : the error comes from this line : In ModeWriteTest.php , the data array to save does not contain 'Portfolio' key, so '$count' is equal to 0 in Model.php. As this is another bug, should I fix it here or should I create a new bug ? |
|
Might as well fix it here. |
|
sorry, too late :( |
|
oups... sorry, not used to git .... I had some problem with the 3 'cake.bat' files related to this fix : I could not work with my branch anymore. |
|
You can use |
|
this PR was closed when i deleted my previous branch. how can I reopen this PR? |
add a transaction context to 'save()' API in order to rollback possible modifications done
in some 'Model.beforeSave' listener callback.
this will allow cakephp 2.x to behave like 3.0 .
this commit resolves issue #3511