Skip to content

Add PHP 8.2 to Unit Tests GHA - #6172

Merged
paulbalandan merged 1 commit into
codeigniter4:developfrom
paulbalandan:GHA-8.2
Nov 1, 2022
Merged

Add PHP 8.2 to Unit Tests GHA#6172
paulbalandan merged 1 commit into
codeigniter4:developfrom
paulbalandan:GHA-8.2

Conversation

@paulbalandan

@paulbalandanpaulbalandan commented Jun 22, 2022•
edited by kenjis
Loading

Copy link
Copy Markdown
Member

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

@paulbalandanpaulbalandan added the github_actions Pull requests that update Github_actions code label Jun 22, 2022
@kenjis

kenjis commented Jun 22, 2022

Copy link
Copy Markdown
Member

There are too many dynamic properties!

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

@kenjiskenjis mentioned this pull request Jun 22, 2022
Closed
7 tasks
@samsonasik

samsonasik commented Jun 22, 2022

Copy link
Copy Markdown
Member

Rector can be used to transform AllowDynamicProperties if needed

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

@kenjis

kenjis commented Jun 22, 2022

Copy link
Copy Markdown
Member

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

@paulbalandan

paulbalandan commented Jun 22, 2022

Copy link
Copy Markdown
MemberAuthor

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

@kenjis

kenjis commented Jun 22, 2022

Copy link
Copy Markdown
Member

Yes, probably CompleteDynamicPropertiesRector can fix most of the errors.

MGatner
MGatner approved these changes Jun 22, 2022
View reviewed changes

@MGatnerMGatner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MGatner

MGatner commented Jun 22, 2022

Copy link
Copy Markdown
Member

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

@MGatner

MGatner commented Jun 22, 2022

Copy link
Copy Markdown
Member

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

kenjis commented Jun 22, 2022

Copy link
Copy Markdown
Member

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

MGatner commented Jun 22, 2022

Copy link
Copy Markdown
Member

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

kenjis commented Jun 22, 2022

Copy link
Copy Markdown
Member

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

MGatner commented Jun 23, 2022

Copy link
Copy Markdown
Member

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

kenjis commented Jun 24, 2022

Copy link
Copy Markdown
Member

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

MGatner commented Jun 24, 2022

Copy link
Copy Markdown
Member

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.

This was referenced Jun 27, 2022
Closed
Merged
@kenjis

kenjis commented Jun 29, 2022

Copy link
Copy Markdown
Member

About 200 errors were reduced.

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

@MGatner

MGatner commented Jun 29, 2022

Copy link
Copy Markdown
Member

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

@MGatner

MGatner commented Jun 29, 2022

Copy link
Copy Markdown
Member

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

paulbalandan commented Jun 30, 2022

Copy link
Copy Markdown
MemberAuthor

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

MGatner commented Jun 30, 2022

Copy link
Copy Markdown
Member

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

Thanks Paul; glad to see their quick response too.

@MGatner

MGatner commented Jul 8, 2022

Copy link
Copy Markdown
Member

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

kenjis commented Jul 9, 2022

Copy link
Copy Markdown
Member

But there are still 133 errors.

@MGatner

MGatner commented Jul 9, 2022

Copy link
Copy Markdown
Member

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

MGatner commented Oct 22, 2022

Copy link
Copy Markdown
Member

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
paulbalandanforce-pushed the GHA-8.2 branch 2 times, most recently from c3fabd0 to 7976a07CompareOctober 22, 2022 15:52
@paulbalandan

paulbalandan commented Oct 22, 2022

Copy link
Copy Markdown
MemberAuthor

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

@kenjis

kenjis commented Oct 22, 2022

Copy link
Copy Markdown
Member

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

kenjis commented Oct 23, 2022

Copy link
Copy Markdown
Member

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

@paulbalandan

paulbalandan commented Oct 23, 2022

Copy link
Copy Markdown
MemberAuthor

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

kenjis commented Oct 23, 2022

Copy link
Copy Markdown
Member

I mean all CI4 devs that use Faker.

@MGatner

MGatner commented Oct 23, 2022

Copy link
Copy Markdown
Member

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

kenjis commented Oct 24, 2022

Copy link
Copy Markdown
Member

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

bash-3.2$ composer test
> phpunitPHPUnit 9.5.25 #StandWithUkraineRuntime: PHP 8.2.0-devConfiguration: /Users/kenji/work/codeigniter/official/codeigniter-shield/phpunit.xmlRandom 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 MBThere were 44 errors:

@kenjiskenjis added the stale Pull requests with conflicts label Oct 25, 2022
@kenjis

kenjis commented Oct 25, 2022

Copy link
Copy Markdown
Member
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

@kenjiskenjis mentioned this pull request Oct 26, 2022
Merged
2 tasks
@kenjiskenjis removed the stale Pull requests with conflicts label Oct 26, 2022
@kenjis

kenjis commented Oct 26, 2022

Copy link
Copy Markdown
Member

I merged #6757
So all error should be gone.

@MGatnerMGatner closed this Oct 26, 2022
@MGatnerMGatner reopened this Oct 26, 2022
@MGatner

MGatner commented Oct 27, 2022

Copy link
Copy Markdown
Member

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

@codeigniter4codeigniter4 deleted a comment from martinemily3Oct 30, 2022
MGatner
MGatner approved these changes Oct 30, 2022
View reviewed changes

@MGatnerMGatner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! We did it!

@MGatner

MGatner commented Oct 30, 2022•
edited
Loading

Copy link
Copy Markdown
Member

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
paulbalandan merged commit 5e9d12f into codeigniter4:developNov 1, 2022
@paulbalandan
paulbalandan deleted the GHA-8.2 branch November 1, 2022 08:54
@paulbalandan

paulbalandan commented Nov 1, 2022

Copy link
Copy Markdown
MemberAuthor

🚀

@kenjis

kenjis commented Nov 1, 2022

Copy link
Copy Markdown
Member

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

MGatner commented Nov 1, 2022•
edited
Loading

Copy link
Copy Markdown
Member

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 🤪

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kenjiskenjiskenjis approved these changes
@MGatnerMGatnerMGatner approved these changes

Assignees

No one assigned

Labels

github_actionsPull requests that update Github_actions code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@paulbalandan@kenjis@samsonasik@MGatner
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Add PHP 8.2 to Unit Tests GHA by paulbalandan · Pull Request #6172 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
Add PHP 8.2 to Unit Tests GHA - #6172
Merged
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Nov 1, 2022
Merged
Add PHP 8.2 to Unit Tests GHA#6172
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Conversation
@paulbalandan
@paulbalandanpaulbalandan
commented
Jun 22, 2022•

edited by kenjis

Loading

Copy link


Copy Markdown

Member
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
@paulbalandanpaulbalandan
added
the 
github_actions
Pull requests that update Github_actions code
label
Jun 22, 2022
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
There are too many dynamic properties!
There were 889 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/6996511004?check_suite_focus=true
@kenjiskenjis
mentioned this pull request
Jun 22, 2022

Closed

7 tasks
@samsonasik
samsonasik
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Rector can be used to transform AllowDynamicProperties if needed
https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#addallowdynamicpropertiesattributerector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
I think #[AllowDynamicProperties] should be used as little as possible.
@paulbalandan
paulbalandan
commented
Jun 22, 2022

Copy link


Copy Markdown

MemberAuthor
Or maybe this is better: https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#completedynamicpropertiesrector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Yes, probably CompleteDynamicPropertiesRector can fix most of the errors.
MGatner
MGatner
approved these changes
Jun 22, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow we passed! Very nice. How about adding to the PHPStan Action as well so we can catch deprecations?
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Oh haha we didn't pass at all 🤦‍♂️ Result was ignored. Digging through these now...
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 24, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 24, 2022

Copy link


Copy Markdown

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

This was referenced Jun 27, 2022

Closed

 Merged

@kenjis
kenjis
commented
Jun 29, 2022

Copy link


Copy Markdown

Member
About 200 errors were reduced.
There were 660 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/7096495359?check_suite_focus=true
@MGatner
MGatner
commented
Jun 29, 2022

Copy link


Copy Markdown

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

Copy link


Copy Markdown

Member
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
paulbalandan
commented
Jun 30, 2022

Copy link


Copy Markdown

MemberAuthor
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
MGatner
commented
Jun 30, 2022

Copy link


Copy Markdown

Member
By our very own @paulbalandan! predis/predis#781
Thanks Paul; glad to see their quick response too.
@MGatner
MGatner
commented
Jul 8, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
But there are still 133 errors.
@MGatner
MGatner
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
paulbalandanforce-pushed
the
GHA-8.2
branch
2 times, most recently
from
c3fabd0 to
7976a07CompareOctober 22, 2022 15:52
@paulbalandan
paulbalandan
commented
Oct 22, 2022

Copy link


Copy Markdown

MemberAuthor
I used a temporary patch for Faker's troubled file. 7976a07
@kenjis
kenjis
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
@paulbalandan Applying patch on GA is not enough.

All users tests would still fail.

Why don't you send a PR to Faker?
@paulbalandan
paulbalandan
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
I mean all CI4 devs that use Faker.
@MGatner
MGatner
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 24, 2022

Copy link


Copy Markdown

Member
This is the test result of Shield. Most of the errors caused by Faker.
bash-3.2$ composer test
> phpunitPHPUnit 9.5.25 #StandWithUkraineRuntime: PHP 8.2.0-devConfiguration: /Users/kenji/work/codeigniter/official/codeigniter-shield/phpunit.xmlRandom 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 MBThere were 44 errors:
@kenjiskenjis
added
the 
stale
Pull requests with conflicts
label
Oct 25, 2022
@kenjis
kenjis
commented
Oct 25, 2022

Copy link


Copy Markdown

Member
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

@kenjiskenjis
mentioned this pull request
Oct 26, 2022
 Merged

2 tasks
@kenjiskenjis
removed
the 
stale
Pull requests with conflicts
label
Oct 26, 2022
@kenjis
kenjis
commented
Oct 26, 2022

Copy link


Copy Markdown

Member
I merged #6757

So all error should be gone.
@MGatnerMGatner
closed this
Oct 26, 2022
@MGatnerMGatner
reopened this
Oct 26, 2022
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
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
@codeigniter4codeigniter4
deleted a comment from martinemily3Oct 30, 2022
MGatner
MGatner
approved these changes
Oct 30, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! We did it!
@MGatner
MGatner
commented
Oct 30, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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
paulbalandan
merged commit 5e9d12f
into
codeigniter4:developNov 1, 2022
@paulbalandan
paulbalandan
deleted the

GHA-8.2

branch
November 1, 2022 08:54
@paulbalandan
paulbalandan
commented
Nov 1, 2022

Copy link


Copy Markdown

MemberAuthor
🚀
@kenjis
kenjis
commented
Nov 1, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Nov 1, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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 🤪
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kenjiskenjiskenjis approved these changes
@MGatnerMGatnerMGatner approved these changes

Assignees


No one assigned


Labels

github_actionsPull requests that update Github_actions code

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@paulbalandan@kenjis@samsonasik@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Add PHP 8.2 to Unit Tests GHA by paulbalandan · Pull Request #6172 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
Add PHP 8.2 to Unit Tests GHA - #6172
Merged
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Nov 1, 2022
Merged
Add PHP 8.2 to Unit Tests GHA#6172
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Conversation
@paulbalandan
@paulbalandanpaulbalandan
commented
Jun 22, 2022•

edited by kenjis

Loading

Copy link


Copy Markdown

Member
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
@paulbalandanpaulbalandan
added
the 
github_actions
Pull requests that update Github_actions code
label
Jun 22, 2022
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
There are too many dynamic properties!
There were 889 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/6996511004?check_suite_focus=true
@kenjiskenjis
mentioned this pull request
Jun 22, 2022

Closed

7 tasks
@samsonasik
samsonasik
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Rector can be used to transform AllowDynamicProperties if needed
https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#addallowdynamicpropertiesattributerector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
I think #[AllowDynamicProperties] should be used as little as possible.
@paulbalandan
paulbalandan
commented
Jun 22, 2022

Copy link


Copy Markdown

MemberAuthor
Or maybe this is better: https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#completedynamicpropertiesrector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Yes, probably CompleteDynamicPropertiesRector can fix most of the errors.
MGatner
MGatner
approved these changes
Jun 22, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow we passed! Very nice. How about adding to the PHPStan Action as well so we can catch deprecations?
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Oh haha we didn't pass at all 🤦‍♂️ Result was ignored. Digging through these now...
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 24, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 24, 2022

Copy link


Copy Markdown

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

This was referenced Jun 27, 2022

Closed

 Merged

@kenjis
kenjis
commented
Jun 29, 2022

Copy link


Copy Markdown

Member
About 200 errors were reduced.
There were 660 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/7096495359?check_suite_focus=true
@MGatner
MGatner
commented
Jun 29, 2022

Copy link


Copy Markdown

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

Copy link


Copy Markdown

Member
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
paulbalandan
commented
Jun 30, 2022

Copy link


Copy Markdown

MemberAuthor
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
MGatner
commented
Jun 30, 2022

Copy link


Copy Markdown

Member
By our very own @paulbalandan! predis/predis#781
Thanks Paul; glad to see their quick response too.
@MGatner
MGatner
commented
Jul 8, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
But there are still 133 errors.
@MGatner
MGatner
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
paulbalandanforce-pushed
the
GHA-8.2
branch
2 times, most recently
from
c3fabd0 to
7976a07CompareOctober 22, 2022 15:52
@paulbalandan
paulbalandan
commented
Oct 22, 2022

Copy link


Copy Markdown

MemberAuthor
I used a temporary patch for Faker's troubled file. 7976a07
@kenjis
kenjis
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
@paulbalandan Applying patch on GA is not enough.

All users tests would still fail.

Why don't you send a PR to Faker?
@paulbalandan
paulbalandan
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
I mean all CI4 devs that use Faker.
@MGatner
MGatner
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 24, 2022

Copy link


Copy Markdown

Member
This is the test result of Shield. Most of the errors caused by Faker.
bash-3.2$ composer test
> phpunitPHPUnit 9.5.25 #StandWithUkraineRuntime: PHP 8.2.0-devConfiguration: /Users/kenji/work/codeigniter/official/codeigniter-shield/phpunit.xmlRandom 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 MBThere were 44 errors:
@kenjiskenjis
added
the 
stale
Pull requests with conflicts
label
Oct 25, 2022
@kenjis
kenjis
commented
Oct 25, 2022

Copy link


Copy Markdown

Member
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

@kenjiskenjis
mentioned this pull request
Oct 26, 2022
 Merged

2 tasks
@kenjiskenjis
removed
the 
stale
Pull requests with conflicts
label
Oct 26, 2022
@kenjis
kenjis
commented
Oct 26, 2022

Copy link


Copy Markdown

Member
I merged #6757

So all error should be gone.
@MGatnerMGatner
closed this
Oct 26, 2022
@MGatnerMGatner
reopened this
Oct 26, 2022
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
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
@codeigniter4codeigniter4
deleted a comment from martinemily3Oct 30, 2022
MGatner
MGatner
approved these changes
Oct 30, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! We did it!
@MGatner
MGatner
commented
Oct 30, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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
paulbalandan
merged commit 5e9d12f
into
codeigniter4:developNov 1, 2022
@paulbalandan
paulbalandan
deleted the

GHA-8.2

branch
November 1, 2022 08:54
@paulbalandan
paulbalandan
commented
Nov 1, 2022

Copy link


Copy Markdown

MemberAuthor
🚀
@kenjis
kenjis
commented
Nov 1, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Nov 1, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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 🤪
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kenjiskenjiskenjis approved these changes
@MGatnerMGatnerMGatner approved these changes

Assignees


No one assigned


Labels

github_actionsPull requests that update Github_actions code

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@paulbalandan@kenjis@samsonasik@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Add PHP 8.2 to Unit Tests GHA by paulbalandan · Pull Request #6172 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
Add PHP 8.2 to Unit Tests GHA - #6172
Merged
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Nov 1, 2022
Merged
Add PHP 8.2 to Unit Tests GHA#6172
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Conversation
@paulbalandan
@paulbalandanpaulbalandan
commented
Jun 22, 2022•

edited by kenjis

Loading

Copy link


Copy Markdown

Member
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
@paulbalandanpaulbalandan
added
the 
github_actions
Pull requests that update Github_actions code
label
Jun 22, 2022
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
There are too many dynamic properties!
There were 889 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/6996511004?check_suite_focus=true
@kenjiskenjis
mentioned this pull request
Jun 22, 2022

Closed

7 tasks
@samsonasik
samsonasik
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Rector can be used to transform AllowDynamicProperties if needed
https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#addallowdynamicpropertiesattributerector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
I think #[AllowDynamicProperties] should be used as little as possible.
@paulbalandan
paulbalandan
commented
Jun 22, 2022

Copy link


Copy Markdown

MemberAuthor
Or maybe this is better: https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#completedynamicpropertiesrector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Yes, probably CompleteDynamicPropertiesRector can fix most of the errors.
MGatner
MGatner
approved these changes
Jun 22, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow we passed! Very nice. How about adding to the PHPStan Action as well so we can catch deprecations?
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Oh haha we didn't pass at all 🤦‍♂️ Result was ignored. Digging through these now...
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 24, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 24, 2022

Copy link


Copy Markdown

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

This was referenced Jun 27, 2022

Closed

 Merged

@kenjis
kenjis
commented
Jun 29, 2022

Copy link


Copy Markdown

Member
About 200 errors were reduced.
There were 660 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/7096495359?check_suite_focus=true
@MGatner
MGatner
commented
Jun 29, 2022

Copy link


Copy Markdown

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

Copy link


Copy Markdown

Member
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
paulbalandan
commented
Jun 30, 2022

Copy link


Copy Markdown

MemberAuthor
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
MGatner
commented
Jun 30, 2022

Copy link


Copy Markdown

Member
By our very own @paulbalandan! predis/predis#781
Thanks Paul; glad to see their quick response too.
@MGatner
MGatner
commented
Jul 8, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
But there are still 133 errors.
@MGatner
MGatner
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
paulbalandanforce-pushed
the
GHA-8.2
branch
2 times, most recently
from
c3fabd0 to
7976a07CompareOctober 22, 2022 15:52
@paulbalandan
paulbalandan
commented
Oct 22, 2022

Copy link


Copy Markdown

MemberAuthor
I used a temporary patch for Faker's troubled file. 7976a07
@kenjis
kenjis
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
@paulbalandan Applying patch on GA is not enough.

All users tests would still fail.

Why don't you send a PR to Faker?
@paulbalandan
paulbalandan
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
I mean all CI4 devs that use Faker.
@MGatner
MGatner
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 24, 2022

Copy link


Copy Markdown

Member
This is the test result of Shield. Most of the errors caused by Faker.
bash-3.2$ composer test
> phpunitPHPUnit 9.5.25 #StandWithUkraineRuntime: PHP 8.2.0-devConfiguration: /Users/kenji/work/codeigniter/official/codeigniter-shield/phpunit.xmlRandom 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 MBThere were 44 errors:
@kenjiskenjis
added
the 
stale
Pull requests with conflicts
label
Oct 25, 2022
@kenjis
kenjis
commented
Oct 25, 2022

Copy link


Copy Markdown

Member
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

@kenjiskenjis
mentioned this pull request
Oct 26, 2022
 Merged

2 tasks
@kenjiskenjis
removed
the 
stale
Pull requests with conflicts
label
Oct 26, 2022
@kenjis
kenjis
commented
Oct 26, 2022

Copy link


Copy Markdown

Member
I merged #6757

So all error should be gone.
@MGatnerMGatner
closed this
Oct 26, 2022
@MGatnerMGatner
reopened this
Oct 26, 2022
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
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
@codeigniter4codeigniter4
deleted a comment from martinemily3Oct 30, 2022
MGatner
MGatner
approved these changes
Oct 30, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! We did it!
@MGatner
MGatner
commented
Oct 30, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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
paulbalandan
merged commit 5e9d12f
into
codeigniter4:developNov 1, 2022
@paulbalandan
paulbalandan
deleted the

GHA-8.2

branch
November 1, 2022 08:54
@paulbalandan
paulbalandan
commented
Nov 1, 2022

Copy link


Copy Markdown

MemberAuthor
🚀
@kenjis
kenjis
commented
Nov 1, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Nov 1, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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 🤪
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kenjiskenjiskenjis approved these changes
@MGatnerMGatnerMGatner approved these changes

Assignees


No one assigned


Labels

github_actionsPull requests that update Github_actions code

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@paulbalandan@kenjis@samsonasik@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
Add PHP 8.2 to Unit Tests GHA by paulbalandan · Pull Request #6172 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
Add PHP 8.2 to Unit Tests GHA - #6172
Merged
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Nov 1, 2022
Merged
Add PHP 8.2 to Unit Tests GHA#6172
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Conversation
@paulbalandan
@paulbalandanpaulbalandan
commented
Jun 22, 2022•

edited by kenjis

Loading

Copy link


Copy Markdown

Member
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
@paulbalandanpaulbalandan
added
the 
github_actions
Pull requests that update Github_actions code
label
Jun 22, 2022
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
There are too many dynamic properties!
There were 889 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/6996511004?check_suite_focus=true
@kenjiskenjis
mentioned this pull request
Jun 22, 2022

Closed

7 tasks
@samsonasik
samsonasik
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Rector can be used to transform AllowDynamicProperties if needed
https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#addallowdynamicpropertiesattributerector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
I think #[AllowDynamicProperties] should be used as little as possible.
@paulbalandan
paulbalandan
commented
Jun 22, 2022

Copy link


Copy Markdown

MemberAuthor
Or maybe this is better: https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#completedynamicpropertiesrector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Yes, probably CompleteDynamicPropertiesRector can fix most of the errors.
MGatner
MGatner
approved these changes
Jun 22, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow we passed! Very nice. How about adding to the PHPStan Action as well so we can catch deprecations?
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Oh haha we didn't pass at all 🤦‍♂️ Result was ignored. Digging through these now...
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 24, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 24, 2022

Copy link


Copy Markdown

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

This was referenced Jun 27, 2022

Closed

 Merged

@kenjis
kenjis
commented
Jun 29, 2022

Copy link


Copy Markdown

Member
About 200 errors were reduced.
There were 660 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/7096495359?check_suite_focus=true
@MGatner
MGatner
commented
Jun 29, 2022

Copy link


Copy Markdown

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

Copy link


Copy Markdown

Member
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
paulbalandan
commented
Jun 30, 2022

Copy link


Copy Markdown

MemberAuthor
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
MGatner
commented
Jun 30, 2022

Copy link


Copy Markdown

Member
By our very own @paulbalandan! predis/predis#781
Thanks Paul; glad to see their quick response too.
@MGatner
MGatner
commented
Jul 8, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
But there are still 133 errors.
@MGatner
MGatner
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
paulbalandanforce-pushed
the
GHA-8.2
branch
2 times, most recently
from
c3fabd0 to
7976a07CompareOctober 22, 2022 15:52
@paulbalandan
paulbalandan
commented
Oct 22, 2022

Copy link


Copy Markdown

MemberAuthor
I used a temporary patch for Faker's troubled file. 7976a07
@kenjis
kenjis
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
@paulbalandan Applying patch on GA is not enough.

All users tests would still fail.

Why don't you send a PR to Faker?
@paulbalandan
paulbalandan
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
I mean all CI4 devs that use Faker.
@MGatner
MGatner
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 24, 2022

Copy link


Copy Markdown

Member
This is the test result of Shield. Most of the errors caused by Faker.
bash-3.2$ composer test
> phpunitPHPUnit 9.5.25 #StandWithUkraineRuntime: PHP 8.2.0-devConfiguration: /Users/kenji/work/codeigniter/official/codeigniter-shield/phpunit.xmlRandom 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 MBThere were 44 errors:
@kenjiskenjis
added
the 
stale
Pull requests with conflicts
label
Oct 25, 2022
@kenjis
kenjis
commented
Oct 25, 2022

Copy link


Copy Markdown

Member
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

@kenjiskenjis
mentioned this pull request
Oct 26, 2022
 Merged

2 tasks
@kenjiskenjis
removed
the 
stale
Pull requests with conflicts
label
Oct 26, 2022
@kenjis
kenjis
commented
Oct 26, 2022

Copy link


Copy Markdown

Member
I merged #6757

So all error should be gone.
@MGatnerMGatner
closed this
Oct 26, 2022
@MGatnerMGatner
reopened this
Oct 26, 2022
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
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
@codeigniter4codeigniter4
deleted a comment from martinemily3Oct 30, 2022
MGatner
MGatner
approved these changes
Oct 30, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! We did it!
@MGatner
MGatner
commented
Oct 30, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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
paulbalandan
merged commit 5e9d12f
into
codeigniter4:developNov 1, 2022
@paulbalandan
paulbalandan
deleted the

GHA-8.2

branch
November 1, 2022 08:54
@paulbalandan
paulbalandan
commented
Nov 1, 2022

Copy link


Copy Markdown

MemberAuthor
🚀
@kenjis
kenjis
commented
Nov 1, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Nov 1, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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 🤪
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kenjiskenjiskenjis approved these changes
@MGatnerMGatnerMGatner approved these changes

Assignees


No one assigned


Labels

github_actionsPull requests that update Github_actions code

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@paulbalandan@kenjis@samsonasik@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
Add PHP 8.2 to Unit Tests GHA by paulbalandan · Pull Request #6172 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
Add PHP 8.2 to Unit Tests GHA - #6172
Merged
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Nov 1, 2022
Merged
Add PHP 8.2 to Unit Tests GHA#6172
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Conversation
@paulbalandan
@paulbalandanpaulbalandan
commented
Jun 22, 2022•

edited by kenjis

Loading

Copy link


Copy Markdown

Member
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
@paulbalandanpaulbalandan
added
the 
github_actions
Pull requests that update Github_actions code
label
Jun 22, 2022
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
There are too many dynamic properties!
There were 889 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/6996511004?check_suite_focus=true
@kenjiskenjis
mentioned this pull request
Jun 22, 2022

Closed

7 tasks
@samsonasik
samsonasik
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Rector can be used to transform AllowDynamicProperties if needed
https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#addallowdynamicpropertiesattributerector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
I think #[AllowDynamicProperties] should be used as little as possible.
@paulbalandan
paulbalandan
commented
Jun 22, 2022

Copy link


Copy Markdown

MemberAuthor
Or maybe this is better: https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#completedynamicpropertiesrector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Yes, probably CompleteDynamicPropertiesRector can fix most of the errors.
MGatner
MGatner
approved these changes
Jun 22, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow we passed! Very nice. How about adding to the PHPStan Action as well so we can catch deprecations?
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Oh haha we didn't pass at all 🤦‍♂️ Result was ignored. Digging through these now...
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 24, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 24, 2022

Copy link


Copy Markdown

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

This was referenced Jun 27, 2022

Closed

 Merged

@kenjis
kenjis
commented
Jun 29, 2022

Copy link


Copy Markdown

Member
About 200 errors were reduced.
There were 660 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/7096495359?check_suite_focus=true
@MGatner
MGatner
commented
Jun 29, 2022

Copy link


Copy Markdown

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

Copy link


Copy Markdown

Member
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
paulbalandan
commented
Jun 30, 2022

Copy link


Copy Markdown

MemberAuthor
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
MGatner
commented
Jun 30, 2022

Copy link


Copy Markdown

Member
By our very own @paulbalandan! predis/predis#781
Thanks Paul; glad to see their quick response too.
@MGatner
MGatner
commented
Jul 8, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
But there are still 133 errors.
@MGatner
MGatner
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
paulbalandanforce-pushed
the
GHA-8.2
branch
2 times, most recently
from
c3fabd0 to
7976a07CompareOctober 22, 2022 15:52
@paulbalandan
paulbalandan
commented
Oct 22, 2022

Copy link


Copy Markdown

MemberAuthor
I used a temporary patch for Faker's troubled file. 7976a07
@kenjis
kenjis
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
@paulbalandan Applying patch on GA is not enough.

All users tests would still fail.

Why don't you send a PR to Faker?
@paulbalandan
paulbalandan
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
I mean all CI4 devs that use Faker.
@MGatner
MGatner
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 24, 2022

Copy link


Copy Markdown

Member
This is the test result of Shield. Most of the errors caused by Faker.
bash-3.2$ composer test
> phpunitPHPUnit 9.5.25 #StandWithUkraineRuntime: PHP 8.2.0-devConfiguration: /Users/kenji/work/codeigniter/official/codeigniter-shield/phpunit.xmlRandom 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 MBThere were 44 errors:
@kenjiskenjis
added
the 
stale
Pull requests with conflicts
label
Oct 25, 2022
@kenjis
kenjis
commented
Oct 25, 2022

Copy link


Copy Markdown

Member
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

@kenjiskenjis
mentioned this pull request
Oct 26, 2022
 Merged

2 tasks
@kenjiskenjis
removed
the 
stale
Pull requests with conflicts
label
Oct 26, 2022
@kenjis
kenjis
commented
Oct 26, 2022

Copy link


Copy Markdown

Member
I merged #6757

So all error should be gone.
@MGatnerMGatner
closed this
Oct 26, 2022
@MGatnerMGatner
reopened this
Oct 26, 2022
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
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
@codeigniter4codeigniter4
deleted a comment from martinemily3Oct 30, 2022
MGatner
MGatner
approved these changes
Oct 30, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! We did it!
@MGatner
MGatner
commented
Oct 30, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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
paulbalandan
merged commit 5e9d12f
into
codeigniter4:developNov 1, 2022
@paulbalandan
paulbalandan
deleted the

GHA-8.2

branch
November 1, 2022 08:54
@paulbalandan
paulbalandan
commented
Nov 1, 2022

Copy link


Copy Markdown

MemberAuthor
🚀
@kenjis
kenjis
commented
Nov 1, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Nov 1, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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 🤪
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kenjiskenjiskenjis approved these changes
@MGatnerMGatnerMGatner approved these changes

Assignees


No one assigned


Labels

github_actionsPull requests that update Github_actions code

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@paulbalandan@kenjis@samsonasik@MGatner

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
})();
Add PHP 8.2 to Unit Tests GHA by paulbalandan · Pull Request #6172 · codeigniter4/CodeIgniter4 · GitHub
Skip to content
Add PHP 8.2 to Unit Tests GHA - #6172
Merged
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Nov 1, 2022
Merged
Add PHP 8.2 to Unit Tests GHA#6172
paulbalandan merged 1 commit into
codeigniter4:developfrom 
paulbalandan:GHA-8.2
Conversation
@paulbalandan
@paulbalandanpaulbalandan
commented
Jun 22, 2022•

edited by kenjis

Loading

Copy link


Copy Markdown

Member
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
@paulbalandanpaulbalandan
added
the 
github_actions
Pull requests that update Github_actions code
label
Jun 22, 2022
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
There are too many dynamic properties!
There were 889 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/6996511004?check_suite_focus=true
@kenjiskenjis
mentioned this pull request
Jun 22, 2022

Closed

7 tasks
@samsonasik
samsonasik
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Rector can be used to transform AllowDynamicProperties if needed
https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#addallowdynamicpropertiesattributerector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
I think #[AllowDynamicProperties] should be used as little as possible.
@paulbalandan
paulbalandan
commented
Jun 22, 2022

Copy link


Copy Markdown

MemberAuthor
Or maybe this is better: https://github.com/rectorphp/rector/blob/main/docs/rector_rules_overview.md#completedynamicpropertiesrector
@kenjis
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Yes, probably CompleteDynamicPropertiesRector can fix most of the errors.
MGatner
MGatner
approved these changes
Jun 22, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow we passed! Very nice. How about adding to the PHPStan Action as well so we can catch deprecations?
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
Oh haha we didn't pass at all 🤦‍♂️ Result was ignored. Digging through these now...
@MGatner
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 22, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jun 24, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Jun 24, 2022

Copy link


Copy Markdown

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

This was referenced Jun 27, 2022

Closed

 Merged

@kenjis
kenjis
commented
Jun 29, 2022

Copy link


Copy Markdown

Member
About 200 errors were reduced.
There were 660 errors:
https://github.com/codeigniter4/CodeIgniter4/runs/7096495359?check_suite_focus=true
@MGatner
MGatner
commented
Jun 29, 2022

Copy link


Copy Markdown

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

Copy link


Copy Markdown

Member
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
paulbalandan
commented
Jun 30, 2022

Copy link


Copy Markdown

MemberAuthor
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
MGatner
commented
Jun 30, 2022

Copy link


Copy Markdown

Member
By our very own @paulbalandan! predis/predis#781
Thanks Paul; glad to see their quick response too.
@MGatner
MGatner
commented
Jul 8, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
But there are still 133 errors.
@MGatner
MGatner
commented
Jul 9, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
paulbalandanforce-pushed
the
GHA-8.2
branch
2 times, most recently
from
c3fabd0 to
7976a07CompareOctober 22, 2022 15:52
@paulbalandan
paulbalandan
commented
Oct 22, 2022

Copy link


Copy Markdown

MemberAuthor
I used a temporary patch for Faker's troubled file. 7976a07
@kenjis
kenjis
commented
Oct 22, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
@paulbalandan Applying patch on GA is not enough.

All users tests would still fail.

Why don't you send a PR to Faker?
@paulbalandan
paulbalandan
commented
Oct 23, 2022

Copy link


Copy Markdown

MemberAuthor
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
kenjis
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
I mean all CI4 devs that use Faker.
@MGatner
MGatner
commented
Oct 23, 2022

Copy link


Copy Markdown

Member
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
kenjis
commented
Oct 24, 2022

Copy link


Copy Markdown

Member
This is the test result of Shield. Most of the errors caused by Faker.
bash-3.2$ composer test
> phpunitPHPUnit 9.5.25 #StandWithUkraineRuntime: PHP 8.2.0-devConfiguration: /Users/kenji/work/codeigniter/official/codeigniter-shield/phpunit.xmlRandom 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 MBThere were 44 errors:
@kenjiskenjis
added
the 
stale
Pull requests with conflicts
label
Oct 25, 2022
@kenjis
kenjis
commented
Oct 25, 2022

Copy link


Copy Markdown

Member
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

@kenjiskenjis
mentioned this pull request
Oct 26, 2022
 Merged

2 tasks
@kenjiskenjis
removed
the 
stale
Pull requests with conflicts
label
Oct 26, 2022
@kenjis
kenjis
commented
Oct 26, 2022

Copy link


Copy Markdown

Member
I merged #6757

So all error should be gone.
@MGatnerMGatner
closed this
Oct 26, 2022
@MGatnerMGatner
reopened this
Oct 26, 2022
@MGatner
MGatner
commented
Oct 27, 2022

Copy link


Copy Markdown

Member
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
@codeigniter4codeigniter4
deleted a comment from martinemily3Oct 30, 2022
MGatner
MGatner
approved these changes
Oct 30, 2022
View reviewed changes
@MGatnerMGatner
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! We did it!
@MGatner
MGatner
commented
Oct 30, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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
paulbalandan
merged commit 5e9d12f
into
codeigniter4:developNov 1, 2022
@paulbalandan
paulbalandan
deleted the

GHA-8.2

branch
November 1, 2022 08:54
@paulbalandan
paulbalandan
commented
Nov 1, 2022

Copy link


Copy Markdown

MemberAuthor
🚀
@kenjis
kenjis
commented
Nov 1, 2022

Copy link


Copy Markdown

Member
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
MGatner
commented
Nov 1, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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 🤪
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@kenjiskenjiskenjis approved these changes
@MGatnerMGatnerMGatner approved these changes

Assignees


No one assigned


Labels

github_actionsPull requests that update Github_actions code

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

4 participants

@paulbalandan@kenjis@samsonasik@MGatner