-
Notifications
You must be signed in to change notification settings - Fork 69
fix(agent): fix Drupal error and exception handling #995
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
Conversation
|
!= nr_php_error_record_exception(NRPRG(txn), exception, priority, true, | ||
NULL, | ||
&NRPRG(exception_filters) TSRMLS_CC)) { | ||
nrl_verbosedebug(NRL_TXN, "Drupal: unable to record exception"); |
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.
Should this be higher priority than verbosedebug
?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #995 +/- ##
==========================================
- Coverage 78.68% 78.60% -0.08%
==========================================
Files 196 196
Lines 27047 27076 +29
==========================================
+ Hits 21281 21283 +2
- Misses 5766 5793 +27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
/* Warning avoidance */ | ||
(void)wraprec; | ||
|
||
NR_PHP_WRAPPER_REQUIRE_FRAMEWORK(NR_FW_DRUPAL8); |
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.
Should this use a variation of the NR_PHP_WRAPPER_REQUIRE_FRAMEWORK
macro (which doesnt yet exist) to make sure a compatible version of Drupal is present? This works with 9+ correct?
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 fixes exceptions for 8 and exceptions and errors for 9+. For Drupal 8 errors, this code has no impact (positive or negative) as the problem with error reporting originates from this block unique to pre-oapi instrumentation, and D8 is only compatible with PHP up to 7.4.
|
||
/* | ||
* Get the exception from the event. | ||
*/ |
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 code seems to rely on nr_php_call()
gracefully handling if these methods did not exist. Have you verified this is how it works?
I am also curious if these calls could return a valid zval that is not an exception object and perhaps it would be best to verify it is what we expect?
Or does nr_php_error_record_exception()
handle this inspection and verification?
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 code seems to rely on nr_php_call() gracefully handling if these methods did not exist. Have you verified this is how it works?
I believe this is outside the scope of this PR. We use nr_php_call
in this same manner throughout the agent, but for what it's worth, I do believe it handles the case that it does not exist gracefully.
I am also curious if these calls could return a valid zval that is not an exception object and perhaps it would be best to verify it is what we expect?
The very first call in nr_php_error_record_exception
is to nr_php_error_zval_is_exception
.
Co-authored-by: Michal Nowacki <[email protected]>
Co-authored-by: Michal Nowacki <[email protected]>
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
No description provided.