Skip to content

Commit

Permalink
remove ErrorHandler wrapper for PHPs 8.0+
Browse files Browse the repository at this point in the history
Agent for PHPs 8.0+ uses observer API to hook into Zend Engine, which allows
offers better out-of-the-box instrumentation of user exception handler. When
observer API is used, there's no need to wrap exception handlers to record
errors.
  • Loading branch information
lavarou committed Apr 12, 2024
1 parent c94a4bd commit 3d90b20
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion agent/fw_yii.c
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,8 @@ NR_PHP_WRAPPER(nr_yii2_runWithParams_wrapper) {
}
NR_PHP_WRAPPER_END

#if ZEND_MODULE_API_NO < ZEND_8_0_X_API_NO \
|| defined OVERWRITE_ZEND_EXECUTE_DATA
/*
* Yii2: Report errors and exceptions when built-in ErrorHandler is enabled.
*/
Expand Down Expand Up @@ -184,6 +186,7 @@ NR_PHP_WRAPPER(nr_yii2_error_handler_wrapper) {
nr_php_arg_release(&exception);
}
NR_PHP_WRAPPER_END
#endif

/*
* Enable Yii2 instrumentation.
Expand All @@ -202,7 +205,6 @@ void nr_yii2_enable(TSRMLS_D) {
nr_yii2_runWithParams_wrapper TSRMLS_CC);
nr_php_wrap_user_function(NR_PSTR("yii\\base\\InlineAction::runWithParams"),
nr_yii2_runWithParams_wrapper TSRMLS_CC);
#endif
/*
* Wrap Yii2 global error and exception handling methods.
* Given that: ErrorHandler::handleException(), ::handleError() and
Expand All @@ -217,4 +219,5 @@ void nr_yii2_enable(TSRMLS_D) {
*/
nr_php_wrap_user_function(NR_PSTR("yii\\base\\ErrorHandler::logException"),
nr_yii2_error_handler_wrapper TSRMLS_CC);
#endif
}

3 comments on commit 3d90b20

@razvanphp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is true, I was using PHP 8.2 and had absolutely no error reported in NewRelic.

@lavarou
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@razvanphp This PR is now based on #877, which has the fix that addresses this problem.

@lavarou
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@razvanphp Your patience and collaboration is greatly appreciated! πŸ™‡ The tests revealing this problem, which I mention here, are passing. However, I would appreciate if you could verify, that the agent built from this PR reports errors in your test environment.

Please sign in to comment.