Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[9.x] Fix afterCommit and RefreshDatabase #41782

Merged
merged 3 commits into from Apr 6, 2022

Conversation

taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Apr 1, 2022

Previously, using the RefreshDatabase trait was fundamentally incompatible with afterCommit. This is because the trait is based around the fact that it opens a transaction and rolls it back at the end of the test, never committing it.

This slight modification updates the DatabaseTransactionManager to "ignore" a given transaction when determining if a callback should be executed, since we want the outer transaction to essentially be invisible.

Would appreciate some feedback on if this solves people's problems and / or creates new problems.

Fix #35857

@BrandonSurowiec
Copy link
Contributor

Shouldn't this target 8.x?

@GrahamCampbell GrahamCampbell changed the title Fix afterCommit and RefreshDatabase [9.x] Fix afterCommit and RefreshDatabase Apr 2, 2022
@driesvints
Copy link
Member

@taylorotwell I think @BrandonSurowiec is correct and we should target 8.x

@taylorotwell taylorotwell merged commit 76a158e into 9.x Apr 6, 2022
@taylorotwell
Copy link
Member Author

Hmm, I'm torn - existing users can just use DatabaseMigrations trait instead of RefreshDatabase on 8.x. Let's see if this actually works out on 9.x first.

@taylorotwell taylorotwell deleted the refresh-database-after-commit branch April 6, 2022 15:17
@BrandonSurowiec
Copy link
Contributor

Hey @taylorotwell I use the LazilyRefreshDatabase trait on my base TestCase, so in my 8.x project it is difficult to switch to DatabaseMigrations. This PR seems to have "worked out" so would you accept a PR to backport this and #42502 into 8.x? I ran into this issue today and would love to delete my workaround.

@reza305z
Copy link

reza305z commented Dec 1, 2022

@taylorotwell still have this issue on laravel 9.42.2 using postgres as database for testing.

@mpyw
Copy link
Contributor

mpyw commented Sep 20, 2023

Still reproduces if the transactions are nested. Please check #48451

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DB::afterCommit callbacks aren't run in tests when RefreshDatabase trait is used
5 participants