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 database transactions have committed #436

Merged
merged 1 commit into from Dec 21, 2020
Merged

[8.x] Run observer callbacks after database transactions have committed #436

merged 1 commit into from Dec 21, 2020

Conversation

bakerkretzmar
Copy link
Contributor

This PR sets public $afterCommit = true on Scout's ModelObserver class, so that the saved, deleted, forceDeleted, and restored model event callbacks, if they are triggered inside a database transaction, are not executed until that transaction has committed.

Background

This fixes an existing issue where incorrect data is synced to a search index if a model event Scout listens for, such as saved, is fired inside a database transaction.

If Scout is not set up to queue search index updates, this issue affects all code that fires the above model events inside database transactions, because Scout's observer method is executed immediately—before the transaction has committed or been rolled back—syncing stale or invalid data.

If Scout is set up to use the queue this issue will appear intermittently, when the queue workers pick up and execute the syncing job before the transaction has committed or rolled back. In my experience this happens almost all the time.

See #152 and laravel/nova-issues#1906.

It wasn't really possible to address this at all up until now, but transaction-aware code execution was just added to Laravel core (see laravel/framework#35373 and laravel/framework#35434). In Laravel 8.17.0 and later, it's trivial to delay any listeners and observers until open database transactions have committed.

Backwards compatibility

Theoretically this could change some behaviour in existing Scout installations, but I would argue that that behaviour is a bug and should be fixed. I can't imagine a situation where an application would be relying on stale or invalid data being persisted to a search index, so I doubt this change would break anything.

Tests

Since model observers are basically just larger listener classes, in my opinion this PR is covered by Laravel's tests for delaying listener execution until transactions have committed: https://github.com/laravel/framework/blob/8.x/tests/Integration/Events/ListenerTest.php.

@driesvints driesvints changed the title Run observer callbacks after database transactions have committed [8.x] Run observer callbacks after database transactions have committed Dec 21, 2020
@taylorotwell taylorotwell merged commit 31f02b8 into laravel:8.x Dec 21, 2020
driesvints added a commit that referenced this pull request Dec 28, 2020
@driesvints
Copy link
Member

I've sent in a PR to revert this because this causes test suite that have the DatabaseTransactions trait applied to fail. We can make this change in the next major release. Feel free to send in a PR to master.

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

3 participants