fixing php 8.1 deprecation warnings #9370

brettmc · 2022-01-05T05:22:44Z

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.

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.

brettmc · 2022-01-05T05:32:52Z

have signed CLA now. I can't add labels, but I think php and release notes: no.
Also, this is a duplicate of #9359 but this PR goes a little further and adds some more type hints etc

zeriyoshi · 2022-01-07T03:15:24Z

Looks good.
I think that the PHP Extension needs to support these as well.
If there are no problems with the PHP Library, can you the PHP Extension be modified in the same way?

brettmc · 2022-01-07T12:14:42Z

@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

zeriyoshi · 2022-01-12T02:45:46Z

@brettmc
Very good job!

However, it seems that the minimum supported version of PHP has been changed to 7.1 or higher.
This needs to be mentioned in the release notes and Google needs to agree to this.

Perhaps You can lower the minimum required version of PHP, is that possible?

MrMage · 2022-01-12T08:56:03Z

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.

zeriyoshi · 2022-01-12T09:54:40Z

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.

brettmc · 2022-01-12T10:39:15Z

The reason for php 7.1 is because that's where the void return type was introduced (I agree that whether that's acceptable would be the project owner's decision). It might be possible to make the C extension work with both 8.1 and 7.1 (by conditionally compiling in the void return type), but I don't think that's true of the library.
I'm still struggling along with how to make the extension work in 8.1 whilst still being backwards-compatible, but I've just asked on the php-internals list for guidance.
As far at 8.1+ is concerned, it might be ok for users to ignore the deprecation warnings (in the project I'm working on, they cause tests to fail but that's a phpunit feature we could possibly disable). But, in php 9.0 they will all be a hard fail, so whenever that comes out, it will definitely not work without dropping support for <= 8.1

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

brettmc · 2022-01-13T01:59:53Z

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:
library: 7.1+
extension: 7.2+

How do we seek approval to drop 7.0/7.1 support? (and, could somebody please add release notes: yes)

zeriyoshi · 2022-01-13T02:04:52Z

Perhaps @haberman knows about it?

haberman · 2022-01-13T03:50:14Z

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.

brettmc · 2022-01-13T10:33:47Z

Asked on https://groups.google.com/g/grpc-io about my proposed changes.

brettmc · 2022-01-24T22:44:47Z

@haberman - the (hopefully) last issue with 32bit tests is that I added ext-bcmath as a required extension in composer.json (since it's used extensively, eg in Internal/GPBUtil), but it appears to not be enabled in the test setup for 7.1-zts. The path of least resistance would be for me to just remove requirement and leave everything as it was. Wdyt?

MrMage · 2022-01-25T09:20:42Z

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

brettmc · 2022-01-27T04:58:12Z

@haberman - update on the ext-bcmath dependency. The 32bit php-7.x-zts images used in testing do not have the bcmath extension. The tests currently pass because only the extension, and not the PHP library, are tested with -zts. That makes sense, you do not need bcmath for the extension.
I've added testing against 8.1, but note that some build process needs to publish a newer version of protobuftesting/php80_<hash> based on my Dockerfile changes - until that happens it falls back to php 8.0 which happens to be in the PATH. Oh, and because the hash changes, the test run may fail until a new image gets build, but I don't know what process takes care of that.
Lastly, I needed to update the php80 image from jessie to stretch since 8.1 would not compile with the openssl version which jessie provides.

brettmc · 2022-01-27T22:06:46Z

kokoro/linux/dockerfile/test/php80/Dockerfile

@@ -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


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

tony-sol

maybe added some safe, 8.0 and 7.x compatible typehints

UPD: suggested : void hints is not safe with 7.0, my fault for not checked this before comment

brettmc · 2022-01-28T22:56:00Z

php/src/Google/Protobuf/Internal/RepeatedField.php

@@ -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)


@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)

haberman · 2022-02-01T23:40:58Z

@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!

brettmc · 2022-02-02T01:54:30Z

@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 Linux PHP kokoro job, based on checking other PHP PRs

haberman · 2022-02-02T18:06:10Z

Hmm, it looks like only some of the presubmits ran. Let me try kicking it again.

haberman · 2022-02-02T22:17:33Z

All tests have passed, but it looks like 8.1 tests did not run. I am realizing that your edits to test.sh are not taking effect because the current Linux PHP build is using this script, which does not invoke tests.sh.

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.

MrMage · 2022-02-09T10:48:27Z

Thanks for merging this, @haberman! Are there any plans to tag a patch release that includes this fix?

fowles added php release notes: no labels Jan 5, 2022

fowles requested a review from haberman January 5, 2022 21:07

acozzette added the kokoro:run label Jan 6, 2022

protobuf-kokoro removed kokoro:run labels Jan 6, 2022

adding return type hints to php ext

fabe73d

haberman added the kokoro:run label Jan 7, 2022

protobuf-kokoro removed the kokoro:run label Jan 7, 2022

require bcmath

558d128

bcmath (specifically, the bccomp function) is internally required, and tests fail if it's not available

brettmc marked this pull request as draft January 12, 2022 10:39

conditionally adding mixed return type

0476a89

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

brettmc marked this pull request as ready for review January 13, 2022 00:45

brettmc marked this pull request as draft January 13, 2022 01:22

bump extension min version to 7.2

9e65df2

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

brettmc marked this pull request as ready for review January 13, 2022 01:59

haberman added the kokoro:run label Jan 14, 2022

protobuf-kokoro removed the kokoro:run label Jan 14, 2022

haberman added the kokoro:run label Jan 24, 2022

protobuf-kokoro removed the kokoro:run label Jan 24, 2022

brettmc added 2 commits January 25, 2022 21:00

removing ext-bcmath require

a78b89e

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

php8.1 testing

4c03fcf

haberman mentioned this pull request Jan 27, 2022

Segmentation Fault caused on PHP #9446

Closed

brettmc commented Jan 27, 2022

View reviewed changes

tony-sol reviewed Jan 28, 2022

View reviewed changes

brettmc commented Jan 28, 2022

View reviewed changes

caugner mentioned this pull request Jan 31, 2022

Fix PHP 8.1 compatibility #9359

Closed

haberman added the kokoro:run label Feb 1, 2022

protobuf-kokoro removed the kokoro:run label Feb 1, 2022

haberman added the kokoro:force-run label Feb 2, 2022

protobuf-kokoro removed the kokoro:force-run label Feb 2, 2022

haberman added the kokoro:force-run label Feb 2, 2022

protobuf-kokoro removed the kokoro:force-run label Feb 2, 2022

haberman merged commit 8c8fb0e into protocolbuffers:master Feb 2, 2022

This was referenced Feb 3, 2022

#9293 - incompatible return type php 8.1 #9301

Merged

PHP: protobuf 3.19.1 causes several Deprecation notices #9293

Closed

JanKyselica mentioned this pull request Feb 23, 2022

Warning about deprecated return type PHP 8.1 #9512

Closed

brettmc mentioned this pull request Mar 4, 2022

[3.20.0RC] [release-notes] add php8.1 compatibility to release notes #9587

Closed

taka-oyama mentioned this pull request Apr 18, 2022

Installation issue | symfony/cache does not support 6.1 version colopl/laravel-spanner#38

Closed

siketyan mentioned this pull request May 9, 2022

PHP 8.1 compatibility #10343

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixing php 8.1 deprecation warnings #9370

fixing php 8.1 deprecation warnings #9370

brettmc commented Jan 5, 2022

brettmc commented Jan 5, 2022

zeriyoshi commented Jan 7, 2022

brettmc commented Jan 7, 2022

zeriyoshi commented Jan 12, 2022

MrMage commented Jan 12, 2022 •

edited

Loading

zeriyoshi commented Jan 12, 2022

brettmc commented Jan 12, 2022

brettmc commented Jan 13, 2022

zeriyoshi commented Jan 13, 2022

haberman commented Jan 13, 2022

brettmc commented Jan 13, 2022

brettmc commented Jan 24, 2022

MrMage commented Jan 25, 2022

brettmc commented Jan 27, 2022 •

edited

Loading

brettmc Jan 27, 2022

tony-sol left a comment •

edited

Loading

brettmc Jan 28, 2022

haberman commented Feb 1, 2022

brettmc commented Feb 2, 2022 •

edited

Loading

haberman commented Feb 2, 2022

haberman commented Feb 2, 2022

MrMage commented Feb 9, 2022

fixing php 8.1 deprecation warnings #9370

fixing php 8.1 deprecation warnings #9370

Conversation

brettmc commented Jan 5, 2022

brettmc commented Jan 5, 2022

zeriyoshi commented Jan 7, 2022

brettmc commented Jan 7, 2022

zeriyoshi commented Jan 12, 2022

MrMage commented Jan 12, 2022 • edited Loading

zeriyoshi commented Jan 12, 2022

brettmc commented Jan 12, 2022

brettmc commented Jan 13, 2022

zeriyoshi commented Jan 13, 2022

haberman commented Jan 13, 2022

brettmc commented Jan 13, 2022

brettmc commented Jan 24, 2022

MrMage commented Jan 25, 2022

brettmc commented Jan 27, 2022 • edited Loading

brettmc Jan 27, 2022

Choose a reason for hiding this comment

tony-sol left a comment • edited Loading

Choose a reason for hiding this comment

brettmc Jan 28, 2022

Choose a reason for hiding this comment

haberman commented Feb 1, 2022

brettmc commented Feb 2, 2022 • edited Loading

haberman commented Feb 2, 2022

haberman commented Feb 2, 2022

MrMage commented Feb 9, 2022

MrMage commented Jan 12, 2022 •

edited

Loading

brettmc commented Jan 27, 2022 •

edited

Loading

tony-sol left a comment •

edited

Loading

brettmc commented Feb 2, 2022 •

edited

Loading