-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat: PHP 8.4 #996
feat: PHP 8.4 #996
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #996 +/- ##
==========================================
+ Coverage 77.94% 78.04% +0.09%
==========================================
Files 195 196 +1
Lines 26325 27104 +779
==========================================
+ Hits 20520 21154 +634
- Misses 5805 5950 +145
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tests/integration/api/error_group_callback/test_error_group_callback_error_web.php84.php
Show resolved
Hide resolved
`ZEND_MODULE_API_NO` for PHP 8.4 is actually `20240924`.
PHP's build system received a fix for 'out of tree' builds. This broke build of agent's unit tests which require agent's source directory to be in the include search path. Working agent's unit tests showed that `test_function_debug_name` needed its expectations tweaked - closures are named differently on PHP 8.4.
PHP 8.4 includes implementation of PDO driver specific sub-classes RFC (see https://wiki.php.net/rfc/pdo_driver_specific_subclasses). This means that when database access code uses new factory method `PDO::connect` to create PDO object, an instance of driver specific sub-class of `PDO` is returned instead of an instance of generic `PDO` class. This means that instrumentation of generic `PDO` class is not enough to provide instrumentation of datastores. Add wrappers for driver specific subclasses of `PDO` supported by the agent: `Pdo\Firebird`, `Pdo\Mysql`, `Pdo\Odbc`, `Pdo\Pgsql`, `Pdo\Sqlite`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thanks!
docs/development.md
Outdated
@@ -58,7 +58,7 @@ _(most operating systems package these with `-dev` or `-devel` suffixes)_ | |||
|
|||
### PHP | |||
|
|||
The PHP agent supports PHP versions `7.2`, `7.3`, `7.4`,`8.0`, `8.1`, '8.2', and `8.3`. | |||
The PHP agent supports PHP versions `7.2`, `7.3`, `7.4`,`8.0`, `8.1`, '8.2', `8.3` and `8.4`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'8.2' -> 8.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 654f61b
@@ -142,8 +142,8 @@ | |||
*/ | |||
|
|||
function a() { | |||
trigger_error("4 arg error", E_USER_ERROR); | |||
trigger_error("4 arg error", E_USER_WARNING); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A warning is not an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is for all occurences where E_USER_ERROR is changed to E_USER_WARNING
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E_USER_ERROR was deprecated in PHP 8.4. However, using E_USER_WARNING with trigger_error fulfills the same role for our purposes here and elsewhere in these tests.
PHP (or at the very least our configuration of PHP) itself is treating this as an error, calling any error handler set with set_error_handler. Thus, the agent also treats this as an error, recording the appropriate information. The differences in behavior, however, are 2-fold. 1) E_USER_WARNING is not deprecated, resulting in no additional warnings. and 2) E_USER_WARNING does not trigger a bailout, which some tests have been split into a pre/post 8.4 version to account for.
trigger_error ("lowest priority", E_USER_NOTICE); | ||
trigger_error ("goldilox", E_USER_DEPRECATED); | ||
trigger_error ("highest priority", E_USER_WARNING); | ||
trigger_error ("goldilox", E_USER_DEPRECATED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warnings have been changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E_USER_ERROR was deprecated, so these triggered errors were changed while maintaining their relative statuses. Previously, this test wasn't even fully doing what it intended to though, because E_USER_ERROR triggers a bailout and prevents the execution of further code. For the purposes of this test, because E_USER_ERROR is also the highest priority, that was resulting in tested behavior that would be indistinguishable from the agent just keeping the most recent error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds 8.4 to builds/tests/installers. Requires some updates to our make/configure scripts to adapt to changes in PHP autoconf macros.
Bumps monolog and guzzle versions for 8.4 support from those libraries.
Adjusts tests to account for new closure naming in PHP 8.4
Adjusts tests to account for deprecation of E_USER_ERROR