Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PHPUnit 9 run on php 8.1 to Github actions, Fix some php 8.1 depricated issue #269

Merged
merged 5 commits into from
Nov 7, 2022

Conversation

hungtrinh
Copy link

@hungtrinh hungtrinh commented Nov 3, 2022

Work Done

As mentioned in section What next of PR 240

  • Add PHPUnit 9 run on PHP 8.1 to Github
  • Fixed PHP 8.1 Deprecated warning:
    • current(): Calling current() on an object is deprecated
    • Passing null to parameter #1 () of type string is deprecated

Unresolved problem

On php 8.1:

  • Function date_sunset() is deprecated
  • Function date_sunrise() is deprecated
❯  docker run -e CI=true --rm -v $(pwd):/app jakzal/phpqa:php8.1-alpine phpunit --bootstrap /app/tests/TestHelper.php /app/tests/Zend/AllTests.php

PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

....................S....S.......

Time: 00:30.474, Memory: 207.38 MB

There were 2 errors:

1) Zend_DateTest::testSunFunc
Function date_sunset() is deprecated

/app/library/Zend/Date/DateObject.php:929
/app/library/Zend/Date.php:3160
/app/tests/Zend/DateTest.php:3828
/app/tests/Zend/AllTests.php:139
/app/tests/Zend/AllTests.php:263

2) Zend_Date_DateObjectTest::testCalcSunInternal
Function date_sunrise() is deprecated

/app/library/Zend/Date/DateObject.php:932
/app/tests/Zend/Date/DateObjectTest.php:658
/app/tests/Zend/Date/DateObjectTest.php:280
/app/tests/Zend/AllTests.php:139
/app/tests/Zend/AllTests.php:263

ERRORS!
Tests: 9275, Assertions: 39641, Errors: 2, Skipped: 449, Incomplete: 13.

@PHPGangsta Could you please resolve this issue? I guess you have a lot of experience with this problem
I try apply this solution into Zend/Date/DateObject.php but test case DateObjectTest::testCalcSunInternal will failed

Fix deprecation message like that 'Passing null to parameter Shardj#1 () of type string is deprecated'
@Jimbolino
Copy link
Collaborator

i've done some digging, and the original test code was copied from @jaydiablo
diablomedia/zf1-date@f69acaf

also the calcSun function is wrongly documented:

     * @param  array  $location  Location for calculation MUST include 'latitude', 'longitude', 'horizon'
     * @param  bool   $horizon   true: sunrise; false: sunset
     * @return mixed  - false: midnight sun, integer:

So if people put true/false there, the $zenith will be 90+true or 90+false

The main difference between date_sunset / date_sun_info is the $zenith / $horizon parameter.
By switching to date_sun_info we lose the support for a custom zenith angle, which i think is fine.

I'm not sure why the zf1 devs used 90-0.0145439 = 89,9854561 as the zenith for their tests.

The default should be float 90.833 aka 90°50'
https://3v4l.org/b9LFD

I dont believe there are people actually depending on this code, for exact sunset/sunrise times, so worst case there is a 1-5 min difference between versions.

on php 8.1 use date_sun_info() instead of date_sunset(), date_sunrise() other case backwards compatible with php < 8.1
@hungtrinh
Copy link
Author

hungtrinh commented Nov 7, 2022

@Jimbolino thanks for your help, in this situation to keep backward compatibility with php < 8.1 i added new case for php >= 8.1, keep old code for php < 8.1

Resolved problem

On php 8.1:

@Jimbolino
Copy link
Collaborator

you accidentally added a .sqlite file :)

@hungtrinh
Copy link
Author

you accidentally added a .sqlite file :)

Thanks ^^ @Jimbolino don't worry about it. Everytime i run full test on my pc this fixture sqlite file changed but without any ill effect.

@Jimbolino Jimbolino merged commit 673290a into Shardj:master Nov 7, 2022
@hungtrinh hungtrinh deleted the ci-phpunit-on-php81 branch March 7, 2023 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants