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] Close doctrineConnection on disconnect #41584

Merged
merged 2 commits into from
Mar 21, 2022

Conversation

lcharette
Copy link
Contributor

@lcharette lcharette commented Mar 20, 2022

Consider this test:

class ExampleTest extends TestCase
{
    use DatabaseMigrations; 

    protected function tearDown(): void
    {
        // parent::tearDown(); <-- Omitted on purposed (or not)

        DB::disconnect();
    }

    public function test_1()
    {
        $this->assertTrue(true);
    }

    public function test_2()
    {
        $this->assertTrue(true);
    }

    // Repeat test 102x times...

    public function test_105()
    {
        $this->assertTrue(true);
    }
}

With a migration that modify a column:

public function up()
{
    Schema::create('users', function (Blueprint $table) {
        $table->increments('id');
        $table->string('user_name', 50);
    });

    Schema::table('users', function (Blueprint $table) {
        $table->string('user_name', 120)->change();
    });
}

On PostgreSQL, after the 100th iteration, this error will pop up:

Illuminate\Database\QueryException: SQLSTATE[08006] [7] connection to server at "127.0.0.1", port 5432 failed: FATAL:  sorry, too many clients already (SQL: create table "migrations" ("id" serial primary key not null, "migration" varchar(255) not null, "batch" integer not null))

While disconnect sets the “main” PDO object as null, the Doctrine connection created by the “change” action is not. The actual connection to the database is not actually closed in this specific case as the underlying PDO object is not fully destroyed as per PHP Documentation: https://www.php.net/manual/en/pdo.connections.php.

This bug won't affect any migration that are not modifying a column, as the Doctrine Connection is only created when needed, eg. using ->change().

I understand this fix won’t have much impact on a normal Laravel install, as :

  1. Laravel TestCase undefined the whole app on tearDown(), Connection included, closing the Doctrine connection.
  2. This test is pointless and should call parent::tearDown();

But in a situation where disconnect() is called outside of a test scenario, or anyone not using the whole Laravel TestCase might encounter this issue.

References :

@taylorotwell taylorotwell merged commit 4a9027a into laravel:8.x Mar 21, 2022
@lcharette lcharette deleted the lcharette-patch-1 branch March 21, 2022 23:36
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.

2 participants