GitHub actions phpunit on php 8.2 & Fix bug on php 8.2 #275

hungtrinh · 2022-11-14T19:56:24Z

Follow up PR #240, PR #269

This PR focus on running PHPUnit 9 on PHP 8.2 without fail test case, reduce a deprecated message when run on PHP 8.2.

To save time, i used Classes with #[AllowDynamicProperties] attribute to resolve issue Deprecated: Creation of dynamic property {x} is deprecated on PHP 8.2

Work done

Add github action run PHPUnit 9 on PHP 8.2
Fixed bug on php 8.2: Deprecated: Creation of dynamic property {x} is deprecated
resolved php 8.2 - Zend_Console_GetoptTest::testUsingDashWithoutOptionNotAsLastArgumentThrowsException bug on php 8.2 #276

How to show all deprecated message on php 8.2 before apply this PR

cd zf1-future

echo "Checkout to commit before merge this PR"
git checkout b1d3fcc1cc4825b04f3da594b80f178ef8f47f95

echo "Install phpunit on your pc"
composer i

echo "Use docker to run PHPUnit 9 on PHP 8.2"
docker run -e CI=true -v $(pwd):$(pwd) -w $(pwd) -t --rm php:8.2-rc-cli-alpine -d memory_limit=-1 ./bin/phpunit -v --bootstrap tests/TestHelper.php tests/Zend/AllTests.php

Result? You will see something like that :)

Known issue

Run PHPUnit on php 8.2 show this message. PHP 8.2: Partially-supported callable are deprecated

Deprecated: Callables of the form ["Zend_EventManager_EventManagerTest", "Zend_EventManager_EventManagerTest::testAttachShouldReturnCallbackHandler"] are deprecated in /github/workspace/library/Zend/Stdlib/CallbackHandler.php on line 97
.
Deprecated: Callables of the form ["Zend_EventManager_EventManagerTest", "Zend_EventManager_EventManagerTest::testAttachShouldAddListenerToEvent"] are deprecated in /github/workspace/library/Zend/Stdlib/CallbackHandler.php on line 97
.
Deprecated: Callables of the form ["Zend_EventManager_EventManagerTest", "Zend_EventManager_EventManagerTest::testAttachShouldAddEventIfItDoesNotExist"] are deprecated in /github/workspace/library/Zend/Stdlib/CallbackHandler.php on line 97
...
Deprecated: Callables of the form ["Zend_EventManager_EventManagerTest", "Zend_EventManager_EventManagerTest::testDetachShouldRemoveListenerFromEvent"] are deprecated in /github/workspace/library/Zend/Stdlib/CallbackHandler.php on line 97
.
Deprecated: Callables of the form ["Zend_EventManager_EventManagerTest", "Zend_EventManager_EventManagerTest::testDetachShouldReturnFalseIfEventDoesNotExist"] are deprecated in /github/workspace/library/Zend/Stdlib/CallbackHandler.php on line 97
.
Deprecated: Callables of the form ["Zend_EventManager_EventManagerTest", "Zend_EventManager_EventManagerTest::testDetachShouldReturnFalseIfListenerDoesNotExist"] are deprecated in /github/workspace/library/Zend/Stdlib/CallbackHandler.php on line 97
...........................S.......  3658 / 10[79](https://github.com/Shardj/zf1-future/actions/runs/3467598245/jobs/5792619404#step:5:80)1 ( 33%)

Deprecated: Callables of the form ["Zend_EventManager_FilterChainTest", "Zend_EventManager_FilterChainTest::testSubscribeShouldReturnCallbackHandler"] are deprecated in /github/workspace/library/Zend/Stdlib/CallbackHandler.php on line 97
.
Deprecated: Callables of the form ["Zend_EventManager_FilterChainTest", "Zend_EventManager_FilterChainTest::testSubscribeShouldAddCallbackHandlerToFilters"] are deprecated in /github/workspace/library/Zend/Stdlib/CallbackHandler.php on line 97
.
Deprecated: Callables of the form ["Zend_EventManager_FilterChainTest", "Zend_EventManager_FilterChainTest::testDetachShouldRemoveCallbackHandlerFromFilters"] are deprecated in /github/workspace/library/Zend/Stdlib/CallbackHandler.php on line 97
.
Deprecated: Callables of the form ["Zend_EventManager_FilterChainTest", "Zend_EventManager_FilterChainTest::testDetachShouldReturnFalseIfCallbackHandlerDoesNotExist"] are deprecated in /github/workspace/library/Zend/Stdlib/CallbackHandler.php on line 97

.github/workflows/phpunit.yml

library/Zend/Amf/Parse/Serializer.php

library/Zend/Console/Getopt.php

glensc · 2022-11-15T14:15:55Z

Can Classes with #[AllowDynamicProperties] attribute be moved to separate PR? It's easier to review if the other changes don't span 232 files.

hungtrinh · 2022-11-15T14:44:40Z

Can Classes with #[AllowDynamicProperties] attribute be moved to separate PR? It's easier to review if the other changes don't span 232 files.

it's will consume a lot of time.
To speed up apply PHP 8.2 into zf1-future, i skipped those steps

Log separate issue XXX Deprecated: Creation of dynamic property {x} is deprecated for sub package tests/Zend/SubPackage & library/Zend/SubPackage
Make fixing commit for XXX

glensc · 2022-11-15T17:42:05Z

it's will consume a lot of time.

if it was separate commit in first place, it's just cherry pick on a new branch or rebase on new branch to remove other commits

https://www.freshconsulting.com/insights/blog/atomic-commits/

glensc · 2022-11-15T17:42:50Z

To speed up apply PHP 8.2 into zf1-future, i skipped those steps

yet you're slowing down review because the changeset in single pull request is so big to review

glensc · 2022-11-15T18:08:35Z

why have you modified this file in this PR:

tests/Zend/Paginator/_files/test.sqlite

what's in it anyway? perhaps it should be reverted to value in 59ede18?
because all changes to it are yours, but totally bogus commit messages. or beter be added to .gitignore?

glensc · 2022-11-15T19:16:00Z

extracted and merged some changes that were ready:

more issues:

hidden docker image change in dc049a3 commit -> moved to e6be069 commit
wget options change, hidden into 2822b92 commit -> moved to e6be069 commit
bunch of trailing whitespaces. removed in

not fixed:

hidden changes in tests:
- $locales[$tmp[0]] = count($tmp) > 1 ? $tmp[1] : $tmp[0];
- "${substitute} 123!";

move them to separate commit so explanation is present

glensc · 2022-11-15T19:51:42Z

Extracted properties changes:

PHP 8.2: Add missing properties to classes #281

and rebased this PR after its merge.

you should rewrite commit message, I just put "PHP 8.2 fixes in tests" placeholder for now

…se {$var} instead Failling test case "Zend_Reflection_FileTest::testFileGetFunctionsReturnsFunctions"

…etlocale(LC_ALL, 0) return unexpected string format. Expected string format like that: setlocale(LC_ALL, 0) === 'LC_CTYPE=en_US.UTF-8;LC_NUMERIC=C;...' Actual string format on apline container: setlocale(LC_ALL, 0) === 'C.UTF-8;C;C;C;C;C' missing part '=Something'

glensc · 2022-11-16T08:22:40Z

@hungtrinh can you answer about the sqlite file:

GitHub actions phpunit on php 8.2 & Fix bug on php 8.2 #275 (comment)

Seems I forgot to take it out, so it got merged via 65c1385. But should be resolved in future PR then.

hungtrinh · 2022-11-16T08:24:15Z

Mr @glensc, Thanks you for fixing my mess commit history.

I separated rest commit here:

hungtrinh · 2022-11-16T08:50:59Z

@hungtrinh can you answer about the sqlite file:

GitHub actions phpunit on php 8.2 & Fix bug on php 8.2 #275 (comment)

why have you modified this file in this PR:

tests/Zend/Paginator/_files/test.sqlite

what's in it anyway? perhaps it should be reverted to value in 59ede18? because all changes to it are yours, but totally bogus commit messages. or beter be added to .gitignore?

Mr @glensc
tests/Zend/Paginator/_files/test.sqlite is fixture file use by Zend test suite from beginning, so i don't think we can ignore it

And here it's root cause, Every time this test case run will change content of tests/Zend/Paginator/_files/test.sqlite
https://github.com/Shardj/zf1-future/blob/master/tests/Zend/Paginator/Adapter/DbSelectTest.php#L304-L369

Reproceduce bug step:
docker run -v $(pwd):$(pwd) -w $(pwd) -t --rm php:8.2-rc-cli-alpine -d memory_limit=-1 ./bin/phpunit -v --bootstrap tests/TestHelper.php tests/Zend/Paginator/Adapter/DbSelectTest.php

How to fix it, here is my idea:

add tests/Zend/Paginator/_files/test-fixture.sqlite into .gitignore
in setUp() cp tests/Zend/Paginator/_files/test.sqlite tests/Zend/Paginator/_files/test-fixture.sqlite
in tearDown() rm -rf tests/Zend/Paginator/_files/test-fixture.sqlite

If you think it's ok i will log issue and fix it

glensc · 2022-11-16T11:23:03Z

Yes. create issue or start PR, can discuss details here. I'm thinking maybe it can be generated from scratch.

hungtrinh · 2022-11-16T16:24:11Z

Yes. create issue or start PR, can discuss details here. I'm thinking maybe it can be generated from scratch.

Mr @glensc please review PR #284 - to close Issue ##283

glensc reviewed Nov 14, 2022

View reviewed changes

.github/workflows/phpunit.yml Outdated Show resolved Hide resolved

glensc reviewed Nov 14, 2022

View reviewed changes

.github/workflows/phpunit.yml Outdated Show resolved Hide resolved

glensc reviewed Nov 14, 2022

View reviewed changes

library/Zend/Amf/Parse/Serializer.php Outdated Show resolved Hide resolved

hungtrinh marked this pull request as ready for review November 15, 2022 07:27

glensc reviewed Nov 15, 2022

View reviewed changes

library/Zend/Console/Getopt.php Outdated Show resolved Hide resolved

hungtrinh force-pushed the github-actions-phpunit-on-php82 branch from 0987574 to 168a6df Compare November 15, 2022 10:08

This was referenced Nov 15, 2022

Fix str_split('') logic to keep same as PHP before 8.2 on PHP 8.2 #277

Merged

php 8.2 - Zend_Console_GetoptTest::testUsingDashWithoutOptionNotAsLastArgumentThrowsException bug on php 8.2 #276

Closed

This was referenced Nov 15, 2022

Add AllowDynamicProperties Attribute to classes #278

Merged

Fix typo attribute '_mbStringFunctionsOverloaded' typo #279

Merged

CI: Remove fetch-depth: 0 #280

Merged

glensc force-pushed the github-actions-phpunit-on-php82 branch from 168a6df to f8122d9 Compare November 15, 2022 19:15

glensc force-pushed the github-actions-phpunit-on-php82 branch from f8122d9 to 0c4602c Compare November 15, 2022 19:17

This was referenced Nov 15, 2022

Add AllowDynamicProperties Attribute to classes zf1s/zf1#145

Closed

Fix str_split('') logic to keep same as PHP before 8.2 on PHP 8.2 zf1s/zf1#143

Merged

PHP 8.2: Add missing properties to classes #281

Merged

glensc force-pushed the github-actions-phpunit-on-php82 branch from 0c4602c to 9676af1 Compare November 15, 2022 19:51

hungtrinh added 3 commits November 16, 2022 14:41

Add phpunit-php82 job

ac3a3a5

PHP 8.2 fix bug. Deprecated: Using ${var} in strings is deprecated, u…

9d2ed51

…se {$var} instead Failling test case "Zend_Reflection_FileTest::testFileGetFunctionsReturnsFunctions"

hungtrinh force-pushed the github-actions-phpunit-on-php82 branch from 9676af1 to a919093 Compare November 16, 2022 08:01

glensc approved these changes Nov 16, 2022

View reviewed changes

glensc merged commit bc1be9c into Shardj:master Nov 16, 2022

This was referenced Nov 17, 2022

Test fixture file tests/Zend/Paginator/_files/test.sqlite modified everytime run unittest #283

Closed

PHP 8.2 - Fix deprecated callable patterns in unit test #285

Merged

Complete dynamic properties for tests/Zend/** #289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub actions phpunit on php 8.2 & Fix bug on php 8.2 #275

GitHub actions phpunit on php 8.2 & Fix bug on php 8.2 #275

hungtrinh commented Nov 14, 2022 •

edited

Loading

glensc commented Nov 15, 2022

hungtrinh commented Nov 15, 2022

glensc commented Nov 15, 2022

glensc commented Nov 15, 2022

glensc commented Nov 15, 2022 •

edited

Loading

glensc commented Nov 15, 2022 •

edited

Loading

glensc commented Nov 15, 2022 •

edited

Loading

glensc commented Nov 16, 2022 •

edited

Loading

hungtrinh commented Nov 16, 2022

hungtrinh commented Nov 16, 2022

glensc commented Nov 16, 2022

hungtrinh commented Nov 16, 2022 •

edited

Loading

GitHub actions phpunit on php 8.2 & Fix bug on php 8.2 #275

GitHub actions phpunit on php 8.2 & Fix bug on php 8.2 #275

Conversation

hungtrinh commented Nov 14, 2022 • edited Loading

Work done

How to show all deprecated message on php 8.2 before apply this PR

Known issue

glensc commented Nov 15, 2022

hungtrinh commented Nov 15, 2022

glensc commented Nov 15, 2022

glensc commented Nov 15, 2022

glensc commented Nov 15, 2022 • edited Loading

glensc commented Nov 15, 2022 • edited Loading

glensc commented Nov 15, 2022 • edited Loading

glensc commented Nov 16, 2022 • edited Loading

hungtrinh commented Nov 16, 2022

hungtrinh commented Nov 16, 2022

glensc commented Nov 16, 2022

hungtrinh commented Nov 16, 2022 • edited Loading

hungtrinh commented Nov 14, 2022 •

edited

Loading

glensc commented Nov 15, 2022 •

edited

Loading

glensc commented Nov 15, 2022 •

edited

Loading

glensc commented Nov 15, 2022 •

edited

Loading

glensc commented Nov 16, 2022 •

edited

Loading

hungtrinh commented Nov 16, 2022 •

edited

Loading