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

feat: support for PHP 8.1 #62

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TorstenDittmann
Copy link

@TorstenDittmann TorstenDittmann commented Mar 17, 2022

I used main as the base branch, since it appeared to be more up2date. If that was a mistake I'll gladly fix that and work from develop.

Opened this PR as draft - since I wasn't able to test it completely on my machine and would like to rely on the CI at this stage. Especially since I cannot test multiple PHP versions that easily.

Depends on #56

Changes

  • bumps phpunit/phpunit to ^8.5
  • replaces deprecated strftime($format) with date($format, time())
  • adapts test classes that extend the PHPUnit Testcase to new version
    • using PHPUnit\Framework\TestCase over PHPUnit_Framework_TestCase
    • adds parameters to some test classes*
    • replaces PHPDoc comments for expecting exceptions with $this->expectException(Exception::class)
    • replaces $this->assertInternalType('array', $array) with $this->assertIsArray($array)

* According to PHP 8.0 backward compatible changes => call_user_func_array() array keys will now be interpreted as parameter names, instead of being silently ignored.

@pprkut
Copy link
Collaborator

pprkut commented Mar 17, 2022

Your requirements could not be resolved to an installable set of packages.

php-resque still supports PHP 5.6, and the newer phpunit versions all require a newer one.

My idea was to get the namespace changes in first (#56), since that is something that is still supported by 5.6, release that and do breaking changes for PHP 7/8 support after. Not sure what @danhunsaker 's plans are though :)

@TorstenDittmann
Copy link
Author

Your requirements could not be resolved to an installable set of packages.

php-resque still supports PHP 5.6, and the newer phpunit versions all require a newer one.

My idea was to get the namespace changes in first (#56), since that is something that is still supported by 5.6, release that and do breaking changes for PHP 7/8 support after. Not sure what @danhunsaker 's plans are though :)

Yeah, that makes sense 👍🏻

In my opinion 5.x shouldn't be a priority at this point anyway, since it is a blocker for teams/people using this library to adapt more recent versions of PHP.

I'll be more than happy to help out for this transition wherever possible 🙂

@danhunsaker
Copy link
Member

I used main as the base branch, since it appeared to be more up2date.

Yeah, I need to fix the branches. PR target should always be develop, but let me update it before you switch targets.

Not sure what @danhunsaker 's plans are though :)

Exactly that, actually! Just gotta finish the work involved.

@TorstenDittmann
Copy link
Author

Exactly that, actually! Just gotta finish the work involved.

Awesome, feel free to reach out if you need a set of helping hands 🙂

@pprkut
Copy link
Collaborator

pprkut commented Mar 17, 2022

Awesome, feel free to reach out if you need a set of helping hands slightly_smiling_face

The pull request for that is actually pretty complete already, from a functional perspective. A minor code refactor and documentation update is all that's missing and then it should be ready. I plan to tackle that this weekend.

What would be really appreciated though is testing. I updated all the existing unit tests and they pass, but "existing" is the watchword :)
Not all functionality is covered by unit tests yet and unit tests themselves aren't perfect either. So while they pass, it might very well be that some odd thing here and there fell through the cracks and testing the code will expose that. If you want, you could take a stab at that, though it's probably a bit easier if you wait until I made the last small refactor.

@TorstenDittmann
Copy link
Author

TorstenDittmann commented Mar 18, 2022

@danhunsaker what if I create a partial PR only replacing the deprecated strftime($format) with date($format, time()) - which should in theory work for all supported versions including 5.6. We could already have compatibility for PHP 8.1 with the next version. Obviously not officially because of missing tests.

Depending on the ETA for the namespace changes - our team could already move to PHP 8.1 👍🏻

@danhunsaker
Copy link
Member

Yeah, that seems straightforward enough.

@thedotedge
Copy link

hey guys, very welcome changes. Any ETA for a new tag? 😄

@JoyceBabu
Copy link
Contributor

@danhunsaker what if I create a partial PR only replacing the deprecated strftime($format) with date($format, time()) - which should in theory work for all supported versions including 5.6. We could already have compatibility for PHP 8.1 with the next version. Obviously not officially because of missing tests.

Depending on the ETA for the namespace changes - our team could already move to PHP 8.1 👍🏻

I had created a PR doing exactly that. I closed it on seeing this PR. Shall I re-open it?

@pprkut
Copy link
Collaborator

pprkut commented Aug 25, 2022

@JoyceBabu Yes, I think that makes sense. I'm not sure of what's planned for everything that's in this PR, but that change alone should be fairly easy to merge, so a separate PR should be fine 🙂

@JoyceBabu
Copy link
Contributor

I have reopened the PR.
#66

@Avinash-Ravula
Copy link

Can anyone provide me an ETA on when this PR gonna be merged?

@danhunsaker
Copy link
Member

@TorstenDittmann Ok, go ahead and rebase against develop at your earliest convenience.

@fersk
Copy link

fersk commented Aug 26, 2023

It's great to see activity here. This looks good and would be great to have it released. Puts it closer to full support to 8.x.

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.

7 participants