codeigniter4
Add PHP 8.2 to Unit Tests GHA
Checklist:
- [x] 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
There are too many dynamic properties!
There were 889 errors: https://github.com/codeigniter4/CodeIgniter4/runs/6996511004?check_suite_focus=true
Rector can be used to transform AllowDynamicProperties if needed
https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#addallowdynamicpropertiesattributerector
Or maybe this is better: https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#completedynamicpropertiesrector
Oh haha we didn't pass at all 🤦♂️ Result was ignored. Digging through these now...
Okay this isn't so bad. Lots of the same culprits over and over. Some specifics...
- 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. - Anything in tests/ I'm happy to have Rector allow or add, since they are pretty much all just mocks.
- Predis looks like it isn't our handler that is the issue, but maybe I'm reading that wrong?
- Image handlers need some digging but I think that's a pretty easy fix
- 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!
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.
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.
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.
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.
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?
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.
About 200 errors were reduced.
There were 660 errors: https://github.com/codeigniter4/CodeIgniter4/runs/7096495359?check_suite_focus=true
The vfsStream issue has been fixed but not released (https://github.com/bovigo/vfsStream/commit/49283282eba373c14552d57c8b17247f48acf543). I'm not sure about Predis, that one still surprises me.
Predis constructor straight up sets it's own property that doesn't exist 🤦♂️
https://github.com/predis/predis/blob/main/src/Connection/Parameters.php
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.
By our very own @paulbalandan! https://github.com/predis/predis/pull/781
Thanks Paul; glad to see their quick response too.
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.
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).
Merging this PR will let us keep an eye on those errors
I doubt it. If I make an error in my PR, I probably can't find the error out of more than 100 errors. I hope less errors, ideally 0, at least less than 10.
But those errors will only show on the PHPUnit 8.2 Action logs. You should be able to look at all the regular pipeline checks to discern your code impact, then the 8.2 run will be optionally available for future information.
Unless I misunderstood something you were saying?
I don't want to increase GA resource consumption by adding checks that almost no one sees. It will also make the checks take even longer.
Wow, greatly improved!
Tests: 5251, Assertions: 8657, Errors: 70, Failures: 28, Skipped: 14.
Predis just posted a release! https://github.com/predis/predis/compare/v2.0.0...v2.0.1
It includes Paul's fix for the dynamic property warning. Closing and reopening to run again...
Tests: 5286, Assertions: 8699, Errors: 70, Failures: 28, Skipped: 14.
No changes?
1) CodeIgniter\Cache\Handlers\PredisHandlerTest::testNew
ErrorException: Use of "static" in callables is deprecated
/home/runner/work/CodeIgniter4/CodeIgniter4/vendor/predis/predis/src/Command/Processor/KeyPrefixProcessor.php:200
/home/runner/work/CodeIgniter4/CodeIgniter4/vendor/predis/predis/src/Command/Factory.php:76
/home/runner/work/CodeIgniter4/CodeIgniter4/vendor/predis/predis/src/Client.php:308
/home/runner/work/CodeIgniter4/CodeIgniter4/vendor/predis/predis/src/Client.php:299
/home/runner/work/CodeIgniter4/CodeIgniter4/system/Cache/Handlers/PredisHandler.php:144
/home/runner/work/CodeIgniter4/CodeIgniter4/tests/system/Cache/Handlers/PredisHandlerTest.php:49
The error has changed, even though the count hasn't 🤦♂️ Looks like Predis is failing to deal with 8.2 properly.
Some Predis movement:
- https://github.com/predis/predis/pull/796
- https://github.com/predis/predis/pull/797
Looking good.