Update phpstan-codeigniter and fix errors on Modules

[paulbalandan]

Description
Explain what you have changed, and why.

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 don't understand the failure on testLoad. using dd shows that it is http://test.example.jp/this-is-test/

[kenjis]

@paulbalandan It is a bug. I sent a PR #8037

 3) CodeIgniter\Cache\FactoriesCacheFileVarExportHandlerTest::testLoad
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://test.example.jp/this-is-test/'
+'http://example.com/'
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cache/FactoriesCacheFileVarExportHandlerTest.php:80

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6490929878/job/17627453343?pr=8036

[MGatner]

(Copying my comment from the other PR)

My only qualm with this new rule is that it hides dependencies from Deptrac. I think Config is fine as a globally-available dependency so I'm not too worried about it, but I did like the easily-built dependency graph.

[kenjis]

Is there any benefit in changing to short names in this main repository?

Another drawback is that completion in the IDE will not work.
If we use ::class, IDE can autocomplete.

[kenjis]

@paulbalandan By the way, why is ::class call discouraged?
The only case this becomes a problem is if devs want to change the class in a module to be loaded at will.

[paulbalandan]

My inspiration for this is when I remember you made a PR on shield changing from config(AuthJWT::class) to config('AuthJWT') to allow overriding. So I thought why not made this a PHPStan rule.

[kenjis]

Yes, this is good for libraries (modules). Because config(\CodeIgniter\Shield\Config\Auth::class) do not load \Config\Auth (now by default).

But if there is \Config\Foo and devs use it, config(\Config\Foo::class) is no problem.

[paulbalandan]

Ok. So Config\ should be the "champion". In cases where it is module-specific, that needs to be flagged. Hmm, I think that's doable.

[kenjis]

There were 2 errors:
1) CodeIgniter\Config\BaseConfigTest::testNotEnabled
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:273
2) CodeIgniter\Config\BaseConfigTest::testDidDiscovery
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:288
ERRORS!
Tests: 548, Assertions: 930, Errors: 2.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6596168617/job/17924550098?pr=8036

[kenjis]

BaseConfig does not share the Config\Modules.
So there is no way to change the Config\Modules values during testing.

CodeIgniter4/tests/system/Config/BaseConfigTest.php

Lines 268 to 281 in 4c09ca3

	publicfunctiontestNotEnabled(): void
	{
	$modulesConfig = config('Modules');
	$modulesConfig->enabled = false;
	$config = newRegistrarConfig();
	$config::$registrars = [];
	$expected = $config::$registrars;
	$method = $this->getPrivateMethodInvoker($config, 'registerProperties');
	$method();
	$this->assertSame($expected, $config::$registrars);
	}

[kenjis]

The fix will be something like this, but it is a breaking change.

diff --git a/system/Config/BaseConfig.php b/system/Config/BaseConfig.php
index b4b9a4a213..2d0a0e9fe6 100644
--- a/system/Config/BaseConfig.php+++ b/system/Config/BaseConfig.php@@ -80,9 +80,9 @@ class BaseConfig
*
* The "shortPrefix" is the lowercase-only config class name.
*/
- public function __construct()+ public function __construct(?Modules $config = null)
{
- static::$moduleConfig = new Modules();+ static::$moduleConfig = $config ?? new Modules();
if (! static::$override) {
return;
diff --git a/tests/system/Config/BaseConfigTest.php b/tests/system/Config/BaseConfigTest.php
index 2da390392e..4c66673189 100644
--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -12,6 +12,7 @@
namespace CodeIgniter\Config;
use CodeIgniter\Test\CIUnitTestCase;
+use Config\Modules;
use Encryption;
use RegistrarConfig;
use RuntimeException;
@@ -267,10 +268,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testNotEnabled(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = false;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$expected = $config::$registrars;
@@ -282,10 +283,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testDidDiscovery(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = true;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$this->setPrivateProperty($config, 'didDiscovery', false);

[MGatner]

We can't modify that constructor. What about a static @testTag setter? Then the constructor could be:

static::$moduleConfig ??= newModules();

[paulbalandan]

Okay. I'll change.

[paulbalandan]

Would it be okay to change static::$moduleConfig as nullable? phpstan complains on ??=

[kenjis]

It has only @var, so it is okay.

CodeIgniter4/system/Config/BaseConfig.php

Lines 56 to 60 in 6f81e6e

	* The modules configuration.
	*
	* @var Modules
	*/
	protectedstatic$moduleConfig;

[github-actions]

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

[kenjis]

The following is easier to understand.

--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -290,9 +290,9 @@ final class BaseConfigTest extends CIUnitTestCase
$locator->method('search')->with('Config/Registrar.php')->willReturn([]);
Services::injectMock('locator', $locator);
+ $this->setPrivateProperty(RegistrarConfig::class, 'didDiscovery', false);+
$config = new RegistrarConfig();
- $this->setPrivateProperty($config, 'didDiscovery', false);- ($this->getPrivateMethodInvoker($config, 'registerProperties'))();
$this->assertTrue($this->getPrivateProperty($config, 'didDiscovery'));
}

[paulbalandan]

Description
Explain what you have changed, and why.

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 don't understand the failure on testLoad. using dd shows that it is http://test.example.jp/this-is-test/

[kenjis]

@paulbalandan It is a bug. I sent a PR #8037

 3) CodeIgniter\Cache\FactoriesCacheFileVarExportHandlerTest::testLoad
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://test.example.jp/this-is-test/'
+'http://example.com/'
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cache/FactoriesCacheFileVarExportHandlerTest.php:80

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6490929878/job/17627453343?pr=8036

[MGatner]

(Copying my comment from the other PR)

My only qualm with this new rule is that it hides dependencies from Deptrac. I think Config is fine as a globally-available dependency so I'm not too worried about it, but I did like the easily-built dependency graph.

[kenjis]

Is there any benefit in changing to short names in this main repository?

Another drawback is that completion in the IDE will not work.
If we use ::class, IDE can autocomplete.

[kenjis]

@paulbalandan By the way, why is ::class call discouraged?
The only case this becomes a problem is if devs want to change the class in a module to be loaded at will.

[paulbalandan]

My inspiration for this is when I remember you made a PR on shield changing from config(AuthJWT::class) to config('AuthJWT') to allow overriding. So I thought why not made this a PHPStan rule.

[kenjis]

Yes, this is good for libraries (modules). Because config(\CodeIgniter\Shield\Config\Auth::class) do not load \Config\Auth (now by default).

But if there is \Config\Foo and devs use it, config(\Config\Foo::class) is no problem.

[paulbalandan]

Ok. So Config\ should be the "champion". In cases where it is module-specific, that needs to be flagged. Hmm, I think that's doable.

[kenjis]

There were 2 errors:
1) CodeIgniter\Config\BaseConfigTest::testNotEnabled
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:273
2) CodeIgniter\Config\BaseConfigTest::testDidDiscovery
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:288
ERRORS!
Tests: 548, Assertions: 930, Errors: 2.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6596168617/job/17924550098?pr=8036

[kenjis]

BaseConfig does not share the Config\Modules.
So there is no way to change the Config\Modules values during testing.

CodeIgniter4/tests/system/Config/BaseConfigTest.php

Lines 268 to 281 in 4c09ca3

	publicfunctiontestNotEnabled(): void
	{
	$modulesConfig = config('Modules');
	$modulesConfig->enabled = false;
	$config = newRegistrarConfig();
	$config::$registrars = [];
	$expected = $config::$registrars;
	$method = $this->getPrivateMethodInvoker($config, 'registerProperties');
	$method();
	$this->assertSame($expected, $config::$registrars);
	}

[kenjis]

The fix will be something like this, but it is a breaking change.

diff --git a/system/Config/BaseConfig.php b/system/Config/BaseConfig.php
index b4b9a4a213..2d0a0e9fe6 100644
--- a/system/Config/BaseConfig.php+++ b/system/Config/BaseConfig.php@@ -80,9 +80,9 @@ class BaseConfig
*
* The "shortPrefix" is the lowercase-only config class name.
*/
- public function __construct()+ public function __construct(?Modules $config = null)
{
- static::$moduleConfig = new Modules();+ static::$moduleConfig = $config ?? new Modules();
if (! static::$override) {
return;
diff --git a/tests/system/Config/BaseConfigTest.php b/tests/system/Config/BaseConfigTest.php
index 2da390392e..4c66673189 100644
--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -12,6 +12,7 @@
namespace CodeIgniter\Config;
use CodeIgniter\Test\CIUnitTestCase;
+use Config\Modules;
use Encryption;
use RegistrarConfig;
use RuntimeException;
@@ -267,10 +268,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testNotEnabled(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = false;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$expected = $config::$registrars;
@@ -282,10 +283,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testDidDiscovery(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = true;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$this->setPrivateProperty($config, 'didDiscovery', false);

[MGatner]

We can't modify that constructor. What about a static @testTag setter? Then the constructor could be:

static::$moduleConfig ??= newModules();

[paulbalandan]

Okay. I'll change.

[paulbalandan]

Would it be okay to change static::$moduleConfig as nullable? phpstan complains on ??=

[kenjis]

It has only @var, so it is okay.

CodeIgniter4/system/Config/BaseConfig.php

Lines 56 to 60 in 6f81e6e

	* The modules configuration.
	*
	* @var Modules
	*/
	protectedstatic$moduleConfig;

[github-actions]

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

[kenjis]

The following is easier to understand.

--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -290,9 +290,9 @@ final class BaseConfigTest extends CIUnitTestCase
$locator->method('search')->with('Config/Registrar.php')->willReturn([]);
Services::injectMock('locator', $locator);
+ $this->setPrivateProperty(RegistrarConfig::class, 'didDiscovery', false);+
$config = new RegistrarConfig();
- $this->setPrivateProperty($config, 'didDiscovery', false);- ($this->getPrivateMethodInvoker($config, 'registerProperties'))();
$this->assertTrue($this->getPrivateProperty($config, 'didDiscovery'));
}

[paulbalandan]

Description
Explain what you have changed, and why.

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 don't understand the failure on testLoad. using dd shows that it is http://test.example.jp/this-is-test/

[kenjis]

@paulbalandan It is a bug. I sent a PR #8037

 3) CodeIgniter\Cache\FactoriesCacheFileVarExportHandlerTest::testLoad
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://test.example.jp/this-is-test/'
+'http://example.com/'
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cache/FactoriesCacheFileVarExportHandlerTest.php:80

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6490929878/job/17627453343?pr=8036

[MGatner]

(Copying my comment from the other PR)

My only qualm with this new rule is that it hides dependencies from Deptrac. I think Config is fine as a globally-available dependency so I'm not too worried about it, but I did like the easily-built dependency graph.

[kenjis]

Is there any benefit in changing to short names in this main repository?

Another drawback is that completion in the IDE will not work.
If we use ::class, IDE can autocomplete.

[kenjis]

@paulbalandan By the way, why is ::class call discouraged?
The only case this becomes a problem is if devs want to change the class in a module to be loaded at will.

[paulbalandan]

My inspiration for this is when I remember you made a PR on shield changing from config(AuthJWT::class) to config('AuthJWT') to allow overriding. So I thought why not made this a PHPStan rule.

[kenjis]

Yes, this is good for libraries (modules). Because config(\CodeIgniter\Shield\Config\Auth::class) do not load \Config\Auth (now by default).

But if there is \Config\Foo and devs use it, config(\Config\Foo::class) is no problem.

[paulbalandan]

Ok. So Config\ should be the "champion". In cases where it is module-specific, that needs to be flagged. Hmm, I think that's doable.

[kenjis]

There were 2 errors:
1) CodeIgniter\Config\BaseConfigTest::testNotEnabled
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:273
2) CodeIgniter\Config\BaseConfigTest::testDidDiscovery
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:288
ERRORS!
Tests: 548, Assertions: 930, Errors: 2.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6596168617/job/17924550098?pr=8036

[kenjis]

BaseConfig does not share the Config\Modules.
So there is no way to change the Config\Modules values during testing.

CodeIgniter4/tests/system/Config/BaseConfigTest.php

Lines 268 to 281 in 4c09ca3

	publicfunctiontestNotEnabled(): void
	{
	$modulesConfig = config('Modules');
	$modulesConfig->enabled = false;
	$config = newRegistrarConfig();
	$config::$registrars = [];
	$expected = $config::$registrars;
	$method = $this->getPrivateMethodInvoker($config, 'registerProperties');
	$method();
	$this->assertSame($expected, $config::$registrars);
	}

[kenjis]

The fix will be something like this, but it is a breaking change.

diff --git a/system/Config/BaseConfig.php b/system/Config/BaseConfig.php
index b4b9a4a213..2d0a0e9fe6 100644
--- a/system/Config/BaseConfig.php+++ b/system/Config/BaseConfig.php@@ -80,9 +80,9 @@ class BaseConfig
*
* The "shortPrefix" is the lowercase-only config class name.
*/
- public function __construct()+ public function __construct(?Modules $config = null)
{
- static::$moduleConfig = new Modules();+ static::$moduleConfig = $config ?? new Modules();
if (! static::$override) {
return;
diff --git a/tests/system/Config/BaseConfigTest.php b/tests/system/Config/BaseConfigTest.php
index 2da390392e..4c66673189 100644
--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -12,6 +12,7 @@
namespace CodeIgniter\Config;
use CodeIgniter\Test\CIUnitTestCase;
+use Config\Modules;
use Encryption;
use RegistrarConfig;
use RuntimeException;
@@ -267,10 +268,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testNotEnabled(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = false;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$expected = $config::$registrars;
@@ -282,10 +283,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testDidDiscovery(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = true;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$this->setPrivateProperty($config, 'didDiscovery', false);

[MGatner]

We can't modify that constructor. What about a static @testTag setter? Then the constructor could be:

static::$moduleConfig ??= newModules();

[paulbalandan]

Okay. I'll change.

[paulbalandan]

Would it be okay to change static::$moduleConfig as nullable? phpstan complains on ??=

[kenjis]

It has only @var, so it is okay.

CodeIgniter4/system/Config/BaseConfig.php

Lines 56 to 60 in 6f81e6e

	* The modules configuration.
	*
	* @var Modules
	*/
	protectedstatic$moduleConfig;

[github-actions]

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

[kenjis]

The following is easier to understand.

--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -290,9 +290,9 @@ final class BaseConfigTest extends CIUnitTestCase
$locator->method('search')->with('Config/Registrar.php')->willReturn([]);
Services::injectMock('locator', $locator);
+ $this->setPrivateProperty(RegistrarConfig::class, 'didDiscovery', false);+
$config = new RegistrarConfig();
- $this->setPrivateProperty($config, 'didDiscovery', false);- ($this->getPrivateMethodInvoker($config, 'registerProperties'))();
$this->assertTrue($this->getPrivateProperty($config, 'didDiscovery'));
}

[paulbalandan]

Description
Explain what you have changed, and why.

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 don't understand the failure on testLoad. using dd shows that it is http://test.example.jp/this-is-test/

[kenjis]

@paulbalandan It is a bug. I sent a PR #8037

 3) CodeIgniter\Cache\FactoriesCacheFileVarExportHandlerTest::testLoad
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://test.example.jp/this-is-test/'
+'http://example.com/'
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cache/FactoriesCacheFileVarExportHandlerTest.php:80

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6490929878/job/17627453343?pr=8036

[MGatner]

(Copying my comment from the other PR)

My only qualm with this new rule is that it hides dependencies from Deptrac. I think Config is fine as a globally-available dependency so I'm not too worried about it, but I did like the easily-built dependency graph.

[kenjis]

Is there any benefit in changing to short names in this main repository?

Another drawback is that completion in the IDE will not work.
If we use ::class, IDE can autocomplete.

[kenjis]

@paulbalandan By the way, why is ::class call discouraged?
The only case this becomes a problem is if devs want to change the class in a module to be loaded at will.

[paulbalandan]

My inspiration for this is when I remember you made a PR on shield changing from config(AuthJWT::class) to config('AuthJWT') to allow overriding. So I thought why not made this a PHPStan rule.

[kenjis]

Yes, this is good for libraries (modules). Because config(\CodeIgniter\Shield\Config\Auth::class) do not load \Config\Auth (now by default).

But if there is \Config\Foo and devs use it, config(\Config\Foo::class) is no problem.

[paulbalandan]

Ok. So Config\ should be the "champion". In cases where it is module-specific, that needs to be flagged. Hmm, I think that's doable.

[kenjis]

There were 2 errors:
1) CodeIgniter\Config\BaseConfigTest::testNotEnabled
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:273
2) CodeIgniter\Config\BaseConfigTest::testDidDiscovery
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:288
ERRORS!
Tests: 548, Assertions: 930, Errors: 2.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6596168617/job/17924550098?pr=8036

[kenjis]

BaseConfig does not share the Config\Modules.
So there is no way to change the Config\Modules values during testing.

CodeIgniter4/tests/system/Config/BaseConfigTest.php

Lines 268 to 281 in 4c09ca3

	publicfunctiontestNotEnabled(): void
	{
	$modulesConfig = config('Modules');
	$modulesConfig->enabled = false;
	$config = newRegistrarConfig();
	$config::$registrars = [];
	$expected = $config::$registrars;
	$method = $this->getPrivateMethodInvoker($config, 'registerProperties');
	$method();
	$this->assertSame($expected, $config::$registrars);
	}

[kenjis]

The fix will be something like this, but it is a breaking change.

diff --git a/system/Config/BaseConfig.php b/system/Config/BaseConfig.php
index b4b9a4a213..2d0a0e9fe6 100644
--- a/system/Config/BaseConfig.php+++ b/system/Config/BaseConfig.php@@ -80,9 +80,9 @@ class BaseConfig
*
* The "shortPrefix" is the lowercase-only config class name.
*/
- public function __construct()+ public function __construct(?Modules $config = null)
{
- static::$moduleConfig = new Modules();+ static::$moduleConfig = $config ?? new Modules();
if (! static::$override) {
return;
diff --git a/tests/system/Config/BaseConfigTest.php b/tests/system/Config/BaseConfigTest.php
index 2da390392e..4c66673189 100644
--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -12,6 +12,7 @@
namespace CodeIgniter\Config;
use CodeIgniter\Test\CIUnitTestCase;
+use Config\Modules;
use Encryption;
use RegistrarConfig;
use RuntimeException;
@@ -267,10 +268,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testNotEnabled(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = false;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$expected = $config::$registrars;
@@ -282,10 +283,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testDidDiscovery(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = true;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$this->setPrivateProperty($config, 'didDiscovery', false);

[MGatner]

We can't modify that constructor. What about a static @testTag setter? Then the constructor could be:

static::$moduleConfig ??= newModules();

[paulbalandan]

Okay. I'll change.

[paulbalandan]

Would it be okay to change static::$moduleConfig as nullable? phpstan complains on ??=

[kenjis]

It has only @var, so it is okay.

CodeIgniter4/system/Config/BaseConfig.php

Lines 56 to 60 in 6f81e6e

	* The modules configuration.
	*
	* @var Modules
	*/
	protectedstatic$moduleConfig;

[github-actions]

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

[kenjis]

The following is easier to understand.

--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -290,9 +290,9 @@ final class BaseConfigTest extends CIUnitTestCase
$locator->method('search')->with('Config/Registrar.php')->willReturn([]);
Services::injectMock('locator', $locator);
+ $this->setPrivateProperty(RegistrarConfig::class, 'didDiscovery', false);+
$config = new RegistrarConfig();
- $this->setPrivateProperty($config, 'didDiscovery', false);- ($this->getPrivateMethodInvoker($config, 'registerProperties'))();
$this->assertTrue($this->getPrivateProperty($config, 'didDiscovery'));
}

[paulbalandan]

Description
Explain what you have changed, and why.

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 don't understand the failure on testLoad. using dd shows that it is http://test.example.jp/this-is-test/

[kenjis]

@paulbalandan It is a bug. I sent a PR #8037

 3) CodeIgniter\Cache\FactoriesCacheFileVarExportHandlerTest::testLoad
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://test.example.jp/this-is-test/'
+'http://example.com/'
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cache/FactoriesCacheFileVarExportHandlerTest.php:80

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6490929878/job/17627453343?pr=8036

[MGatner]

(Copying my comment from the other PR)

My only qualm with this new rule is that it hides dependencies from Deptrac. I think Config is fine as a globally-available dependency so I'm not too worried about it, but I did like the easily-built dependency graph.

[kenjis]

Is there any benefit in changing to short names in this main repository?

Another drawback is that completion in the IDE will not work.
If we use ::class, IDE can autocomplete.

[kenjis]

@paulbalandan By the way, why is ::class call discouraged?
The only case this becomes a problem is if devs want to change the class in a module to be loaded at will.

[paulbalandan]

My inspiration for this is when I remember you made a PR on shield changing from config(AuthJWT::class) to config('AuthJWT') to allow overriding. So I thought why not made this a PHPStan rule.

[kenjis]

Yes, this is good for libraries (modules). Because config(\CodeIgniter\Shield\Config\Auth::class) do not load \Config\Auth (now by default).

But if there is \Config\Foo and devs use it, config(\Config\Foo::class) is no problem.

[paulbalandan]

Ok. So Config\ should be the "champion". In cases where it is module-specific, that needs to be flagged. Hmm, I think that's doable.

[kenjis]

There were 2 errors:
1) CodeIgniter\Config\BaseConfigTest::testNotEnabled
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:273
2) CodeIgniter\Config\BaseConfigTest::testDidDiscovery
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:288
ERRORS!
Tests: 548, Assertions: 930, Errors: 2.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6596168617/job/17924550098?pr=8036

[kenjis]

BaseConfig does not share the Config\Modules.
So there is no way to change the Config\Modules values during testing.

CodeIgniter4/tests/system/Config/BaseConfigTest.php

Lines 268 to 281 in 4c09ca3

	publicfunctiontestNotEnabled(): void
	{
	$modulesConfig = config('Modules');
	$modulesConfig->enabled = false;
	$config = newRegistrarConfig();
	$config::$registrars = [];
	$expected = $config::$registrars;
	$method = $this->getPrivateMethodInvoker($config, 'registerProperties');
	$method();
	$this->assertSame($expected, $config::$registrars);
	}

[kenjis]

The fix will be something like this, but it is a breaking change.

diff --git a/system/Config/BaseConfig.php b/system/Config/BaseConfig.php
index b4b9a4a213..2d0a0e9fe6 100644
--- a/system/Config/BaseConfig.php+++ b/system/Config/BaseConfig.php@@ -80,9 +80,9 @@ class BaseConfig
*
* The "shortPrefix" is the lowercase-only config class name.
*/
- public function __construct()+ public function __construct(?Modules $config = null)
{
- static::$moduleConfig = new Modules();+ static::$moduleConfig = $config ?? new Modules();
if (! static::$override) {
return;
diff --git a/tests/system/Config/BaseConfigTest.php b/tests/system/Config/BaseConfigTest.php
index 2da390392e..4c66673189 100644
--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -12,6 +12,7 @@
namespace CodeIgniter\Config;
use CodeIgniter\Test\CIUnitTestCase;
+use Config\Modules;
use Encryption;
use RegistrarConfig;
use RuntimeException;
@@ -267,10 +268,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testNotEnabled(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = false;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$expected = $config::$registrars;
@@ -282,10 +283,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testDidDiscovery(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = true;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$this->setPrivateProperty($config, 'didDiscovery', false);

[MGatner]

We can't modify that constructor. What about a static @testTag setter? Then the constructor could be:

static::$moduleConfig ??= newModules();

[paulbalandan]

Okay. I'll change.

[paulbalandan]

Would it be okay to change static::$moduleConfig as nullable? phpstan complains on ??=

[kenjis]

It has only @var, so it is okay.

CodeIgniter4/system/Config/BaseConfig.php

Lines 56 to 60 in 6f81e6e

	* The modules configuration.
	*
	* @var Modules
	*/
	protectedstatic$moduleConfig;

[github-actions]

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

[kenjis]

The following is easier to understand.

--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -290,9 +290,9 @@ final class BaseConfigTest extends CIUnitTestCase
$locator->method('search')->with('Config/Registrar.php')->willReturn([]);
Services::injectMock('locator', $locator);
+ $this->setPrivateProperty(RegistrarConfig::class, 'didDiscovery', false);+
$config = new RegistrarConfig();
- $this->setPrivateProperty($config, 'didDiscovery', false);- ($this->getPrivateMethodInvoker($config, 'registerProperties'))();
$this->assertTrue($this->getPrivateProperty($config, 'didDiscovery'));
}

[paulbalandan]

Description
Explain what you have changed, and why.

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 don't understand the failure on testLoad. using dd shows that it is http://test.example.jp/this-is-test/

[kenjis]

@paulbalandan It is a bug. I sent a PR #8037

 3) CodeIgniter\Cache\FactoriesCacheFileVarExportHandlerTest::testLoad
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://test.example.jp/this-is-test/'
+'http://example.com/'
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cache/FactoriesCacheFileVarExportHandlerTest.php:80

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6490929878/job/17627453343?pr=8036

[MGatner]

(Copying my comment from the other PR)

My only qualm with this new rule is that it hides dependencies from Deptrac. I think Config is fine as a globally-available dependency so I'm not too worried about it, but I did like the easily-built dependency graph.

[kenjis]

Is there any benefit in changing to short names in this main repository?

Another drawback is that completion in the IDE will not work.
If we use ::class, IDE can autocomplete.

[kenjis]

@paulbalandan By the way, why is ::class call discouraged?
The only case this becomes a problem is if devs want to change the class in a module to be loaded at will.

[paulbalandan]

My inspiration for this is when I remember you made a PR on shield changing from config(AuthJWT::class) to config('AuthJWT') to allow overriding. So I thought why not made this a PHPStan rule.

[kenjis]

Yes, this is good for libraries (modules). Because config(\CodeIgniter\Shield\Config\Auth::class) do not load \Config\Auth (now by default).

But if there is \Config\Foo and devs use it, config(\Config\Foo::class) is no problem.

[paulbalandan]

Ok. So Config\ should be the "champion". In cases where it is module-specific, that needs to be flagged. Hmm, I think that's doable.

[kenjis]

There were 2 errors:
1) CodeIgniter\Config\BaseConfigTest::testNotEnabled
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:273
2) CodeIgniter\Config\BaseConfigTest::testDidDiscovery
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:288
ERRORS!
Tests: 548, Assertions: 930, Errors: 2.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6596168617/job/17924550098?pr=8036

[kenjis]

BaseConfig does not share the Config\Modules.
So there is no way to change the Config\Modules values during testing.

CodeIgniter4/tests/system/Config/BaseConfigTest.php

Lines 268 to 281 in 4c09ca3

	publicfunctiontestNotEnabled(): void
	{
	$modulesConfig = config('Modules');
	$modulesConfig->enabled = false;
	$config = newRegistrarConfig();
	$config::$registrars = [];
	$expected = $config::$registrars;
	$method = $this->getPrivateMethodInvoker($config, 'registerProperties');
	$method();
	$this->assertSame($expected, $config::$registrars);
	}

[kenjis]

The fix will be something like this, but it is a breaking change.

diff --git a/system/Config/BaseConfig.php b/system/Config/BaseConfig.php
index b4b9a4a213..2d0a0e9fe6 100644
--- a/system/Config/BaseConfig.php+++ b/system/Config/BaseConfig.php@@ -80,9 +80,9 @@ class BaseConfig
*
* The "shortPrefix" is the lowercase-only config class name.
*/
- public function __construct()+ public function __construct(?Modules $config = null)
{
- static::$moduleConfig = new Modules();+ static::$moduleConfig = $config ?? new Modules();
if (! static::$override) {
return;
diff --git a/tests/system/Config/BaseConfigTest.php b/tests/system/Config/BaseConfigTest.php
index 2da390392e..4c66673189 100644
--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -12,6 +12,7 @@
namespace CodeIgniter\Config;
use CodeIgniter\Test\CIUnitTestCase;
+use Config\Modules;
use Encryption;
use RegistrarConfig;
use RuntimeException;
@@ -267,10 +268,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testNotEnabled(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = false;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$expected = $config::$registrars;
@@ -282,10 +283,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testDidDiscovery(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = true;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$this->setPrivateProperty($config, 'didDiscovery', false);

[MGatner]

We can't modify that constructor. What about a static @testTag setter? Then the constructor could be:

static::$moduleConfig ??= newModules();

[paulbalandan]

Okay. I'll change.

[paulbalandan]

Would it be okay to change static::$moduleConfig as nullable? phpstan complains on ??=

[kenjis]

It has only @var, so it is okay.

CodeIgniter4/system/Config/BaseConfig.php

Lines 56 to 60 in 6f81e6e

	* The modules configuration.
	*
	* @var Modules
	*/
	protectedstatic$moduleConfig;

[github-actions]

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

[kenjis]

The following is easier to understand.

--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -290,9 +290,9 @@ final class BaseConfigTest extends CIUnitTestCase
$locator->method('search')->with('Config/Registrar.php')->willReturn([]);
Services::injectMock('locator', $locator);
+ $this->setPrivateProperty(RegistrarConfig::class, 'didDiscovery', false);+
$config = new RegistrarConfig();
- $this->setPrivateProperty($config, 'didDiscovery', false);- ($this->getPrivateMethodInvoker($config, 'registerProperties'))();
$this->assertTrue($this->getPrivateProperty($config, 'didDiscovery'));
}

[paulbalandan]

Description
Explain what you have changed, and why.

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 don't understand the failure on testLoad. using dd shows that it is http://test.example.jp/this-is-test/

[kenjis]

@paulbalandan It is a bug. I sent a PR #8037

 3) CodeIgniter\Cache\FactoriesCacheFileVarExportHandlerTest::testLoad
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://test.example.jp/this-is-test/'
+'http://example.com/'
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cache/FactoriesCacheFileVarExportHandlerTest.php:80

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6490929878/job/17627453343?pr=8036

[MGatner]

(Copying my comment from the other PR)

My only qualm with this new rule is that it hides dependencies from Deptrac. I think Config is fine as a globally-available dependency so I'm not too worried about it, but I did like the easily-built dependency graph.

[kenjis]

Is there any benefit in changing to short names in this main repository?

Another drawback is that completion in the IDE will not work.
If we use ::class, IDE can autocomplete.

[kenjis]

@paulbalandan By the way, why is ::class call discouraged?
The only case this becomes a problem is if devs want to change the class in a module to be loaded at will.

[paulbalandan]

My inspiration for this is when I remember you made a PR on shield changing from config(AuthJWT::class) to config('AuthJWT') to allow overriding. So I thought why not made this a PHPStan rule.

[kenjis]

Yes, this is good for libraries (modules). Because config(\CodeIgniter\Shield\Config\Auth::class) do not load \Config\Auth (now by default).

But if there is \Config\Foo and devs use it, config(\Config\Foo::class) is no problem.

[paulbalandan]

Ok. So Config\ should be the "champion". In cases where it is module-specific, that needs to be flagged. Hmm, I think that's doable.

[kenjis]

There were 2 errors:
1) CodeIgniter\Config\BaseConfigTest::testNotEnabled
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:273
2) CodeIgniter\Config\BaseConfigTest::testDidDiscovery
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:288
ERRORS!
Tests: 548, Assertions: 930, Errors: 2.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6596168617/job/17924550098?pr=8036

[kenjis]

BaseConfig does not share the Config\Modules.
So there is no way to change the Config\Modules values during testing.

CodeIgniter4/tests/system/Config/BaseConfigTest.php

Lines 268 to 281 in 4c09ca3

	publicfunctiontestNotEnabled(): void
	{
	$modulesConfig = config('Modules');
	$modulesConfig->enabled = false;
	$config = newRegistrarConfig();
	$config::$registrars = [];
	$expected = $config::$registrars;
	$method = $this->getPrivateMethodInvoker($config, 'registerProperties');
	$method();
	$this->assertSame($expected, $config::$registrars);
	}

[kenjis]

The fix will be something like this, but it is a breaking change.

diff --git a/system/Config/BaseConfig.php b/system/Config/BaseConfig.php
index b4b9a4a213..2d0a0e9fe6 100644
--- a/system/Config/BaseConfig.php+++ b/system/Config/BaseConfig.php@@ -80,9 +80,9 @@ class BaseConfig
*
* The "shortPrefix" is the lowercase-only config class name.
*/
- public function __construct()+ public function __construct(?Modules $config = null)
{
- static::$moduleConfig = new Modules();+ static::$moduleConfig = $config ?? new Modules();
if (! static::$override) {
return;
diff --git a/tests/system/Config/BaseConfigTest.php b/tests/system/Config/BaseConfigTest.php
index 2da390392e..4c66673189 100644
--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -12,6 +12,7 @@
namespace CodeIgniter\Config;
use CodeIgniter\Test\CIUnitTestCase;
+use Config\Modules;
use Encryption;
use RegistrarConfig;
use RuntimeException;
@@ -267,10 +268,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testNotEnabled(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = false;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$expected = $config::$registrars;
@@ -282,10 +283,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testDidDiscovery(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = true;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$this->setPrivateProperty($config, 'didDiscovery', false);

[MGatner]

We can't modify that constructor. What about a static @testTag setter? Then the constructor could be:

static::$moduleConfig ??= newModules();

[paulbalandan]

Okay. I'll change.

[paulbalandan]

Would it be okay to change static::$moduleConfig as nullable? phpstan complains on ??=

[kenjis]

It has only @var, so it is okay.

CodeIgniter4/system/Config/BaseConfig.php

Lines 56 to 60 in 6f81e6e

	* The modules configuration.
	*
	* @var Modules
	*/
	protectedstatic$moduleConfig;

[github-actions]

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

[kenjis]

The following is easier to understand.

--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -290,9 +290,9 @@ final class BaseConfigTest extends CIUnitTestCase
$locator->method('search')->with('Config/Registrar.php')->willReturn([]);
Services::injectMock('locator', $locator);
+ $this->setPrivateProperty(RegistrarConfig::class, 'didDiscovery', false);+
$config = new RegistrarConfig();
- $this->setPrivateProperty($config, 'didDiscovery', false);- ($this->getPrivateMethodInvoker($config, 'registerProperties'))();
$this->assertTrue($this->getPrivateProperty($config, 'didDiscovery'));
}

[paulbalandan]

Description
Explain what you have changed, and why.

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 don't understand the failure on testLoad. using dd shows that it is http://test.example.jp/this-is-test/

[kenjis]

@paulbalandan It is a bug. I sent a PR #8037

 3) CodeIgniter\Cache\FactoriesCacheFileVarExportHandlerTest::testLoad
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://test.example.jp/this-is-test/'
+'http://example.com/'
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cache/FactoriesCacheFileVarExportHandlerTest.php:80

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6490929878/job/17627453343?pr=8036

[MGatner]

(Copying my comment from the other PR)

My only qualm with this new rule is that it hides dependencies from Deptrac. I think Config is fine as a globally-available dependency so I'm not too worried about it, but I did like the easily-built dependency graph.

[kenjis]

Is there any benefit in changing to short names in this main repository?

Another drawback is that completion in the IDE will not work.
If we use ::class, IDE can autocomplete.

[kenjis]

@paulbalandan By the way, why is ::class call discouraged?
The only case this becomes a problem is if devs want to change the class in a module to be loaded at will.

[paulbalandan]

My inspiration for this is when I remember you made a PR on shield changing from config(AuthJWT::class) to config('AuthJWT') to allow overriding. So I thought why not made this a PHPStan rule.

[kenjis]

Yes, this is good for libraries (modules). Because config(\CodeIgniter\Shield\Config\Auth::class) do not load \Config\Auth (now by default).

But if there is \Config\Foo and devs use it, config(\Config\Foo::class) is no problem.

[paulbalandan]

Ok. So Config\ should be the "champion". In cases where it is module-specific, that needs to be flagged. Hmm, I think that's doable.

[kenjis]

There were 2 errors:
1) CodeIgniter\Config\BaseConfigTest::testNotEnabled
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:273
2) CodeIgniter\Config\BaseConfigTest::testDidDiscovery
RuntimeException: Registrars must return an array of properties and their values.
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:233
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Config/BaseConfig.php:91
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Config/BaseConfigTest.php:288
ERRORS!
Tests: 548, Assertions: 930, Errors: 2.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/6596168617/job/17924550098?pr=8036

[kenjis]

BaseConfig does not share the Config\Modules.
So there is no way to change the Config\Modules values during testing.

CodeIgniter4/tests/system/Config/BaseConfigTest.php

Lines 268 to 281 in 4c09ca3

	publicfunctiontestNotEnabled(): void
	{
	$modulesConfig = config('Modules');
	$modulesConfig->enabled = false;
	$config = newRegistrarConfig();
	$config::$registrars = [];
	$expected = $config::$registrars;
	$method = $this->getPrivateMethodInvoker($config, 'registerProperties');
	$method();
	$this->assertSame($expected, $config::$registrars);
	}

[kenjis]

The fix will be something like this, but it is a breaking change.

diff --git a/system/Config/BaseConfig.php b/system/Config/BaseConfig.php
index b4b9a4a213..2d0a0e9fe6 100644
--- a/system/Config/BaseConfig.php+++ b/system/Config/BaseConfig.php@@ -80,9 +80,9 @@ class BaseConfig
*
* The "shortPrefix" is the lowercase-only config class name.
*/
- public function __construct()+ public function __construct(?Modules $config = null)
{
- static::$moduleConfig = new Modules();+ static::$moduleConfig = $config ?? new Modules();
if (! static::$override) {
return;
diff --git a/tests/system/Config/BaseConfigTest.php b/tests/system/Config/BaseConfigTest.php
index 2da390392e..4c66673189 100644
--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -12,6 +12,7 @@
namespace CodeIgniter\Config;
use CodeIgniter\Test\CIUnitTestCase;
+use Config\Modules;
use Encryption;
use RegistrarConfig;
use RuntimeException;
@@ -267,10 +268,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testNotEnabled(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = false;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$expected = $config::$registrars;
@@ -282,10 +283,10 @@ final class BaseConfigTest extends CIUnitTestCase
public function testDidDiscovery(): void
{
- $modulesConfig = config('Modules');+ $modulesConfig = new Modules();
$modulesConfig->enabled = true;
- $config = new RegistrarConfig();+ $config = new RegistrarConfig($modulesConfig);
$config::$registrars = [];
$this->setPrivateProperty($config, 'didDiscovery', false);

[MGatner]

We can't modify that constructor. What about a static @testTag setter? Then the constructor could be:

static::$moduleConfig ??= newModules();

[paulbalandan]

Okay. I'll change.

[paulbalandan]

Would it be okay to change static::$moduleConfig as nullable? phpstan complains on ??=

[kenjis]

It has only @var, so it is okay.

CodeIgniter4/system/Config/BaseConfig.php

Lines 56 to 60 in 6f81e6e

	* The modules configuration.
	*
	* @var Modules
	*/
	protectedstatic$moduleConfig;

[github-actions]

👋 Hi, @paulbalandan!

We detected conflicts in your PR against the base branch 🙊
You may want to sync 🔄 your branch with upstream!

Ref: Syncing Your Branch

[kenjis]

The following is easier to understand.

--- a/tests/system/Config/BaseConfigTest.php+++ b/tests/system/Config/BaseConfigTest.php@@ -290,9 +290,9 @@ final class BaseConfigTest extends CIUnitTestCase
$locator->method('search')->with('Config/Registrar.php')->willReturn([]);
Services::injectMock('locator', $locator);
+ $this->setPrivateProperty(RegistrarConfig::class, 'didDiscovery', false);+
$config = new RegistrarConfig();
- $this->setPrivateProperty($config, 'didDiscovery', false);- ($this->getPrivateMethodInvoker($config, 'registerProperties'))();
$this->assertTrue($this->getPrivateProperty($config, 'didDiscovery'));
}