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] Run observer callbacks after transactions by default #441

Closed
wants to merge 1 commit into from
Closed

[8.x] Run observer callbacks after transactions by default #441

wants to merge 1 commit into from

Conversation

bakerkretzmar
Copy link
Contributor

@bakerkretzmar bakerkretzmar commented Jan 3, 2021

Following up on #436, #437, and #440, this PR changes the default value of the new after_commit config option to true. The current behaviour, with this option set to false, causes stale data to be synced to search indexes in applications using database transactions (see #436).

I'm targeting the 8.x branch again because it's very unlikely that this change will affect consuming application code, and the config option now makes it easy to disable when necessary, like in test code that uses database transactions.

Personally, I think it's preferable to make this behaviour the default as soon as possible. If you folks are more comfortable making that change in the next major version of Scout I'm happy to update this PR to target the master branch instead, let me know. I'll PR an update to the docs soon too. Thanks for all your attention to this!

@taylorotwell
Copy link
Member

Why is it necessary to make this change as soon as possible? Hasn't the current code been the default behavior of Scout since its initial release?

@bakerkretzmar
Copy link
Contributor Author

@taylorotwell yes, the current behaviour has been in place since the beginning. I just think it's a bug and we should fix it now since we're able to, rather than waiting. To be honest I'm surprised it hasn't caused more issues for people, especially since Nova is affected by it.

@taylorotwell
Copy link
Member

I sympathize with your point of view but think I will leave the default as-is for now just to avoid a breaking change. It matches the default behavior of the queues in Laravel. We could re-evaluate this in a future release.

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.

None yet

2 participants