Fix compatibility with PHP 8 #270
Conversation
|
Note that compiling the extension unit tests passes; the build failure seems due to unrelated code coverage issues with PHPUnit. |
|
@bshaffer could you please take a look? |
|
A review from @bshaffer or @chingor13 would be greatly appreciated :-) |
|
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) |
There was a problem hiding this comment.
I found some small nits but for the most part this looks good. Would love to get a C-review from @chingor13 !
tests/unit/Trace/TracerTest.php
Outdated
| { | ||
| parent::setUp(); |
There was a problem hiding this comment.
This shouldn't be nesssary, nothing happens in setUp of the parent test case.
There was a problem hiding this comment.
It's good practice. Avoids nasty surprises if stuff later does get added to the parent implementation of setUp.
There was a problem hiding this comment.
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.
| { | ||
| parent::setUp(); |
There was a problem hiding this comment.
Same here... no reason to call parent::setUp
| { | ||
| parent::setUp(); |
There was a problem hiding this comment.
Same here... no reason to call parent::setUp
| { | ||
| parent::setUpBeforeClass(); |
There was a problem hiding this comment.
Same here... no reason to call parent::setUpBeforeClass
| { | ||
| parent::setUp(); |
There was a problem hiding this comment.
Same here... no reason to call parent::setUp
|
@bshaffer PTAL. I have added integration tests for PHP 7.4 now (including WordPress), but I'd like to keep the |
|
Friendly ping @bshaffer @chingor13. |
There was a problem hiding this comment.
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
|
@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.
|
|
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.
|
I know it's a nit, so thank you! |
|
@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. |
|
Thanks for the reviews! @bshaffer, would you please merge this? |
|
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). |
|
0.3.0 should be available on PECL |
Fixes #267.
This PR includes the following changes:
zend_object *instead of azval *arginfofor a lot of methods that do not take any argumentscomposer.jsonfor compatibility with PHP 8 and some newer package versionsOnce this is merged, I recommend tagging a
0.7.0release. 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):
make testfails in PHP 7.4 #250.