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] Disable transaction events in DatabaseTransactions trait #32338

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

mfn
Copy link
Contributor

@mfn mfn commented Apr 11, 2020

This follows commit deafaa7
which already added this for RefreshDatabase.

It's for the exact same reason as outlined in #23832 :

This is motivated by the fact that in tests, when using the RefreshDatabase trait, transactions events are dispatched and may interfer with the application testing itself when it relies on events like these ( for further info, see laravel/ideas#1094 ).

As to not have custom handling of transaction events interfere with the
test suite itself, this simply disables this.

Again, the same change was added to RefreshDatabase and is still
present; this PR merely keeps the behaviour of them in sync in regards
to the handling of events in transactions:

foreach ($this->connectionsToTransact() as $name) {
$connection = $database->connection($name);
$dispatcher = $connection->getEventDispatcher();
$connection->unsetEventDispatcher();
$connection->beginTransaction();
$connection->setEventDispatcher($dispatcher);
}
$this->beforeApplicationDestroyed(function () use ($database) {
foreach ($this->connectionsToTransact() as $name) {
$connection = $database->connection($name);
$dispatcher = $connection->getEventDispatcher();
$connection->unsetEventDispatcher();
$connection->rollback();
$connection->setEventDispatcher($dispatcher);
$connection->disconnect();
}
});

Thanks

This follows commit deafaa7
which already added this for RefreshDatabase.

It's for the exact same reason as outlined in laravel#23832 :
> This is motivated by the fact that in tests, when using the RefreshDatabase trait, transactions events are dispatched and may interfer with the application testing itself when it relies on events like these ( for further info, see laravel/ideas#1094 ).

As to not have custom handling of transaction events interfere with the
test suite itself, this simply disables this.

Again, the same change was added to `RefreshDatabase` and is still
present; this PR merely keeps the behaviour of them in sync in regards
to the handling of events in transactions.
@GrahamCampbell
Copy link
Member

This is a breaking change, so will need to target master.

@mfn mfn changed the base branch from 7.x to master April 12, 2020 08:25
@mfn mfn changed the title [7.x] Disable transaction events in DatabaseTransactions trait [8.x] Disable transaction events in DatabaseTransactions trait Apr 12, 2020
@mfn
Copy link
Contributor Author

mfn commented Apr 12, 2020

Too bad; it was added to 5.6 during the release cycle too => but as suggested, switched branch to master.

Thank you!

@GrahamCampbell
Copy link
Member

it was added to 5.6 during the release cycle too

Yeh, that was a mistake. Too late now.

@taylorotwell taylorotwell merged commit a32a32f into laravel:master Apr 13, 2020
@mfn mfn deleted the mfn-disable-events branch April 13, 2020 18:22
@mfn
Copy link
Contributor Author

mfn commented Apr 13, 2020

Thanks 🙏

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