[11.x] Allows to persist database connection between tests and only reset PDO in afterClassTearDown()

[crynobone]

Background

PDO connection doesn't automatically get closed when the application is unset but instead requires explicitly set to null to trigger connection close:

https://www.php.net/manual/en/pdo.connections.php

Since PHPUnit is especially a long-running process when the application gets booted and terminated on each test the PDO may persist and eventually can cause max database connections. To solve we have two options:

Option A: We can explicitly disconnect the database between tests

Disadvantages:

• Caused breaking change to the existing application since it is still possible to use PDO and loaded instances (without IoC) after Iluminate\Foundation\Testing\TestCase::tearDown() has been called.
• Any cached/static instances holding PDO will still prevent the database from being disconnected.

Option B: Reuse PDO instances between tests (provided in this PR)

Disadvantage:

• The only issue I can think of if tests within a TestCase change database configuration between tests, since the PDO instance is loaded using cache it not going to fetch the specific configuration defined for the test. However, I don't see any reason why a developer would have this for the application tests, even for package development this would be a rare requirement.

To cover Option A, you can configure a base TestCase and set $disconnectDatabaseConnections property to true:

<?php

namespace Tests;

abstract class TestCase extends \Illuminate\Foundation\Testing\TestCase
{
    use CreatesApplication;

+   protected $disconnectDatabaseConnections = true;

}

[crynobone]

$name shouldn't accept null since parseConfig() can only accept string for $name

[deleugpn]

parseConfig() is only restricted via doc-block documentation. Since the property isn't really strictly typed, it actually works fine by using $factory->make($config) on userland code.

[crynobone]

For multi-tenant applications where you may want to change the default connections, this might not be the best usage:

[deleugpn]

sorry I didn't see your reply here, unfortunately I don't get notifications on the Laravel repository anymore (GitHub bug).

Whether you change or not the default connection in that script it's irrelevant since the configuration is being explicitly passed to the $factory->make(). The variable you hold is the one that is relevant to which database connection you have.

the getName() method is returning exactly what was provided when constructing the connection (null), which is my point about this breaking change. It works as intended and it doesn't seem relevant to break it

[mpyw]

Related to #49373? Please comment there

[KentarouTakeda]

Since this applies to code that falls outside the scope of Suggested change , I would like to comment on it separately. (I'm not familiar with MySQL, so I'd like to comment only on PostgreSQL. But I think MySQL probably has similar features as well.)

#49385 (comment)

There is a risk that the transaction state and session settings will be carried over

There is some changeable state that persists while a session is open, whether inside or outside a transaction, and whether commit or rollback.

However, certain status also allow us to explicitly discard their state.
(Some implementations may already exist. Sorry if there are.)

• Session Authorization
  • Return to initial state with RESET SESSION AUTHORIZATION.
• Connection status settings.
  • RESET ALL resets everything. (Also valid for many settings other than SET statements)
  • PHP's ext/pgsql extension uses this method to reset the state of persistent connections. I think it's a good idea to implement the same in our framework.
• Advisory lock, Named Prepared Statement
  • All will be discarded with DISCARD ALL .

In order to preserve existing behavior, we should execute these (or other?) statements to return the session state to its initial state as much as possible.

---

UPDATE: 2023-12-16 9:52 JST

This problem can now be resolved by an option implemented after the first mention. Thank you!

To cover Option A, you can configure a base TestCase and set $disconnectDatabaseConnections property to true:

[driesvints]

@crynobone should we put this PR in draft while this dependency isn't ready yet?

[crynobone]

I can't merge Testbench Core branch to stable unless Laravel commit with this changes.

Testbench will depends on the new Illuminate\Foundation\Testing\DatabaseConnectionFactory added in this PR.

[deleugpn]

I'd like to chime in on this since it seems like it may heavily affect me.

The only issue I can think of if tests within a TestCase change database configuration between tests, since the PDO instance is loaded using cache it not going to fetch the specific configuration defined for the test. However, I don't see any reason why a developer would have this for the application tests, even for package development this would be a rare requirement.

The reason I have is because I run a multi-database multi-tenant application and sometimes I have some critical features that need the extra level of security to ensure that:

1. Given 2 tenants (2 databases) and 1 user in each tenant
2. When User A performs an action
3. It reflects on Tenant's database without crossing/impacting the tenant boundary.

---

From reading the PR, I see Taylor's point. I've been using Laravel since 5.0 and I have never had any max connection issues. I believe many Laravel users don't face this issue at all. Making a significantly drastic change to how the framework handles database connections affecting every Laravel/Phpunit Users to fix an edge case without a way to keep the existing behavior seems like an opportunity to invite many more issues/bug reports than the current situation.

I'm all for reusing the same PDO connection across many tests which could improve performance and speed up tests, but I feel like users need to be able to better control this.

While we're on this subject, a paper cut issue I've had with Laravel for many years is precisely how DatabaseManager is tied with Config Repository, making it unable to work without global state changes that causes confusion.

If something like my personal desire could be implemented, then the disadvantage listed here would be way less relevant. From my understanding, the disadvantage listed is caused exactly by my "paper cut" mention that you need to manipulate a global state on Config Repository prior to establishing a connection with the DatabaseManager, otherwise any changes to the global Config Repository will not reflect on the DatabaseManager connections already established.

If we expose a establishUsing(string $name, array $config, bool $throwsIfAlreadyConnected = true) on the DatabaseManager in a way that we no longer rely on Config Repository, then it completely removes the surprise-factor when a Laravel user wants to hot swap connection configuration on the DatabaseManager without needing to interact with the global Repository Config.

Note: the method signature here is just a prototype. I don't really expect to have a $throwsIfAlreadyConnected parameter, my goal was more to express the desire allow Laravel users to decide whether to override existing connections on-the-fly or throw an exception if they try to override an existing connection without knowing.

This would also make it easier to use the Migrator class outside of Artisan and we could work out the ability to $migrator->runOn([$migrationsPath], $connectionName, $connectionConfiguration) without having to DB::purge() and then Config::set() (this is something we could easily accomplish on user land code without Laravel needing to get involved).

[deleugpn]

@taylorotwell Possibly because everyone uses RefreshDatabase. But in some reason I prefer DatabaseTruncation, so I critically encountered the problem.

@mpyw If this problem mainly affects DatabaseTruncation, can't it be fixed inside DatabaseTruncation trait?

[crynobone]

I believe many Laravel users don't face this issue at all.

It all depends on the max connection configured locally and on CI, we did face this issue on Framework own tests a few years back: #39877

I'm all for reusing the same PDO connection across many tests which could improve performance and speed up tests, but I feel like users need to be able to better control this.

Welcome any suggestion on this, we did add ability to disconnect the database connection via a property flag in this PR.

If this problem mainly affects DatabaseTruncation, can't it be fixed inside DatabaseTruncation trait?

If the approach is to disconnect all database during tearDown, this have been proposed and rejected, also potentially impacts performance (since we only disconnect database connectio via RefreshDatabase and DatabaseMigrations trait at the moment) with less control.

[taylorotwell]

Would it be simpler and less breaking (particularly for the multi-tenant scenario) to just call gc_collect_cycles periodically while running tests?

[deleugpn]

If the approach is to disconnect all database during tearDown, this have been proposed and rejected, also potentially impacts performance (since we only disconnect database connectio via RefreshDatabase and DatabaseMigrations trait at the moment) with less control.

I guess my primary point is: I'm a user of the DatabaseTransactions trait and as such it seems I'm more unlikely to face this problem than users of the DatabaseTruncation as it was mentioned.

Perhaps the implemented solution to the problem can be more targeted at where the problem is and leave users that are outside the scope of it unaffected. The problem with the current proposal is that it completely hijacks the DatabaseConnectionFactory at a global level (OverrideProvidersForTesting), causing 100% of Laravel installation to be affected by this change regardless of whether they are subject to the reported bug or not.

[mpyw]

@crynobone

Would it be simpler and less breaking (particularly for the multi-tenant scenario) to just call gc_collect_cycles periodically while running tests?

Is this the answer?