Update phpstan-codeigniter and fix errors on Modules by paulbalandan · Pull Request #8036 · codeigniter4/CodeIgniter4

paulbalandan · 2023-10-12T03:05:26Z

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

system/Events/Events.php

paulbalandan · 2023-10-12T03:57:23Z

I don't understand the failure on testLoad. using dd shows that it is http://test.example.jp/this-is-test/

kenjis · 2023-10-12T05:49:50Z

@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 · 2023-10-12T10:51:41Z

(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.

system/CodeIgniter.php

kenjis · 2023-10-12T11:21:10Z

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 · 2023-10-12T20:39:19Z

@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 · 2023-10-20T02:32:40Z

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 · 2023-10-20T06:18:11Z

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 · 2023-10-21T01:55:26Z

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 · 2023-10-21T23:39:32Z

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 · 2023-10-21T23:44:17Z

system/Config/BaseConfig.php

    public function __construct()
    {
-        static::$moduleConfig = config(Modules::class);
+        static::$moduleConfig = new Modules();


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);

}

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);

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

static::$moduleConfig ??= new Modules();

Okay. I'll change.

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

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 · 2023-10-22T03:54:23Z

👋 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 · 2023-10-25T00:01:49Z

tests/system/Config/BaseConfigTest.php

-        $method();
+        ($this->getPrivateMethodInvoker($config, 'registerProperties'))();

        $this->assertTrue($this->getPrivateProperty($config, 'didDiscovery'));


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 commented Oct 12, 2023

View reviewed changes

system/Events/Events.php Outdated Show resolved Hide resolved

paulbalandan force-pushed the update-phpstan-codeigniter branch 2 times, most recently from 01ac45a to f3a285c Compare October 12, 2023 03:34

kenjis mentioned this pull request Oct 12, 2023

fix: Factories caching bug #8037

Merged

5 tasks

MGatner reviewed Oct 12, 2023

View reviewed changes

system/CodeIgniter.php Outdated Show resolved Hide resolved

github-actions bot added the stale Pull requests with conflicts label Oct 16, 2023

This comment was marked as outdated.

Sign in to view

paulbalandan force-pushed the update-phpstan-codeigniter branch from f3a285c to 07ebcad Compare October 20, 2023 02:29

paulbalandan removed the stale Pull requests with conflicts label Oct 20, 2023

paulbalandan force-pushed the update-phpstan-codeigniter branch 2 times, most recently from e2ed1c0 to b9d05ec Compare October 20, 2023 02:55

paulbalandan force-pushed the update-phpstan-codeigniter branch 3 times, most recently from bcfbe29 to 550a624 Compare October 21, 2023 08:04

paulbalandan changed the title ~~Update phpstan-codeigniter and fix new errors~~ Update phpstan-codeigniter and fix errors on Modules Oct 21, 2023

kenjis added the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 21, 2023

kenjis reviewed Oct 21, 2023

View reviewed changes

github-actions bot added the stale Pull requests with conflicts label Oct 22, 2023

paulbalandan force-pushed the update-phpstan-codeigniter branch from 550a624 to 3773743 Compare October 22, 2023 11:02

paulbalandan removed the stale Pull requests with conflicts label Oct 22, 2023

kenjis approved these changes Oct 24, 2023

View reviewed changes

kenjis reviewed Oct 25, 2023

View reviewed changes

paulbalandan added 5 commits October 25, 2023 08:59

Update phpstan-codeigniter to v1.4.1

2969640

Fix Modules does not extend BaseConfig

94130de

Inject Modules into constructor

89f1cc6

Use static setter

3142108

Simplify test

61bbbca

paulbalandan force-pushed the update-phpstan-codeigniter branch from d328eac to 61bbbca Compare October 25, 2023 01:03

kenjis approved these changes Oct 25, 2023

View reviewed changes

MGatner approved these changes Oct 25, 2023

View reviewed changes

kenjis merged commit 130be04 into codeigniter4:develop Oct 25, 2023

kenjis mentioned this pull request Oct 25, 2023

test: fix BaseConfigTest #8095

Merged

2 tasks

paulbalandan deleted the update-phpstan-codeigniter branch October 26, 2023 00:08

	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);
	}

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

Conversation

paulbalandan commented Oct 12, 2023

Uh oh!

Uh oh!

paulbalandan commented Oct 12, 2023

Uh oh!

kenjis commented Oct 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MGatner commented Oct 12, 2023

Uh oh!

Uh oh!

kenjis commented Oct 12, 2023

Uh oh!

kenjis commented Oct 12, 2023

Uh oh!

This comment was marked as outdated.

paulbalandan commented Oct 20, 2023

Uh oh!

kenjis commented Oct 20, 2023

Uh oh!

paulbalandan commented Oct 21, 2023

Uh oh!

kenjis commented Oct 21, 2023

Uh oh!

kenjis Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

kenjis Oct 21, 2023

Choose a reason for hiding this comment

Uh oh!

MGatner Oct 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulbalandan Oct 23, 2023

Choose a reason for hiding this comment

Uh oh!

paulbalandan Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

kenjis Oct 24, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 22, 2023

Uh oh!

kenjis Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kenjis commented Oct 12, 2023 •

edited

Loading

MGatner Oct 22, 2023 •

edited

Loading