Skip to content
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.

Sign up for GitHub

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(agent): Drupal hook attribute instrumentation #1030

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

feat(agent): Drupal hook attribute instrumentation #1030

Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2a7da47
feat(agent): add drupal hook attribute instrumentation
bduranleau-nr Feb 25, 2025
9500752
fix: clean up mem freeing
bduranleau-nr Feb 25, 2025
efc0c4e
undef helper macro
bduranleau-nr Feb 25, 2025
3ded06d
review: reduce memory allocation
bduranleau-nr Feb 27, 2025
3c68c3d
review: remove frees
bduranleau-nr Feb 27, 2025
a68bd59
review: improve conditional
bduranleau-nr Feb 27, 2025
6b3d5b1
review: use STR_KEY_VAL zend loop
bduranleau-nr Feb 27, 2025
c811020
cleanup: remove undef
bduranleau-nr Feb 27, 2025
42d5af5
review: flatten conditional
bduranleau-nr Feb 27, 2025
d876e0e
fix warnings
bduranleau-nr Feb 27, 2025
ea0e4cf
clarify warning
bduranleau-nr Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 132 additions & 10 deletions agent/fw_drupal8.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,12 +605,128 @@ NR_PHP_WRAPPER(nr_drupal94_invoke_all_with_clean) {
NR_PHP_WRAPPER_END
#endif // OAPI

#define NR_FREE_HOOK_MEM \
nr_free(hook_str); \
nr_free(class_str); \
nr_free(method_str); \
nr_free(module_str); \
nr_free(hookpath);

/*
* Purpose: Instrument Drupal Attribute Hooks for Drupal 11.1+
*
* Params: 1. A zval pointer to the moduleHandler instance in use by Drupal.
*
* Return: bool
*
*/
static bool nr_drupal_hook_attribute_instrument(zval* module_handler) {
zval* hook_implementation_map = NULL;

nr_php_string_hash_key_t* hook_key = NULL;
zval* hook_val = NULL;
nr_php_string_hash_key_t* class_key = NULL;
zval* class_val = NULL;
nr_php_string_hash_key_t* method_key = NULL;
zval* module_val = NULL;
zend_ulong key_num = 0;

char* hook_str = NULL;
char* class_str = NULL;
char* method_str = NULL;
char* module_str = NULL;
char* hookpath = NULL;

Copy link
Contributor

Choose a reason for hiding this comment

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

Could check module_handler for NULL and exit early.

hook_implementation_map = nr_php_get_zval_object_property(
module_handler, "hookImplementationsMap");
if (hook_implementation_map) {
if (nr_php_is_zval_valid_array(hook_implementation_map)) {
ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(hook_implementation_map), key_num,
hook_key, hook_val) {
(void)key_num;
if ((NULL == hook_key) || (0 == nr_php_is_zval_valid_array(hook_val))) {
nrl_warning(NRL_FRAMEWORK,
"hookImplementationsMap[hook = %s]: invalid value",
NRSAFESTR(ZEND_STRING_VALUE(hook_key)));
NR_FREE_HOOK_MEM
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/newrelic/newrelic-php-agent/pull/1030/files#r1974532294
skip this round of the loop or completely exit?

}

ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(hook_val), key_num, class_key,
class_val) {
(void)key_num;
if ((NULL == class_key)
|| (0 == nr_php_is_zval_valid_array(class_val))) {
nrl_warning(NRL_FRAMEWORK,
"hookImplementationsMap[class = %s]: invalid value",
NRSAFESTR(ZEND_STRING_VALUE(class_key)));
NR_FREE_HOOK_MEM
return false;
}

ZEND_HASH_FOREACH_KEY_VAL(Z_ARRVAL_P(class_val), key_num, method_key,
module_val) {
(void)key_num;

NR_FREE_HOOK_MEM

if ((NULL == method_key)
|| (0 == nr_php_is_zval_valid_string(module_val))) {
nrl_warning(NRL_FRAMEWORK,
"hookImplementationsMap[method = %s]: invalid value",
NRSAFESTR(ZEND_STRING_VALUE(method_key)));
NR_FREE_HOOK_MEM
return false;
}

hook_str = nr_strdup(ZEND_STRING_VALUE(hook_key));
class_str = nr_strdup(ZEND_STRING_VALUE(class_key));
method_str = nr_strdup(ZEND_STRING_VALUE(method_key));
module_str = nr_strdup(Z_STRVAL_P(module_val));

if (0
== nr_stricmp(ZEND_STRING_VALUE(class_key),
"Drupal\\Core\\Extension\\ProceduralCall")) {
hookpath = nr_formatf("%s", method_str);
} else {
hookpath = nr_formatf("%s::%s", class_str, method_str);
}

nr_php_wrap_user_function_drupal(hookpath, nr_strlen(hookpath),
module_str, nr_strlen(module_str),
hook_str, nr_strlen(hook_str));
}
ZEND_HASH_FOREACH_END();
}
ZEND_HASH_FOREACH_END();
}
ZEND_HASH_FOREACH_END();

} else {
nrl_warning(NRL_FRAMEWORK,
"hookImplementationsMap property not a valid array");
NR_FREE_HOOK_MEM
return false;
}
} else {
nrl_warning(NRL_FRAMEWORK, "NULL hookImplementationsMap object property");
NR_FREE_HOOK_MEM
return false;
}

NR_FREE_HOOK_MEM
return true;
}

#undef NR_FREE_HOOK_MEM

/*
* Purpose : Wrap the invoke() method of the module handler instance in use.
*/
NR_PHP_WRAPPER(nr_drupal8_module_handler) {
zend_class_entry* ce = NULL;
zval** retval_ptr = NR_GET_RETURN_VALUE_PTR;
bool hook_attribute_instrumentation = false;

NR_UNUSED_SPECIALFN;
(void)wraprec;
Expand All @@ -632,20 +748,26 @@ NR_PHP_WRAPPER(nr_drupal8_module_handler) {

ce = Z_OBJCE_P(*retval_ptr);

nr_drupal8_add_method_callback(ce, NR_PSTR("getimplementations"),
nr_drupal8_post_get_implementations TSRMLS_CC);
nr_drupal8_add_method_callback(ce, NR_PSTR("implementshook"),
nr_drupal8_post_implements_hook TSRMLS_CC);
/* Drupal 9.4 introduced a replacement method for getImplentations */
hook_attribute_instrumentation
= nr_drupal_hook_attribute_instrument(*retval_ptr);

if (!hook_attribute_instrumentation) {
Copy link
Member

Choose a reason for hiding this comment

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

nr_drupal_hook_attribute_instrument will only return true if it's able to walk the entire hookImplementationsMap. What will happen if it fails mid way? Which hook instrumentation will be used? Does nr_drupal_hook_attribute_instrument cleanup its partial work if it fails to walk the entire hookImplementationsMap? Could you add a test for this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If walking the map fails halfway through, we exit the function and return false. The wraprecs that are already created remain, and we revert back to the old method of wrapping hooks.

Which hook instrumentation will be used?

The answer would be "both". The attribute based instrumentation would persist for anything it managed to wrap, and the other methods act as a fallback to hopefully plug the gaps. I don't see anticipate any negative side-effects from this, do you?

Could you add a test for this case?

This would be difficult and doesn't fit neatly into our established test paradigms, The dependency here is a lot of Drupal code, from Drupal::moduleHandler to the hookImplementationsMap. Is there a specific concern you have with this failing partway though walking the map?

Copy link
Member

Choose a reason for hiding this comment

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

Given #1030 (comment), I would feel more comfortable having a test with mocked implementation of Drupal\Core\Extension\ModuleHandlerInterface that has corrupted hookImplementationsMap property in every possible way - invalid type of the property (not an array), first level has invalid key and value, second level has invalid key and value, third level has invalid key and value.

Copy link
Member

@lavarou lavarou Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Take a look here and here for inspiration how to mock implementation of Drupal\Core\Extension\ModuleHandlerInterface and use it in integration test.

nr_drupal8_add_method_callback(
ce, NR_PSTR("getimplementations"),
nr_drupal8_post_get_implementations TSRMLS_CC);
nr_drupal8_add_method_callback(ce, NR_PSTR("implementshook"),
nr_drupal8_post_implements_hook TSRMLS_CC);
/* Drupal 9.4 introduced a replacement method for getImplentations */
#if ZEND_MODULE_API_NO >= ZEND_8_0_X_API_NO \
&& !defined OVERWRITE_ZEND_EXECUTE_DATA
nr_drupal8_add_method_callback_before_after_clean(
ce, NR_PSTR("invokeallwith"), nr_drupal94_invoke_all_with,
nr_drupal94_invoke_all_with_after, nr_drupal94_invoke_all_with_clean);
nr_drupal8_add_method_callback_before_after_clean(
ce, NR_PSTR("invokeallwith"), nr_drupal94_invoke_all_with,
nr_drupal94_invoke_all_with_after, nr_drupal94_invoke_all_with_clean);
#else
nr_drupal8_add_method_callback(ce, NR_PSTR("invokeallwith"),
nr_drupal94_invoke_all_with TSRMLS_CC);
nr_drupal8_add_method_callback(ce, NR_PSTR("invokeallwith"),
nr_drupal94_invoke_all_with TSRMLS_CC);
#endif
}
}
NR_PHP_WRAPPER_END

Expand Down
Loading