[10.x] Fix "after commit" callbacks not running on nested transactions using RefreshDatabase or DatabaseMigrations
#48523
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Co-authored-by: Mior Muhammad Zaki <[email protected]>
to avoid remembering which transactions to ignore Before, we were remembering the test transaction so we could ignore it when deciding to run the after commit callbacks or not. We're still handling the after commit callbacks like that but now instead of remembering which transactions to ignore, we're always calling the DatabaseTransactionManager::commit method. The difference is that now we're passing the current transaction level to it. The method will decide to call the callbacks or not based on that level and whether or not this is on in test mode. When in tests, instead of setting the current transaction to be remembered so it could be ignored, we're now only setting the DatabaseTransactionManager to test mode. When in test mode, it will execute the callbacks when the transactions count reaches 1 (remember that the test runs in a transaction, so that's the "root" level). Otherwise, it runs the callbacks when the transactions level is on level 0 (like in production). There's also a change in the DatabaseTransactionManager::addCallback method. It now also checks if it's in test mode. When not in test mode, it only adds the callback to the execution queue if there's an open transaction. Otherwise, the callback is executed right away. When in test mode, the number of transactions has to be greater than one for it to be added to the callbacks queue.
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
|
Great work!!!!!!!! |
Signed-off-by: Mior Muhammad Zaki <[email protected]>
Signed-off-by: Mior Muhammad Zaki <[email protected]>
|
Will need to retest if the tests actually still breaking without the code changes after #48531 Marking this as draft. |
|
@crynobone Nice work! I think it will still be needed, even after #48531, because it's more about nested transactions on tests, which was only surfaced by the |
|
Verified this changes still solve transaction + after commit issue using RefreshDatabase. See #48545 |
tonysm
reviewed
Sep 26, 2023
src/Illuminate/Foundation/Testing/DatabaseTransactionsManager.php
Outdated
Show resolved
Hide resolved
|
Thanks! |
timacdonald
pushed a commit
to timacdonald/framework
that referenced
this pull request
Sep 27, 2023
…s using `RefreshDatabase` or `DatabaseMigrations` (laravel#48523) * Tests observer using afterCommit * Adds failing test for savepoint and observers using afterCommit * Ignore the createOrFirst savepoint to run callbacks * Fix typo * Remove DatabaseEloquentAppTest to see if CI passes * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * Update src/Illuminate/Database/DatabaseTransactionsManager.php Co-authored-by: Mior Muhammad Zaki <[email protected]> * Changes the way the after commit callbacks are executed to avoid remembering which transactions to ignore Before, we were remembering the test transaction so we could ignore it when deciding to run the after commit callbacks or not. We're still handling the after commit callbacks like that but now instead of remembering which transactions to ignore, we're always calling the DatabaseTransactionManager::commit method. The difference is that now we're passing the current transaction level to it. The method will decide to call the callbacks or not based on that level and whether or not this is on in test mode. When in tests, instead of setting the current transaction to be remembered so it could be ignored, we're now only setting the DatabaseTransactionManager to test mode. When in test mode, it will execute the callbacks when the transactions count reaches 1 (remember that the test runs in a transaction, so that's the "root" level). Otherwise, it runs the callbacks when the transactions level is on level 0 (like in production). There's also a change in the DatabaseTransactionManager::addCallback method. It now also checks if it's in test mode. When not in test mode, it only adds the callback to the execution queue if there's an open transaction. Otherwise, the callback is executed right away. When in test mode, the number of transactions has to be greater than one for it to be added to the callbacks queue. * Fix DatabaseTransactionsTest * Fix DatabaseTransactionsManagerTest * CSFixer * wip * Rename method and property and inline usage * Adds a depply nested transaction test * Simplify deeply nesting test * Sets default level value to one (since it's a new parameter) * Rename method * Adds back the removed methods from the db.transactions and mark them as deprecated * StyleCI * Inline the if statement using then collection when() method * Tests observer using afterCommit Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * Apply fixes from StyleCI * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * Apply fixes from StyleCI * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * Apply fixes from StyleCI * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * Apply fixes from StyleCI * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * Apply fixes from StyleCI * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * wip Signed-off-by: Mior Muhammad Zaki <[email protected]> * Update src/Illuminate/Foundation/Testing/DatabaseTransactionsManager.php Co-authored-by: Tony Messias <[email protected]> * Update src/Illuminate/Database/DatabaseTransactionsManager.php * formatting --------- Signed-off-by: Mior Muhammad Zaki <[email protected]> Co-authored-by: Tony Messias <[email protected]> Co-authored-by: Taylor Otwell <[email protected]> Co-authored-by: StyleCI Bot <[email protected]>
|
Hey @tonysm, the project I am working on uses nested transactions and they used to work flawlesly however I've updated laravel framework and now I keep getting errors: I just reverted back to v10.25.0 and the problem has gone away, then changed to v10.25.1 and problem re-appeared. |
|
Hi @azgooon Are you consistently using |
|
This PR only change behaviour while running tests and doesn't affect your application code. Submit new bug report with reproducing code. |
|
Hi @mpyw yes, DB::transaction() is the approach we use. |
|
@azgooon Which database are you using and can you share a test where this issue appears (if you can reduce it down to a simple test, that would be great)? As @crynobone mentioned, the changes here should only affect tests, so I assume your tests failed, right? Pls share more context on the tests, which traits you're using and things like that. Thanks! |
|
@tonysm I will happily provide as much details as I can. To start from I am not refering to the test as we don't have any. I was refering to the actual functionality of the project which partially stopped working in places where DB::transaction calls methods which are also performing DB::transaction(); Please bear with me a while, I will do my best to extract a piece of code which does not work anymore, We use PostgreSQL. |
|
Not related to this PR, please compile new bug report with reproducing code and continue there. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Separate
db.transactionsinstance between normal requests and tests. PR based from #48466 and fixes #48451