Fix compatibility with PHP 8 #270

MrMage · 2021-04-26T07:35:41Z

Fixes #267.

This PR includes the following changes:

Remove obsolete locking macros (https://www.seidengroup.com/2020/12/07/porting-extensions-to-php-8/, hat tip to @frost-byte)
Update some calls that now take a zend_object * instead of a zval *
Add now-required arginfo for a lot of methods that do not take any arguments
Remove an obsolete reference to PHP versions earlier than 7.0
Add CI for PHP 8.0
Remove CI for older versions of PHP, as the extensions contains backward-incompatible changes
Update all tests for compatibility with PHPUnit 9
Update composer.json for compatibility with PHP 8 and some newer package versions
Skip two failing integration tests

Once this is merged, I recommend tagging a 0.7.0 release. Note that that release will no longer be compatible with PHP 7, as PHP 8 introduced a bunch of backwards-incompatible changes to interfaces used by this extension.

Merging this PR will resolve a bunch of issues in this repo (formatted so that GitHub can parse them):

Resolves Use PHPUnit 7.5 #233.
Resolves PHP 7.3 compatibility #241.
Resolves make test fails in PHP 7.4 #250.
Resolves Fix failing test for PHP 7.4 #251.
Resolves replace curly braces with square braces #254.
Resolves Build error on Mac from Pecl #255.
Resolves Update dependencies to allow Ramsey/Uuid version 4 #256.
Resolves PHP 7.4 Compatibility Fix #258.
Resolves Fix use of curly brace string access deprecated in PHP 7.4. #260.
Resolves Support ramsey/uuid v4 #261.
Resolves php7.4 compatible code release branch #265.
Resolves PHP 8 compatibility? #267.

MrMage · 2021-04-26T07:39:55Z

Note that compiling the extension unit tests passes; the build failure seems due to unrelated code coverage issues with PHPUnit.

MrMage · 2021-04-26T07:43:49Z

Some work left: https://app.circleci.com/pipelines/github/census-instrumentation/opencensus-php/9/workflows/178d0b1f-3908-45bf-9f4f-9e2376406297/jobs/2663

MrMage · 2021-05-06T12:34:14Z

@bshaffer could you please take a look?

MrMage · 2021-05-12T14:44:09Z

A review from @bshaffer or @chingor13 would be greatly appreciated :-)

CoolGoose · 2021-05-13T12:01:18Z

For what it's worth, this patch does compile nicely on 8.0.3 👍 (working on other non related php8 compat until i can test this properly)

bshaffer

I found some small nits but for the most part this looks good. Would love to get a C-review from @chingor13 !

.circleci/config.yml

bshaffer · 2021-05-17T23:56:02Z

tests/unit/Trace/TracerTest.php

    {
+        parent::setUp();


This shouldn't be nesssary, nothing happens in setUp of the parent test case.

It's good practice. Avoids nasty surprises if stuff later does get added to the parent implementation of setUp.

I disagree with this strongly, it's confusing because it implies that something's being called in the setUp method. If ever this class gets extended (which is highly unlikely), it's very obvious to the person making the change that such logic would need to be added here (if the extended class contained the same methods). That's basic programming IMHO and does not need an attempt at future-proofing.

Also, this is a test class, so the risk is very small.

bshaffer · 2021-05-17T23:56:20Z

tests/unit/Trace/Tracer/ExtentionTracerTest.php

    {
+        parent::setUp();


Same here... no reason to call parent::setUp

bshaffer · 2021-05-17T23:56:25Z

tests/unit/Trace/Tracer/ContextTracerTest.php

    {
+        parent::setUp();


Same here... no reason to call parent::setUp

bshaffer · 2021-05-17T23:57:14Z

tests/integration/curl/tests/CurlTest.php

    {
+        parent::setUpBeforeClass();


Same here... no reason to call parent::setUpBeforeClass

bshaffer · 2021-05-17T23:57:20Z

tests/integration/curl/tests/CurlTest.php

    {
+        parent::setUp();


Same here... no reason to call parent::setUp

MrMage · 2021-05-18T07:51:54Z

@bshaffer PTAL. I have added integration tests for PHP 7.4 now (including WordPress), but I'd like to keep the parent::setUp calls to avoid nasty surprises if stuff later does get added to the parent implementation of setUp.

MrMage · 2021-05-25T14:49:40Z

Friendly ping @bshaffer @chingor13.

bshaffer

Once the unnecessary parent calls are removed, this is LGTM from the PHP-side for me. We will still need @chingor13 to sign off on the C-side

MrMage · 2021-05-25T17:46:01Z

@bshaffer did you see my comment on why it makes sense to keep these calls in? Also, could @chingor13 start reviewing the C part already?

…

On 25. May 2021, at 19:26, Brent Shaffer ***@***.***> wrote: @bshaffer requested changes on this pull request. Once the unnecessary parent calls are removed, this is LGTM from the PHP-side for me. We will still need @chingor13 to sign off on the C-side — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

MrMage · 2021-05-25T17:46:51Z

Never mind, only saw your comments on that now. Will remove those tomorrow.

…

On 25. May 2021, at 19:26, Brent Shaffer ***@***.***> wrote: @bshaffer requested changes on this pull request. Once the unnecessary parent calls are removed, this is LGTM from the PHP-side for me. We will still need @chingor13 to sign off on the C-side — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

bshaffer · 2021-05-25T17:57:17Z

Never mind, only saw your comments on that now. Will remove those tomorrow.

I know it's a nit, so thank you!

MrMage · 2021-05-26T07:48:27Z

@bshaffer removed the parent calls now, PTAL :-)

Would you mind pinging chingor13 to have a look at the C part? I'd like to get this merged soon, if possible.

MrMage · 2021-05-28T06:40:03Z

Thanks for the reviews! @bshaffer, would you please merge this?

bshaffer · 2021-05-31T16:36:16Z

@MrMage mergred and released v0.7.0!

thanks for your hard work on this!!

MrMage · 2021-06-04T08:35:14Z

Thanks @bshaffer! It would be great if someone with the appropriate permissions (probably @chingor13) could also update the package on PECL; it hasn't been updated in a while (also see #243 and #252).

chingor13 · 2021-06-09T22:46:22Z

0.3.0 should be available on PECL

Fix compatibility with PHP 8.

1e7a25a

Add CI for PHP 8.

7bf5d60

MrMage marked this pull request as draft April 26, 2021 07:44

MrMage added 8 commits April 26, 2021 10:01

Also add CI for PHP 7.4.

407a6cd

Try fixing another compiler error.

ca2efdc

Fix two more flags that have been removed.

c6f2280

Try making an extension test compatible with PHP 7.4.

f747721

Solve the PHP 7.4 test compatibility issues the proper way.

f860754

Enable Xdebug coverage in more places.

6083f84

Add support for UUID v4 to fix our integration tests.

1f8829b

Add a bunch of arginfo annotations.

05fcc64

MrMage force-pushed the php8-compat branch from fed7e0e to 05fcc64 Compare April 26, 2021 08:48

MrMage added 16 commits April 26, 2021 10:51

More arginfo fixes.

d38d8b4

Remove call_user_function_ex.

3c72422

Update a bunch of zend_read_property calls.

4fb4005

More Z_OBJ_P fixes.

e7f7a52

Yet more Z_OBJ_P.

e5e471a

Also update our _php_math_longtobase calls.

2747006

Update PHP CodeSniffer.

2b64d6d

Update PHPUnit.

2f55e72

Remove PHP 7 CI; make tests compatible with PHPUnit 9.

9b16d04

Fix a few exceptions; require PHP 8.

7f6fb82

More PHPUnit fixes.

a2f1c32

More compatibility fixes.

e66aa54

Fix another PHPUnit method signature.

6d830fc

More integration test fixes.

4e90929

Yet more integration test fixes...

a814fce

Remove a reference to phpunit.xml.dist.

99d996b

MrMage requested review from bshaffer and chingor13 May 5, 2021 09:00

Use PHP_MAJOR_VERSION instead of PHP_VERSION_ID.

467d646

bshaffer reviewed May 17, 2021

View reviewed changes

MrMage added 4 commits May 18, 2021 09:32

Minor macro tweak.

01914df

Try running integration tests on PHP 7.4 instead.

544ce56

Fix a typo and add integration tests on both 7.4 and 8.0.

f1f8b2a

Another WordPress test fix.

28c1a3e

MrMage requested a review from bshaffer May 25, 2021 14:49

bshaffer suggested changes May 25, 2021

View reviewed changes

Remove unnecessary parent setup calls.

024aac7

MrMage requested a review from bshaffer May 26, 2021 07:47

MrMage mentioned this pull request May 26, 2021

Support ramsey/uuid v4 #261

Closed

bshaffer approved these changes May 27, 2021

View reviewed changes

chingor13 approved these changes May 27, 2021

View reviewed changes

bshaffer merged commit 0cfda50 into census-instrumentation:master May 31, 2021

lepiaf mentioned this pull request Nov 25, 2021

Allow psr/log 2.0 and 3.0 #282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix compatibility with PHP 8 #270

Fix compatibility with PHP 8 #270

MrMage commented Apr 26, 2021 •

edited

Loading

MrMage commented Apr 26, 2021

MrMage commented Apr 26, 2021

MrMage commented May 6, 2021

MrMage commented May 12, 2021

CoolGoose commented May 13, 2021

bshaffer left a comment

bshaffer May 17, 2021

MrMage May 18, 2021

bshaffer May 25, 2021 •

edited

Loading

bshaffer May 17, 2021

bshaffer May 17, 2021

bshaffer May 17, 2021

bshaffer May 17, 2021

MrMage commented May 18, 2021

MrMage commented May 25, 2021

bshaffer left a comment

MrMage commented May 25, 2021 via email

MrMage commented May 25, 2021 via email

bshaffer commented May 25, 2021

MrMage commented May 26, 2021

MrMage commented May 28, 2021

bshaffer commented May 31, 2021 •

edited

Loading

MrMage commented Jun 4, 2021

chingor13 commented Jun 9, 2021

Fix compatibility with PHP 8 #270

Fix compatibility with PHP 8 #270

Conversation

MrMage commented Apr 26, 2021 • edited Loading

MrMage commented Apr 26, 2021

MrMage commented Apr 26, 2021

MrMage commented May 6, 2021

MrMage commented May 12, 2021

CoolGoose commented May 13, 2021

bshaffer left a comment

Choose a reason for hiding this comment

bshaffer May 17, 2021

Choose a reason for hiding this comment

MrMage May 18, 2021

Choose a reason for hiding this comment

bshaffer May 25, 2021 • edited Loading

Choose a reason for hiding this comment

bshaffer May 17, 2021

Choose a reason for hiding this comment

bshaffer May 17, 2021

Choose a reason for hiding this comment

bshaffer May 17, 2021

Choose a reason for hiding this comment

bshaffer May 17, 2021

Choose a reason for hiding this comment

MrMage commented May 18, 2021

MrMage commented May 25, 2021

bshaffer left a comment

Choose a reason for hiding this comment

MrMage commented May 25, 2021 via email

MrMage commented May 25, 2021 via email

bshaffer commented May 25, 2021

MrMage commented May 26, 2021

MrMage commented May 28, 2021

bshaffer commented May 31, 2021 • edited Loading

MrMage commented Jun 4, 2021

chingor13 commented Jun 9, 2021

MrMage commented Apr 26, 2021 •

edited

Loading

bshaffer May 25, 2021 •

edited

Loading

bshaffer commented May 31, 2021 •

edited

Loading