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] Allow running observer callbacks after database transactions have committed #440

Merged
merged 1 commit into from
Jan 2, 2021
Merged

[8.x] Allow running observer callbacks after database transactions have committed #440

merged 1 commit into from
Jan 2, 2021

Conversation

bakerkretzmar
Copy link
Contributor

This PR re-implements #436 but makes it configurable. I added $afterCommit to Scout's ModelObserver again, but this time instead of setting it to true I set it based on a new config file entry called after_commit.

Since this PR defaults these values to false, this approach is completely backwards-compatible with all existing Scout installs, in application and test code. I'm targeting the 8.x branch so that apps that need it can start using this functionality immediately, and I'll submit another PR to master to turn it on by default in the next major version of Scout.

Closes #437.

Alternatives

I'm sure there are other ways to tackle this, and I'm happy to explore them, but this seemed like the simplest by far. Two other ideas I played with briefly:

  • Adding an afterCommit method to the ModelObserver, and updating Laravel's event dispatcher to look for that method and allow it to override the property when present. This pattern feels familiar to me, in line with things like the shouldDiscoverEvents method in the EventServiceProvider, but it could get confusing because in other places (queue-related) there are afterCommit methods that explicitly set it to true and don't allow the logic to be customized. This approach would also still require a config option, because users still wouldn't be able to override the method.
  • Making $afterCommit static, which would allow it to be overridden globally for a specific listener/observer, for example in a service provider or base test case. This is powerful and simple but feels hacky.

@taylorotwell taylorotwell merged commit 172eab9 into laravel:8.x Jan 2, 2021
@bakerkretzmar bakerkretzmar deleted the listeners-after-transactions-with-config branch January 3, 2021 02:13
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
2 participants