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

PHP 8 Support #48

Merged
merged 12 commits into from
Aug 29, 2022
Merged

PHP 8 Support #48

merged 12 commits into from
Aug 29, 2022

Conversation

j03k64
Copy link
Collaborator

@j03k64 j03k64 commented Feb 22, 2021

PHP 8 Support

This PR adds support for getting this library working on PHP 8. I'm going to leave this open for a bit while I determine if there are other places that need to be addressed as well.

Issue 1

get_resource_type fails with $this->socket passed in on PHP8 since it's expecting a resource instead of a Socket object.

Fix 1

Update Net/Gearman/Connection.php to return early if $this->socket is an instance of Socket (which should be only on PHP8+), otherwise falls back to default logic.

Issue 2

Unable to run functional tests.

Fix 2

  • Updated ConnectionTest and TaskTest to use more modern code including short array syntax and the assertArray method introduced in phpunit 7.5. This required a bump of the minimum phpunit to 7.5 from 7.3.
  • Updated ConnectionTest to check for PHP_VERSION >= 8.0 and if so check for the Socket object instead of socket string.
  • Added a tests/README.md for how to run the unit tests.

@j03k64 j03k64 self-assigned this Feb 22, 2021
@j03k64 j03k64 marked this pull request as draft February 22, 2021 17:45
@j03k64
Copy link
Collaborator Author

j03k64 commented Feb 22, 2021

@brianlmoon thoughts on how to update the unit test? Do a PHP version check like below:

// Net_Gearman_ConnectionTest.php:19
if (version_compare(PHP_VERSION, '8.0.0') >= 0) {
  // PHP 8+ returns a Socket class instead of a resource now
  $this->assertInstanceOf('Socket', $connection->socket);
}
else {
  $this->assertEquals('socket', strtolower(get_resource_type($connection->socket)));
}

or would you prefer something else?

@brianlmoon
Copy link
Owner

@j03k64 Makes sense to me to do it that way.

@j03k64
Copy link
Collaborator Author

j03k64 commented Feb 23, 2021

@brianlmoon I don't have an easy way to test this on a PHP 7.X install. I could figure it out, but if you have something readily available that would be appreciated.

@j03k64 j03k64 requested a review from brianlmoon February 24, 2021 18:07
@j03k64 j03k64 marked this pull request as ready for review February 24, 2021 18:07
@j03k64
Copy link
Collaborator Author

j03k64 commented Mar 8, 2021

@brianlmoon any other comments? (I have this branch in production running on 7.4 fwiw)

@brianlmoon brianlmoon merged commit 0f61d29 into master Aug 29, 2022
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