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.1 support #31

Closed
bartvanhoutte opened this issue Oct 15, 2021 · 1 comment · Fixed by #32
Closed

PHP 8.1 support #31

bartvanhoutte opened this issue Oct 15, 2021 · 1 comment · Fixed by #32

Comments

@bartvanhoutte
Copy link
Contributor

As the release of PHP 8.1 is just over a month away, I started testing my code on PHP 8.1-RC4.

In PHP 8.1, return types were added on a lot of functions, one of them being \Countable::count(...). Since the current version of this library does not have a return type on \Clue\React\Mq\Queue::count(...) - and I'm guessing won't any time soon for backwards compatibility - PHP throws a deprecation warning. When PHPUnit or an other testing library is configured to treat deprecations as exceptions, tests fail. From PHP 9 on, this will result in a fatal error.

Currently, the RFC suggests using the attribute #[ReturnTypeWillChange] which is backwards compatible with the versions currently supported by this library. I guess an alternative would be to declare an alternative function signature for newer versions of PHP.

What do you think?

👋

@clue
Copy link
Owner

clue commented Oct 15, 2021

@bartvanhoutte Thanks for bringing this up, good one!

I think adding the #[ReturnTypeWillChange] attribute sounds perfectly reasonable for the current major version. Do you feel like filing a PR along with the updated test suite?

Looking forward, we will release a major version some time in the future which will likely drop support for legacy PHP versions, so we can finally start using proper return types. In this case, we will no longer need the attribute and can instead rely on a count(): int definition. I'm not opposed to rethinking the current API, but I think at this point not much would be gained by coming up with a different API.

Let me know what you think about this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants