Add PHP 8.2 to Unit Tests GHA

[paulbalandan]

See #6170

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

[kenjis]

There are too many dynamic properties!

There were 889 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/6996511004?check_suite_focus=true

[samsonasik]

Rector can be used to transform AllowDynamicProperties if needed

https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#addallowdynamicpropertiesattributerector

[kenjis]

I think #[AllowDynamicProperties] should be used as little as possible.

[paulbalandan]

Or maybe this is better: https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#completedynamicpropertiesrector

[kenjis]

Yes, probably CompleteDynamicPropertiesRector can fix most of the errors.

[MGatner]

Wow we passed! Very nice. How about adding to the PHPStan Action as well so we can catch deprecations?

[MGatner]

Oh haha we didn't pass at all 🤦‍♂️ Result was ignored. Digging through these now...

[MGatner]

Okay this isn't so bad. Lots of the same culprits over and over. Some specifics...

1. I think we need to allow dynamics on BaseConfig (I assume that will extend t child classes?) to keep Registrars working. Maybe there is an alternative where we create some special __set() method and mark it private/internal for Registrars only or something? But I don't see a way around Config needing dynamic properties.
2. Anything in tests/ I'm happy to have Rector allow or add, since they are pretty much all just mocks.
3. Predis looks like it isn't our handler that is the issue, but maybe I'm reading that wrong?
4. Image handlers need some digging but I think that's a pretty easy fix
5. Everything else should be considered for Rector adding, but I don't like the idea of generating a ton of public properties throughout the code, so they are probably worth examining case by case.

This is a good opportunity to improve code quality/smells - I wish we already had some dynamic property rules in place beforehand!

[kenjis]

I think we need to allow dynamics on BaseConfig (I assume that will extend t child classes?) to keep Registrars working. Maybe there is an alternative where we create some special __set() method and mark it private/internal for Registrars only or something? But I don't see a way around Config needing dynamic properties.

I don't know why Configs (or Registrars) need dynamic properties.
What is wrong with just overriding existing properties?
When properties are added dynamically, it is not possible to see all the properties by looking at the class definition.

[MGatner]

Maybe BaseConfig is overkill but there might be some (like our Validation discussion, or Database) that merit dynamic properties. Any Config that a module a) needs to expand but b) needs to allow App to override.

[kenjis]

Maybe BaseConfig is overkill but there might be some (like our Validation discussion, or Database) that merit dynamic properties. Any Config that a module a) needs to expand but b) needs to allow App to override.

I'm not convinced. In the Shield validation case, it seems property overriding is enough.
I don't know why a user needs to use Registrar. It seems good just to add the validation rules in \Config\Validation class.

[MGatner]

I just went through all the Config files and I think the two I mentioned (Database and Validation) are the only ones that function in a way that I think would benefit from dynamic properties. The classes that drive these use properties dynamically (e.g. a database connection is formed from the properties that match a database group) so the only way a module can add additional functionality to those classes is to define a corresponding property. The alternative is to specify that the developer add them manually during setup, which isn't the worst, but it adds a layer to module integration that tends to deter people.

[kenjis]

I don't understand the need to add dynamic properties to Database config.
Third party module adds another database connection config? For what?

Like Validation config, I don't understand the need for it at all.
Does anyone else use the feature? Any use case?

[MGatner]

I'm fine skipping it and then revisiting if the need arises. I certainly have never needed it before. My opinion was based on how these Config files work, which is different than other files.

[kenjis]

About 200 errors were reduced.

There were 660 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/7096495359?check_suite_focus=true

[MGatner]

The vfsStream issue has been fixed but not released (bovigo/vfsStream@4928328). I'm not sure about Predis, that one still surprises me.

[MGatner]

Predis constructor straight up sets it's own property that doesn't exist 🤦‍♂️

https://github.com/predis/predis/blob/main/src/Connection/Parameters.php

[paulbalandan]

Predis constructor straight up sets it's own property that doesn't exist 🤦‍♂️

https://github.com/predis/predis/blob/main/src/Connection/Parameters.php

Patched in main.

[MGatner]

By our very own @paulbalandan! predis/predis#781

Thanks Paul; glad to see their quick response too.

[MGatner]

Since the 8.2 tests are exempt should we go ahead and merge this? It would make tracking the errors easier instead of rebasing this branch over and over.

[kenjis]

But there are still 133 errors.

[MGatner]

But there are still 133 errors.

Right but because of this line:

        continue-on-error: ${{ matrix.php-versions == '8.2' }} # remove when PHP 8.2 is generally available

The errors won't cause the pipeline to fail, so they are only there as information if you open up the job logs. Merging this PR will let us keep an eye on those errors, since many of them are issues upstream that we cannot control. It will also let us send PRs to fix those errors that will actually run the GitHub Actions (beneficial for me since I don't have an 8.2 environment yet).

[MGatner]

It seems that the deprecations a caused by an older structure of Faker factories; we may be able to identify those and replace them with different faked content (if you look at Chris' linked PR s/he did the same).

[paulbalandan]

I used a temporary patch for Faker's troubled file. 7976a07

[kenjis]

Tests: 5368, Assertions: 8917, Errors: 14, Skipped: 14.

The last error is mb_convert_encoding(). #6672

1. CodeIgniter\HTTP\ContentSecurityPolicyTest::testBodyScriptNonceDisableAutoNonce
   ErrorException: mb_convert_encoding(): Handling HTML entities via mbstring is deprecated; use htmlspecialchars, htmlentities, or mb_encode_numericentity/mb_decode_numericentity instead

[kenjis]

@paulbalandan Applying patch on GA is not enough.
All users tests would still fail.
Why don't you send a PR to Faker?

[paulbalandan]

I have never used Faker so I'm not confident my patch will cover all edge cases for faker tests.

I'm not sure what you mean by "all users tests". If that would mean all consumers of Faker aside from us, then that is the least of my concern. Faker itself should provide the official patch. As I said, this addition is just a temporary hack.

[kenjis]

I mean all CI4 devs that use Faker.

[MGatner]

I'm fine with this to pass the framework. We don't actually offer any deprecated Faker usage directly: the Generator is available to users and may or may not be called. I am fine with this workaround (also: very cool use of git apply!) and maybe some notes in the User Guide if this is still an issue when 8.2 comes out.

[kenjis]

This is the test result of Shield. Most of the errors caused by Faker.

bash-3.2$ composer test
> phpunit
PHPUnit 9.5.25 #StandWithUkraine

Runtime:       PHP 8.2.0-dev
Configuration: /Users/kenji/work/codeigniter/official/codeigniter-shield/phpunit.xml
Random Seed:   1666576446

.................................................E....E.E......  63 / 251 ( 25%)
E.E.EE...E.........EE...EEE..............EE.............E.E...E 126 / 251 ( 50%)
...EE.E..E......E.E......................E.E.....E.....EE.E...E 189 / 251 ( 75%)
...........EE..........EE.E....................E..EEE.E.EEE...  251 / 251 (100%)

Time: 00:10.122, Memory: 38.00 MB

There were 44 errors:

[kenjis]

7) CodeIgniter\Database\Live\GetTest::testGetRowWithCustomReturnType
ErrorException: Creation of dynamic property class@anonymous::$id is deprecated

/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Database/SQLite3/Result.php:143
/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Database/SQLite3/Result.php:147
/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Database/BaseResult.php:149
/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Database/BaseResult.php:286
/Users/kenji/work/codeigniter/official/CodeIgniter4/system/Database/BaseResult.php:273
/Users/kenji/work/codeigniter/official/CodeIgniter4/tests/system/Database/Live/GetTest.php:234

[kenjis]

I merged #6757
So all error should be gone.

[MGatner]

Looks like one more dynamic property issue in each of the driver-specific Result classes. E.g.: /home/runner/work/CodeIgniter4/CodeIgniter4/system/Database/OCI8/Result.php:109

[MGatner]

Yay! We did it!

[MGatner]

Side note: scrolling through these test results over and over with all the "skips"... maybe we should tag database-specific tests as their own suites and run them per driver?

Or maybe that is what Paul was already working on?

[paulbalandan]

🚀

[kenjis]

I didn't know but it seems @runInSeparateProcess (or @preserveGlobalState?) tests need XDebug.

There were 52 skipped tests:

...

2) CodeIgniter\CommonFunctionsSendTest::testRedirectResponseCookiesSent
XDebug not found.

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/CIUnitTestCase.php:407
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/CommonFunctionsSendTest.php:60

3) CodeIgniter\HTTP\ContentSecurityPolicyTest::testExistence
XDebug not found.

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/CIUnitTestCase.php:407
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/HTTP/ContentSecurityPolicyTest.php:71

4) CodeIgniter\HTTP\ContentSecurityPolicyTest::testReportOnly
XDebug not found.

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/CIUnitTestCase.php:407
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/HTTP/ContentSecurityPolicyTest.php:84

5) CodeIgniter\HTTP\ContentSecurityPolicyTest::testDefaults
XDebug not found.

/home/runner/work/CodeIgniter4/CodeIgniter4/system/Test/CIUnitTestCase.php:531

...

OK, but incomplete, skipped, or risky tests!
Tests: 5372, Assertions: 8876, Skipped: 52.

https://github.com/codeigniter4/CodeIgniter4/actions/runs/3368104698/jobs/5586268619

[MGatner]

That's odd. Also: why don't we have Xdebug available? I thought setup-php added it.

Edit: Oh right, we just made it conditional 🤪