[5.6] Reuse code from DatabaseTransactions in RefreshDatabase

[mnabialek]

No description provided.

[laurencei]

Nice.

Any reason not to target this straight to 5.5?

[GrahamCampbell]

No urgency to get into 5.5, it doesn't fix or add anything?

[mnabialek]

@GrahamCampbell Right, this is only code cleanup so it can wait until 5.6

[laurencei]

No urgency to get into 5.5, it doesn't fix or add anything?

My thinking was just 5.5 is LTS - so putting this in now makes sense.

Meh - doesnt really matter either way - just a thought.

[skollro]

This change needs to be reverted, it causes problematic side effects.

We're evaluating all the used traits of TestCase recursively. If we apply the RefreshDatabase trait to one of our tests, this applies DatabaseTransactions in addition.

So two transactions are started instead of one. This leads to errors on rollbacks and the strange behaviour I showed in https://twitter.com/skollro/status/962695003624628225

For the desired code reuse we have to come up with another solution.

framework/src/Illuminate/Foundation/Testing/TestCase.php

Lines 99 to 113 in 735b877

	protected function setUpTraits()
	{
	$uses = array_flip(class_uses_recursive(static::class));
	if (isset($uses[RefreshDatabase::class])) {
	$this->refreshDatabase();
	}
	if (isset($uses[DatabaseMigrations::class])) {
	$this->runDatabaseMigrations();
	}
	if (isset($uses[DatabaseTransactions::class])) {
	$this->beginDatabaseTransaction();
	}

[antonkomarev]

@skollro I've found this issue too. I recommend you to create PR or at least separate issue.

[laurencei]

@skollro @a-komarev,

Are you sure it is this PR that is causing that problem? Looking at this PR - the functional code is the same before/after. I dont see how the trait change causes any changes to the code?

There was another PR I remember, where the way DB transactions were performed was changed for 5.6. It might be that PR causing the problem, not this one.

[skollro]

It doesn't change the code, but its behavior as we implement two traits now implicitly. The problem is that we're now starting two transactions instead of one wrapping each test where RefreshDatabase is used. So something's wrong here anyway...

Now both of the two blocks are executed, before it was only one of them. The first one is calling $this->beginDatabaseTransaction() in refreshDatabase().

$uses = array_flip(class_uses_recursive(static::class));

if (isset($uses[RefreshDatabase::class])) { 
    $this->refreshDatabase();
} 
// ...
if (isset($uses[DatabaseTransactions::class])) { 
     $this->beginDatabaseTransaction(); 
}

[laurencei]

Ok - I see the issue now.

I've got a solution in mind - I'll submit a PR

[skollro]

Thank you! Yeah, the issue isn't very easy to track down...

[skollro]

Maybe your solution would be the same as @mnabialek proposed yesterday in #23121

[laurencei]

No - I've got a different idea. I'm testing it now before I submit a PR.

[laurencei]

Ok - I've submitted a PR with a different way to fix this.