fixing php 8.1 deprecation warnings #9370
Conversation
php 8.1 is more strict, and raises some deprecation notices with existing protobuf code. Not all of the deprecations can be fixed without dropping support for php7.x (eg, the 'mixed' type doesn't appear until 8.1, and union types until 8.0, but as an interim solution the 'ReturnTypeWillChange' attribute can be used to suppress the notices. In passing, also be explicit about a cast from float to int in 'zigZagEncode64' which 8.1 also complains about when running tests.
|
have signed CLA now. I can't add labels, but I think |
|
Looks good. |
|
@zeriyoshi - part one is done (adding return type hints), now I just need to work out how to add annotations |
bcmath (specifically, the bccomp function) is internally required, and tests fail if it's not available
|
@brettmc However, it seems that the minimum supported version of PHP has been changed to 7.1 or higher. Perhaps You can lower the minimum required version of PHP, is that possible? |
|
For what it's worth, PHP 7.0 seems to be used on less than 1% of installs these days, anyway, so an upgrade shouldn't be too risky: https://stitcher.io/blog/php-version-stats-january-2022 Given that protobuf is a but esoterical in the PHP community, I'd suspect that most users of this library are actually using even more recent versions of PHP in their tech stack, anyway. |
|
That's right. but, Google still continues to support PHP 5.5 in GAE. https://cloud.google.com/appengine/docs/standard/php The owner has the right to decide the lifetime of the software. It is up to Google to decide this. In either case, I think it is essential to include it in the release notes. Personally, I think that the PHP Library and Extension should drop support for PHP 7.0. |
|
The reason for php 7.1 is because that's where the Anyway, still work-in-progress, so I'll convert this to a draft while I continue to find a fix for the extension. |
With guidance from Remi Collet, use ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_TYPE_EX macro, and use a conditional to fake that macro for earlier php versions. Tested on 8.1 and 7.4, and deprecation notices gone plus all tests pass
The macro ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX changed in 7.2, so it cannot be used in a compatible way with earlier versions
|
OK all, thanks for your patience. I think I have this working now, with help from the php-internals list. I've run the tests against 8.1, 7.4 and 7.2 with/without the extension, and all is green. I couldn't get the extension to be compatible with 7.1, so now the supported versions are: How do we seek approval to drop 7.0/7.1 support? (and, could somebody please add |
|
Perhaps @haberman knows about it? |
|
Thanks all for your work on this PR. When considering what versions of PHP to support, we have to coordinate with other projects, particularly gRPC and Google Cloud APIs. gRPC appears to still be supporting 7.0+ still: https://grpc.io/docs/languages/php/quickstart/ If we were going to drop 7.0 we would need to coordinate with gRPC. |
|
Asked on https://groups.google.com/g/grpc-io about my proposed changes. |
|
@haberman - the (hopefully) last issue with 32bit tests is that I added |
|
FYI, I think there's a list of "recommended" instead of "required" extensions; maybe that would be appropriate? |
I think it _should_ be required, but a test (linux, 32bit, 7.0-zts) is choking on composer install, so putting things back to how I found them
|
@haberman - update on the ext-bcmath dependency. The 32bit |
| @@ -90,6 +90,34 @@ RUN wget -O phpunit https://phar.phpunit.de/phpunit-9.phar \ | |||
| && cp phpunit /usr/local/php-8.0/bin \ | |||
| && mv phpunit /usr/local/php-8.0-zts/bin | |||
|
|
|||
| # php 8.1 | |||
There was a problem hiding this comment.
I wasn't sure whether the idea was to have one dockerfile per php version moving forward, or an all-in-one like is done for php7.x
| @@ -138,6 +140,7 @@ public function offsetGet($offset) | |||
| * @throws \ErrorException Non-existing index. | |||
| * @throws \ErrorException Incorrect type of the element. | |||
| */ | |||
| #[\ReturnTypeWillChange] | |||
| public function offsetSet($offset, $value) | |||
There was a problem hiding this comment.
@Tony-Sol - I did have have void return types originally, but removed them to retain BC to php 7.0 (some discussion above on the subject). The void return type only arrived in 7.1, and the tests were failing (from memory, searching for and not finding a void class in the current namespace)
|
@brettmc I've updated the new Docker images based on your branch and kicked off the tests. We'll see what happens. Thanks for all your work on this PR! |
|
@haberman I see all green for the checks, but I can't find the kokoro job that ran 8.x tests to confirm that all is well (edit): I think I'm expecting to see a |
|
Hmm, it looks like only some of the presubmits ran. Let me try kicking it again. |
|
All tests have passed, but it looks like 8.1 tests did not run. I am realizing that your edits to https://github.com/protocolbuffers/protobuf/blob/master/kokoro/linux/php_all/build.sh is using using Docker images that are not in this repository. I'm going to merge this PR as-is, since it's clearly not breaking any of our existing use cases. Then I'll add an 8.1 Docker image in the other (private) repo and send another PR to test it. If I run into any problems I'll let you know. |
|
Thanks for merging this, @haberman! Are there any plans to tag a patch release that includes this fix? |
php 8.1 is more strict, and raises some deprecation notices with existing protobuf code. Not all of the
deprecations can be fixed without dropping support for php7.x (eg, the 'mixed' type doesn't appear until
8.1, and union types until 8.0, but as an interim solution the 'ReturnTypeWillChange' attribute can be
used to suppress the notices. In passing, also be explicit about a cast from float to int in 'zigZagEncode64'
which 8.1 also complains about when running tests.