fix: migrations not using custom DB connection of migration runner

[paulbalandan]

Description
Closes#8060

When using custom array connection for migration runner, the migrations ran do not use the custom connection of the runner but instead uses the default shared connection.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

I found this bug while working on phpstan-codeigniter in trying to add an extension for properly identifying the property types of Entities.

[kenjis]

@paulbalandan The code is indeed something wrong, but I don't understand the significance of passing DB connection to MigrationRunner.

[paulbalandan]

The use case is this: In order to infer the property types of Entities, I thought of using the database tables and the fields. So, I must run migrations in order to get the tables. However, I don't want to mess with the migrations of the end users so I thought of running the migrations in a separate connection. So, regardless of the default connection of the user, I run my own migrations using SQLite3 with a database file. I don't use command('migrate') because it will use the default group. I don't want either to alter the shared Config\Database. So, here comes the array connection. All went smoothly until I am listing all the tables using $db->listTables() to find out that the db file only contains the migrations table. Upon checking, migration files were found but were ran using the shared connection.

[kenjis]

Okay, so you want to change the DB connection for testing.
After all, the parameter is for testing.

I sent PR #8222 to make it clear.

[kenjis]

What should be the group for the added test?

sqlite> select * from migrations;
1|2018-01-24-102301|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_some_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1
2|2018-01-24-102302|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_another_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1

[kenjis]

It appears that resetDataCache() resolves the OCI8 test failures.
See https://github.com/kenjis/CodeIgniter4/pull/138/files#diff-ef065cd29dd2066d73750ad2480b61bf513b037402889ee0c0d9e0fb8b3fca3bR478-R479

[kenjis]

Frankly, Migration is complex and difficult to maintain consistency.

When temporarily changing a DB connection for testing purposes, it seems better to regard that the DB connection as the default group because it is easier to manipulate.

However, it seems a bit odd that the DB group name in the migration history is changed to the default group even if you specify the name of the DB group, e.g., db1.

Maybe the following patch is needed.

--- a/system/Database/MigrationRunner.php+++ b/system/Database/MigrationRunner.php@@ -141,10 +141,17 @@ class MigrationRunner
// Default name space is the app namespace
$this->namespace = APP_NAMESPACE;
- // get default database group- $config = config(Database::class);- $this->group = $config->defaultGroup;- unset($config);+ if (is_string($db)) {+ $this->group = $db;+ } else {+ // We use `defaultGroup` even if $db is ConnectionInterface or config+ // array. Because this is for testing purposes, and it is easier to+ // understand and operate as if the migrations were run on the+ // default group.+ $config = config(Database::class);+ $this->group = $config->defaultGroup;+ unset($config);+ }
// If no db connection passed in, use
// default database group.
@@ -841,7 +848,7 @@ class MigrationRunner
/** @var Migration $instance */
$instance = new $class(Database::forge($this->db));
- $group = $instance->getDBGroup() ?? config(Database::class)->defaultGroup;+ $group = $instance->getDBGroup() ?? $this->group;
if (ENVIRONMENT !== 'testing' && $group === 'tests' && $this->groupFilter !== 'tests') {
// @codeCoverageIgnoreStart
@@ -857,8 +864,6 @@ class MigrationRunner
return true;
}
- $this->setGroup($group);-
if (! is_callable([$instance, $direction])) {
$message = sprintf(lang('Migrations.missingMethod'), $direction);

[MGatner]

This is complicated. I'm tracking but let me know if you need me more. Thanks for handling.

[kenjis]

LGTM!

[paulbalandan]

Description
Closes#8060

When using custom array connection for migration runner, the migrations ran do not use the custom connection of the runner but instead uses the default shared connection.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

I found this bug while working on phpstan-codeigniter in trying to add an extension for properly identifying the property types of Entities.

[kenjis]

@paulbalandan The code is indeed something wrong, but I don't understand the significance of passing DB connection to MigrationRunner.

[paulbalandan]

The use case is this: In order to infer the property types of Entities, I thought of using the database tables and the fields. So, I must run migrations in order to get the tables. However, I don't want to mess with the migrations of the end users so I thought of running the migrations in a separate connection. So, regardless of the default connection of the user, I run my own migrations using SQLite3 with a database file. I don't use command('migrate') because it will use the default group. I don't want either to alter the shared Config\Database. So, here comes the array connection. All went smoothly until I am listing all the tables using $db->listTables() to find out that the db file only contains the migrations table. Upon checking, migration files were found but were ran using the shared connection.

[kenjis]

Okay, so you want to change the DB connection for testing.
After all, the parameter is for testing.

I sent PR #8222 to make it clear.

[kenjis]

What should be the group for the added test?

sqlite> select * from migrations;
1|2018-01-24-102301|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_some_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1
2|2018-01-24-102302|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_another_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1

[kenjis]

It appears that resetDataCache() resolves the OCI8 test failures.
See https://github.com/kenjis/CodeIgniter4/pull/138/files#diff-ef065cd29dd2066d73750ad2480b61bf513b037402889ee0c0d9e0fb8b3fca3bR478-R479

[kenjis]

Frankly, Migration is complex and difficult to maintain consistency.

When temporarily changing a DB connection for testing purposes, it seems better to regard that the DB connection as the default group because it is easier to manipulate.

However, it seems a bit odd that the DB group name in the migration history is changed to the default group even if you specify the name of the DB group, e.g., db1.

Maybe the following patch is needed.

--- a/system/Database/MigrationRunner.php+++ b/system/Database/MigrationRunner.php@@ -141,10 +141,17 @@ class MigrationRunner
// Default name space is the app namespace
$this->namespace = APP_NAMESPACE;
- // get default database group- $config = config(Database::class);- $this->group = $config->defaultGroup;- unset($config);+ if (is_string($db)) {+ $this->group = $db;+ } else {+ // We use `defaultGroup` even if $db is ConnectionInterface or config+ // array. Because this is for testing purposes, and it is easier to+ // understand and operate as if the migrations were run on the+ // default group.+ $config = config(Database::class);+ $this->group = $config->defaultGroup;+ unset($config);+ }
// If no db connection passed in, use
// default database group.
@@ -841,7 +848,7 @@ class MigrationRunner
/** @var Migration $instance */
$instance = new $class(Database::forge($this->db));
- $group = $instance->getDBGroup() ?? config(Database::class)->defaultGroup;+ $group = $instance->getDBGroup() ?? $this->group;
if (ENVIRONMENT !== 'testing' && $group === 'tests' && $this->groupFilter !== 'tests') {
// @codeCoverageIgnoreStart
@@ -857,8 +864,6 @@ class MigrationRunner
return true;
}
- $this->setGroup($group);-
if (! is_callable([$instance, $direction])) {
$message = sprintf(lang('Migrations.missingMethod'), $direction);

[MGatner]

This is complicated. I'm tracking but let me know if you need me more. Thanks for handling.

[kenjis]

LGTM!

[paulbalandan]

Description
Closes#8060

When using custom array connection for migration runner, the migrations ran do not use the custom connection of the runner but instead uses the default shared connection.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

I found this bug while working on phpstan-codeigniter in trying to add an extension for properly identifying the property types of Entities.

[kenjis]

@paulbalandan The code is indeed something wrong, but I don't understand the significance of passing DB connection to MigrationRunner.

[paulbalandan]

The use case is this: In order to infer the property types of Entities, I thought of using the database tables and the fields. So, I must run migrations in order to get the tables. However, I don't want to mess with the migrations of the end users so I thought of running the migrations in a separate connection. So, regardless of the default connection of the user, I run my own migrations using SQLite3 with a database file. I don't use command('migrate') because it will use the default group. I don't want either to alter the shared Config\Database. So, here comes the array connection. All went smoothly until I am listing all the tables using $db->listTables() to find out that the db file only contains the migrations table. Upon checking, migration files were found but were ran using the shared connection.

[kenjis]

Okay, so you want to change the DB connection for testing.
After all, the parameter is for testing.

I sent PR #8222 to make it clear.

[kenjis]

What should be the group for the added test?

sqlite> select * from migrations;
1|2018-01-24-102301|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_some_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1
2|2018-01-24-102302|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_another_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1

[kenjis]

It appears that resetDataCache() resolves the OCI8 test failures.
See https://github.com/kenjis/CodeIgniter4/pull/138/files#diff-ef065cd29dd2066d73750ad2480b61bf513b037402889ee0c0d9e0fb8b3fca3bR478-R479

[kenjis]

Frankly, Migration is complex and difficult to maintain consistency.

When temporarily changing a DB connection for testing purposes, it seems better to regard that the DB connection as the default group because it is easier to manipulate.

However, it seems a bit odd that the DB group name in the migration history is changed to the default group even if you specify the name of the DB group, e.g., db1.

Maybe the following patch is needed.

--- a/system/Database/MigrationRunner.php+++ b/system/Database/MigrationRunner.php@@ -141,10 +141,17 @@ class MigrationRunner
// Default name space is the app namespace
$this->namespace = APP_NAMESPACE;
- // get default database group- $config = config(Database::class);- $this->group = $config->defaultGroup;- unset($config);+ if (is_string($db)) {+ $this->group = $db;+ } else {+ // We use `defaultGroup` even if $db is ConnectionInterface or config+ // array. Because this is for testing purposes, and it is easier to+ // understand and operate as if the migrations were run on the+ // default group.+ $config = config(Database::class);+ $this->group = $config->defaultGroup;+ unset($config);+ }
// If no db connection passed in, use
// default database group.
@@ -841,7 +848,7 @@ class MigrationRunner
/** @var Migration $instance */
$instance = new $class(Database::forge($this->db));
- $group = $instance->getDBGroup() ?? config(Database::class)->defaultGroup;+ $group = $instance->getDBGroup() ?? $this->group;
if (ENVIRONMENT !== 'testing' && $group === 'tests' && $this->groupFilter !== 'tests') {
// @codeCoverageIgnoreStart
@@ -857,8 +864,6 @@ class MigrationRunner
return true;
}
- $this->setGroup($group);-
if (! is_callable([$instance, $direction])) {
$message = sprintf(lang('Migrations.missingMethod'), $direction);

[MGatner]

This is complicated. I'm tracking but let me know if you need me more. Thanks for handling.

[kenjis]

LGTM!

[paulbalandan]

Description
Closes#8060

When using custom array connection for migration runner, the migrations ran do not use the custom connection of the runner but instead uses the default shared connection.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

I found this bug while working on phpstan-codeigniter in trying to add an extension for properly identifying the property types of Entities.

[kenjis]

@paulbalandan The code is indeed something wrong, but I don't understand the significance of passing DB connection to MigrationRunner.

[paulbalandan]

The use case is this: In order to infer the property types of Entities, I thought of using the database tables and the fields. So, I must run migrations in order to get the tables. However, I don't want to mess with the migrations of the end users so I thought of running the migrations in a separate connection. So, regardless of the default connection of the user, I run my own migrations using SQLite3 with a database file. I don't use command('migrate') because it will use the default group. I don't want either to alter the shared Config\Database. So, here comes the array connection. All went smoothly until I am listing all the tables using $db->listTables() to find out that the db file only contains the migrations table. Upon checking, migration files were found but were ran using the shared connection.

[kenjis]

Okay, so you want to change the DB connection for testing.
After all, the parameter is for testing.

I sent PR #8222 to make it clear.

[kenjis]

What should be the group for the added test?

sqlite> select * from migrations;
1|2018-01-24-102301|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_some_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1
2|2018-01-24-102302|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_another_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1

[kenjis]

It appears that resetDataCache() resolves the OCI8 test failures.
See https://github.com/kenjis/CodeIgniter4/pull/138/files#diff-ef065cd29dd2066d73750ad2480b61bf513b037402889ee0c0d9e0fb8b3fca3bR478-R479

[kenjis]

Frankly, Migration is complex and difficult to maintain consistency.

When temporarily changing a DB connection for testing purposes, it seems better to regard that the DB connection as the default group because it is easier to manipulate.

However, it seems a bit odd that the DB group name in the migration history is changed to the default group even if you specify the name of the DB group, e.g., db1.

Maybe the following patch is needed.

--- a/system/Database/MigrationRunner.php+++ b/system/Database/MigrationRunner.php@@ -141,10 +141,17 @@ class MigrationRunner
// Default name space is the app namespace
$this->namespace = APP_NAMESPACE;
- // get default database group- $config = config(Database::class);- $this->group = $config->defaultGroup;- unset($config);+ if (is_string($db)) {+ $this->group = $db;+ } else {+ // We use `defaultGroup` even if $db is ConnectionInterface or config+ // array. Because this is for testing purposes, and it is easier to+ // understand and operate as if the migrations were run on the+ // default group.+ $config = config(Database::class);+ $this->group = $config->defaultGroup;+ unset($config);+ }
// If no db connection passed in, use
// default database group.
@@ -841,7 +848,7 @@ class MigrationRunner
/** @var Migration $instance */
$instance = new $class(Database::forge($this->db));
- $group = $instance->getDBGroup() ?? config(Database::class)->defaultGroup;+ $group = $instance->getDBGroup() ?? $this->group;
if (ENVIRONMENT !== 'testing' && $group === 'tests' && $this->groupFilter !== 'tests') {
// @codeCoverageIgnoreStart
@@ -857,8 +864,6 @@ class MigrationRunner
return true;
}
- $this->setGroup($group);-
if (! is_callable([$instance, $direction])) {
$message = sprintf(lang('Migrations.missingMethod'), $direction);

[MGatner]

This is complicated. I'm tracking but let me know if you need me more. Thanks for handling.

[kenjis]

LGTM!

[paulbalandan]

Description
Closes#8060

When using custom array connection for migration runner, the migrations ran do not use the custom connection of the runner but instead uses the default shared connection.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

I found this bug while working on phpstan-codeigniter in trying to add an extension for properly identifying the property types of Entities.

[kenjis]

@paulbalandan The code is indeed something wrong, but I don't understand the significance of passing DB connection to MigrationRunner.

[paulbalandan]

The use case is this: In order to infer the property types of Entities, I thought of using the database tables and the fields. So, I must run migrations in order to get the tables. However, I don't want to mess with the migrations of the end users so I thought of running the migrations in a separate connection. So, regardless of the default connection of the user, I run my own migrations using SQLite3 with a database file. I don't use command('migrate') because it will use the default group. I don't want either to alter the shared Config\Database. So, here comes the array connection. All went smoothly until I am listing all the tables using $db->listTables() to find out that the db file only contains the migrations table. Upon checking, migration files were found but were ran using the shared connection.

[kenjis]

Okay, so you want to change the DB connection for testing.
After all, the parameter is for testing.

I sent PR #8222 to make it clear.

[kenjis]

What should be the group for the added test?

sqlite> select * from migrations;
1|2018-01-24-102301|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_some_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1
2|2018-01-24-102302|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_another_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1

[kenjis]

It appears that resetDataCache() resolves the OCI8 test failures.
See https://github.com/kenjis/CodeIgniter4/pull/138/files#diff-ef065cd29dd2066d73750ad2480b61bf513b037402889ee0c0d9e0fb8b3fca3bR478-R479

[kenjis]

Frankly, Migration is complex and difficult to maintain consistency.

When temporarily changing a DB connection for testing purposes, it seems better to regard that the DB connection as the default group because it is easier to manipulate.

However, it seems a bit odd that the DB group name in the migration history is changed to the default group even if you specify the name of the DB group, e.g., db1.

Maybe the following patch is needed.

--- a/system/Database/MigrationRunner.php+++ b/system/Database/MigrationRunner.php@@ -141,10 +141,17 @@ class MigrationRunner
// Default name space is the app namespace
$this->namespace = APP_NAMESPACE;
- // get default database group- $config = config(Database::class);- $this->group = $config->defaultGroup;- unset($config);+ if (is_string($db)) {+ $this->group = $db;+ } else {+ // We use `defaultGroup` even if $db is ConnectionInterface or config+ // array. Because this is for testing purposes, and it is easier to+ // understand and operate as if the migrations were run on the+ // default group.+ $config = config(Database::class);+ $this->group = $config->defaultGroup;+ unset($config);+ }
// If no db connection passed in, use
// default database group.
@@ -841,7 +848,7 @@ class MigrationRunner
/** @var Migration $instance */
$instance = new $class(Database::forge($this->db));
- $group = $instance->getDBGroup() ?? config(Database::class)->defaultGroup;+ $group = $instance->getDBGroup() ?? $this->group;
if (ENVIRONMENT !== 'testing' && $group === 'tests' && $this->groupFilter !== 'tests') {
// @codeCoverageIgnoreStart
@@ -857,8 +864,6 @@ class MigrationRunner
return true;
}
- $this->setGroup($group);-
if (! is_callable([$instance, $direction])) {
$message = sprintf(lang('Migrations.missingMethod'), $direction);

[MGatner]

This is complicated. I'm tracking but let me know if you need me more. Thanks for handling.

[kenjis]

LGTM!

[paulbalandan]

Description
Closes#8060

When using custom array connection for migration runner, the migrations ran do not use the custom connection of the runner but instead uses the default shared connection.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

I found this bug while working on phpstan-codeigniter in trying to add an extension for properly identifying the property types of Entities.

[kenjis]

@paulbalandan The code is indeed something wrong, but I don't understand the significance of passing DB connection to MigrationRunner.

[paulbalandan]

The use case is this: In order to infer the property types of Entities, I thought of using the database tables and the fields. So, I must run migrations in order to get the tables. However, I don't want to mess with the migrations of the end users so I thought of running the migrations in a separate connection. So, regardless of the default connection of the user, I run my own migrations using SQLite3 with a database file. I don't use command('migrate') because it will use the default group. I don't want either to alter the shared Config\Database. So, here comes the array connection. All went smoothly until I am listing all the tables using $db->listTables() to find out that the db file only contains the migrations table. Upon checking, migration files were found but were ran using the shared connection.

[kenjis]

Okay, so you want to change the DB connection for testing.
After all, the parameter is for testing.

I sent PR #8222 to make it clear.

[kenjis]

What should be the group for the added test?

sqlite> select * from migrations;
1|2018-01-24-102301|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_some_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1
2|2018-01-24-102302|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_another_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1

[kenjis]

It appears that resetDataCache() resolves the OCI8 test failures.
See https://github.com/kenjis/CodeIgniter4/pull/138/files#diff-ef065cd29dd2066d73750ad2480b61bf513b037402889ee0c0d9e0fb8b3fca3bR478-R479

[kenjis]

Frankly, Migration is complex and difficult to maintain consistency.

When temporarily changing a DB connection for testing purposes, it seems better to regard that the DB connection as the default group because it is easier to manipulate.

However, it seems a bit odd that the DB group name in the migration history is changed to the default group even if you specify the name of the DB group, e.g., db1.

Maybe the following patch is needed.

--- a/system/Database/MigrationRunner.php+++ b/system/Database/MigrationRunner.php@@ -141,10 +141,17 @@ class MigrationRunner
// Default name space is the app namespace
$this->namespace = APP_NAMESPACE;
- // get default database group- $config = config(Database::class);- $this->group = $config->defaultGroup;- unset($config);+ if (is_string($db)) {+ $this->group = $db;+ } else {+ // We use `defaultGroup` even if $db is ConnectionInterface or config+ // array. Because this is for testing purposes, and it is easier to+ // understand and operate as if the migrations were run on the+ // default group.+ $config = config(Database::class);+ $this->group = $config->defaultGroup;+ unset($config);+ }
// If no db connection passed in, use
// default database group.
@@ -841,7 +848,7 @@ class MigrationRunner
/** @var Migration $instance */
$instance = new $class(Database::forge($this->db));
- $group = $instance->getDBGroup() ?? config(Database::class)->defaultGroup;+ $group = $instance->getDBGroup() ?? $this->group;
if (ENVIRONMENT !== 'testing' && $group === 'tests' && $this->groupFilter !== 'tests') {
// @codeCoverageIgnoreStart
@@ -857,8 +864,6 @@ class MigrationRunner
return true;
}
- $this->setGroup($group);-
if (! is_callable([$instance, $direction])) {
$message = sprintf(lang('Migrations.missingMethod'), $direction);

[MGatner]

This is complicated. I'm tracking but let me know if you need me more. Thanks for handling.

[kenjis]

LGTM!

[paulbalandan]

Description
Closes#8060

When using custom array connection for migration runner, the migrations ran do not use the custom connection of the runner but instead uses the default shared connection.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

I found this bug while working on phpstan-codeigniter in trying to add an extension for properly identifying the property types of Entities.

[kenjis]

@paulbalandan The code is indeed something wrong, but I don't understand the significance of passing DB connection to MigrationRunner.

[paulbalandan]

The use case is this: In order to infer the property types of Entities, I thought of using the database tables and the fields. So, I must run migrations in order to get the tables. However, I don't want to mess with the migrations of the end users so I thought of running the migrations in a separate connection. So, regardless of the default connection of the user, I run my own migrations using SQLite3 with a database file. I don't use command('migrate') because it will use the default group. I don't want either to alter the shared Config\Database. So, here comes the array connection. All went smoothly until I am listing all the tables using $db->listTables() to find out that the db file only contains the migrations table. Upon checking, migration files were found but were ran using the shared connection.

[kenjis]

Okay, so you want to change the DB connection for testing.
After all, the parameter is for testing.

I sent PR #8222 to make it clear.

[kenjis]

What should be the group for the added test?

sqlite> select * from migrations;
1|2018-01-24-102301|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_some_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1
2|2018-01-24-102302|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_another_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1

[kenjis]

It appears that resetDataCache() resolves the OCI8 test failures.
See https://github.com/kenjis/CodeIgniter4/pull/138/files#diff-ef065cd29dd2066d73750ad2480b61bf513b037402889ee0c0d9e0fb8b3fca3bR478-R479

[kenjis]

Frankly, Migration is complex and difficult to maintain consistency.

When temporarily changing a DB connection for testing purposes, it seems better to regard that the DB connection as the default group because it is easier to manipulate.

However, it seems a bit odd that the DB group name in the migration history is changed to the default group even if you specify the name of the DB group, e.g., db1.

Maybe the following patch is needed.

--- a/system/Database/MigrationRunner.php+++ b/system/Database/MigrationRunner.php@@ -141,10 +141,17 @@ class MigrationRunner
// Default name space is the app namespace
$this->namespace = APP_NAMESPACE;
- // get default database group- $config = config(Database::class);- $this->group = $config->defaultGroup;- unset($config);+ if (is_string($db)) {+ $this->group = $db;+ } else {+ // We use `defaultGroup` even if $db is ConnectionInterface or config+ // array. Because this is for testing purposes, and it is easier to+ // understand and operate as if the migrations were run on the+ // default group.+ $config = config(Database::class);+ $this->group = $config->defaultGroup;+ unset($config);+ }
// If no db connection passed in, use
// default database group.
@@ -841,7 +848,7 @@ class MigrationRunner
/** @var Migration $instance */
$instance = new $class(Database::forge($this->db));
- $group = $instance->getDBGroup() ?? config(Database::class)->defaultGroup;+ $group = $instance->getDBGroup() ?? $this->group;
if (ENVIRONMENT !== 'testing' && $group === 'tests' && $this->groupFilter !== 'tests') {
// @codeCoverageIgnoreStart
@@ -857,8 +864,6 @@ class MigrationRunner
return true;
}
- $this->setGroup($group);-
if (! is_callable([$instance, $direction])) {
$message = sprintf(lang('Migrations.missingMethod'), $direction);

[MGatner]

This is complicated. I'm tracking but let me know if you need me more. Thanks for handling.

[kenjis]

LGTM!

[paulbalandan]

Description
Closes#8060

When using custom array connection for migration runner, the migrations ran do not use the custom connection of the runner but instead uses the default shared connection.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[paulbalandan]

I found this bug while working on phpstan-codeigniter in trying to add an extension for properly identifying the property types of Entities.

[kenjis]

@paulbalandan The code is indeed something wrong, but I don't understand the significance of passing DB connection to MigrationRunner.

[paulbalandan]

The use case is this: In order to infer the property types of Entities, I thought of using the database tables and the fields. So, I must run migrations in order to get the tables. However, I don't want to mess with the migrations of the end users so I thought of running the migrations in a separate connection. So, regardless of the default connection of the user, I run my own migrations using SQLite3 with a database file. I don't use command('migrate') because it will use the default group. I don't want either to alter the shared Config\Database. So, here comes the array connection. All went smoothly until I am listing all the tables using $db->listTables() to find out that the db file only contains the migrations table. Upon checking, migration files were found but were ran using the shared connection.

[kenjis]

Okay, so you want to change the DB connection for testing.
After all, the parameter is for testing.

I sent PR #8222 to make it clear.

[kenjis]

What should be the group for the added test?

sqlite> select * from migrations;
1|2018-01-24-102301|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_some_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1
2|2018-01-24-102302|Tests\Support\MigrationTestMigrations\Database\Migrations\Migration_another_migration|tests|Tests\Support\MigrationTestMigrations|1700128890|1

[kenjis]

It appears that resetDataCache() resolves the OCI8 test failures.
See https://github.com/kenjis/CodeIgniter4/pull/138/files#diff-ef065cd29dd2066d73750ad2480b61bf513b037402889ee0c0d9e0fb8b3fca3bR478-R479

[kenjis]

Frankly, Migration is complex and difficult to maintain consistency.

When temporarily changing a DB connection for testing purposes, it seems better to regard that the DB connection as the default group because it is easier to manipulate.

However, it seems a bit odd that the DB group name in the migration history is changed to the default group even if you specify the name of the DB group, e.g., db1.

Maybe the following patch is needed.

--- a/system/Database/MigrationRunner.php+++ b/system/Database/MigrationRunner.php@@ -141,10 +141,17 @@ class MigrationRunner
// Default name space is the app namespace
$this->namespace = APP_NAMESPACE;
- // get default database group- $config = config(Database::class);- $this->group = $config->defaultGroup;- unset($config);+ if (is_string($db)) {+ $this->group = $db;+ } else {+ // We use `defaultGroup` even if $db is ConnectionInterface or config+ // array. Because this is for testing purposes, and it is easier to+ // understand and operate as if the migrations were run on the+ // default group.+ $config = config(Database::class);+ $this->group = $config->defaultGroup;+ unset($config);+ }
// If no db connection passed in, use
// default database group.
@@ -841,7 +848,7 @@ class MigrationRunner
/** @var Migration $instance */
$instance = new $class(Database::forge($this->db));
- $group = $instance->getDBGroup() ?? config(Database::class)->defaultGroup;+ $group = $instance->getDBGroup() ?? $this->group;
if (ENVIRONMENT !== 'testing' && $group === 'tests' && $this->groupFilter !== 'tests') {
// @codeCoverageIgnoreStart
@@ -857,8 +864,6 @@ class MigrationRunner
return true;
}
- $this->setGroup($group);-
if (! is_callable([$instance, $direction])) {
$message = sprintf(lang('Migrations.missingMethod'), $direction);

[MGatner]

This is complicated. I'm tracking but let me know if you need me more. Thanks for handling.

[kenjis]

LGTM!