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

[8.x] Revert "[8.x] Run observer callbacks after database transactions have committed" #438

Closed
wants to merge 1 commit into from

Conversation

driesvints
Copy link
Member

Reverts #436

I'm reverting this because this causes test suites that have the DatabaseTransactions trait applied to fail.

Fixes #437

@driesvints driesvints changed the title Revert "[8.x] Run observer callbacks after database transactions have committed" [8.x] Revert "[8.x] Run observer callbacks after database transactions have committed" Dec 28, 2020
@taylorotwell
Copy link
Member

Personally I think this may need to be fixed at the framework level.

@bakerkretzmar
Copy link
Contributor

bakerkretzmar commented Dec 29, 2020

I think the original issue and bug are more important: Scout should not sync model data to a search index until that data has been persisted into the database. Reverting this would re-break tons of apps using Scout and database transactions (including Nova).

I appreciate where that issue is coming from but that's the expected behaviour, and it can easily be fixed by removing DatabaseTransactions or RefreshDatabase from that specific test.

I agree that it would make a lot of sense for this to be more flexible in the long run.

@driesvints
Copy link
Member Author

I'm going to close this because I agree this probably isn't a fix in the long run.

and it can easily be fixed by removing DatabaseTransactions or RefreshDatabase from that specific test.

How would someone run their tests against a database then?

@driesvints driesvints closed this Dec 29, 2020
@driesvints driesvints deleted the revert-436-listeners-after-transactions branch December 29, 2020 09:32
@bakerkretzmar
Copy link
Contributor

@driesvints thanks!

How would someone run their tests against a database then?

With the DatabaseMigrations trait I think? I've done this before when I had issues with transactions, it's slower and leaves test data in the database but other than that it works fine. Definitely not a perfect solution but it should do the trick for now.

@taylorotwell
Copy link
Member

I went ahead and reverted this. I would like to see it be configurable. You can also force Scout stuff to be update after commits by using Scout's queue configuration option and setting your queue connection to use after_commit.

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.

Model events not called in tests that are wrapped in a transaction
3 participants