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

	public function testNotEnabled(): void
	{
	$modulesConfig = config('Modules');
	$modulesConfig->enabled = false;
	$config = new RegistrarConfig();
	$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 ??= new Modules();

[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
	*/
	protected static $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'));
     }