From 4e02a93b112cbbd1afb29adf45ca3219fc67a125 Mon Sep 17 00:00:00 2001 From: Alexandre Rulleau Date: Thu, 27 Aug 2026 15:08:58 +0200 Subject: [PATCH 1/7] fix(drupal): hook the theme engine service and guarantee render hook removal Drupal 11.3 moved template rendering to a theme_engine service (ThemeEngineInterface::renderTemplate), and ThemeManager::render() only falls back to the deprecated {engine}_render_template() global when no such service resolves. Core still includes twig.engine, so function_exists() stays true while the function is never called: the integration installed a hook per render that never fired and never self-removed, so drupal.template.file was missing on every drupal.theme.render span and datadog.trace.hook_limit was reached within a single request. Hook ThemeEngineInterface::renderTemplate once at init() to cover every engine implementation, re-appending .html.twig for TwigThemeEngine so the tag keeps its pre-11.3 value, and take the legacy per-render branch only when no engine service resolves. The membership test goes through the themeEngines service collection rather than getThemeEngine(), which would instantiate the engine even on core's early-return path. install_hook's callback is not gated by the span limit while trace_method is, so past the limit a nested render has no span of its own and active_span() returns the outer render's. Bail out when the tracer is limited rather than tag that span with the nested template. Independently, ThemeManager::render() can return without ever calling the render function (unknown theme hook, exception) on every Drupal version, so the callback's self-removal is no longer the only removal path: the posthook now removes the id the prehook recorded for that span. Keyed by span rather than a stack because the posthook is skipped for a dropped span. APMS-20395 --- .../Integrations/Drupal/DrupalIntegration.php | 58 +++++++-- .../drupal/drupal_integration.inc | 71 +++++++++++ tests/ext/integrations/drupal/drupal_root.inc | 16 +++ .../drupal/drupal_twig_engine.inc | 29 +++++ .../drupal/theme_engine_service.phpt | 119 ++++++++++++++++++ .../theme_engine_service_preprocess.phpt | 112 +++++++++++++++++ .../theme_engine_service_span_limit.phpt | 118 +++++++++++++++++ .../drupal/theme_render_early_return.phpt | 90 +++++++++++++ 8 files changed, 603 insertions(+), 10 deletions(-) create mode 100644 tests/ext/integrations/drupal/drupal_integration.inc create mode 100644 tests/ext/integrations/drupal/drupal_root.inc create mode 100644 tests/ext/integrations/drupal/drupal_twig_engine.inc create mode 100644 tests/ext/integrations/drupal/theme_engine_service.phpt create mode 100644 tests/ext/integrations/drupal/theme_engine_service_preprocess.phpt create mode 100644 tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt create mode 100644 tests/ext/integrations/drupal/theme_render_early_return.phpt diff --git a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php index 27cd7cffb05..0056184525f 100644 --- a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php +++ b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php @@ -228,12 +228,41 @@ static function ($registry, $scope, $args) { } ); + // Drupal 11.3+ renders through a theme_engine service instead of {engine}_render_template(). + install_hook( + 'Drupal\Core\Theme\ThemeEngineInterface::renderTemplate', + static function (HookData $hook) { + $span = active_span(); + // Past the span limit a nested render gets no span, so active_span() would be + // the outer render's and tagging it would attribute the wrong template. + if (!$span || $span->name !== 'drupal.theme.render' || \dd_trace_tracer_is_limited()) { + return; + } + + $file = $hook->args[0]; + // Core passes the path without its extension here; re-append it so the tag keeps + // the value it had before 11.3. + if (isset($hook->instance) && $hook->instance instanceof \Drupal\Core\Template\TwigThemeEngine) { + $file .= '.html.twig'; + } + $span->meta['drupal.template.file'] = $file; + } + ); + + // Legacy per-render hook ids, keyed by render span: the posthook is skipped for a + // dropped span, which would desync a positional stack. + $renderHookIds = []; + trace_method( 'Drupal\Core\Theme\ThemeManager', 'render', [ 'recurse' => true, - 'prehook' => function (SpanData $span, $args) { + 'prehook' => function (SpanData $span, $args) use (&$renderHookIds) { + // Reset first, so a stale entry left by a dropped span can never be reused. + $spanKey = \spl_object_hash($span); + $renderHookIds[$spanKey] = 0; + $span->name = 'drupal.theme.render'; $span->service = \ddtrace_config_app_name('drupal'); Integration::tagFrameworkServiceSource($span, 'drupal'); @@ -251,23 +280,32 @@ static function ($registry, $scope, $args) { if (!empty($themeEngine)) { $span->meta['drupal.render.engine'] = $themeEngine; - if (function_exists("{$themeEngine}_render_template")) { - $renderFunction = "{$themeEngine}_render_template"; - - // The theme engine may use a different extension and a different renderer - // Moreover, Drupal can use different themes in the same application - // The render function will always be called during the ThemeManager::render call - install_hook( - $renderFunction, + // The theme engine may use a different extension and a different renderer + // Moreover, Drupal can use different themes in the same application + // Take the legacy branch only when no engine service resolves, as core does. + $hasEngineService = \property_exists($this, 'themeEngines') + && $this->themeEngines->has($themeEngine); + if (!$hasEngineService && \function_exists("{$themeEngine}_render_template")) { + $renderHookIds[$spanKey] = install_hook( + "{$themeEngine}_render_template", static function (HookData $hook) use ($span) { $span->meta['drupal.template.file'] = $hook->args[0]; + // Self-removal keeps outer/inner render pairing correct. remove_hook($hook->id); } ); } } }, - 'posthook' => function (SpanData $span, $args) { + 'posthook' => function (SpanData $span, $args) use (&$renderHookIds) { + // render() can return without ever calling the render function (unknown theme + // hook, exception), so the callback's self-removal cannot be the only one. + $spanKey = \spl_object_hash($span); + if (!empty($renderHookIds[$spanKey])) { + remove_hook($renderHookIds[$spanKey]); + } + unset($renderHookIds[$spanKey]); + /** @var null|\Drupal\Core\Theme\Registry $themeRegistry */ $themeRegistry = ObjectKVStore::get($this, 'theme_registry'); if ($themeRegistry) { diff --git a/tests/ext/integrations/drupal/drupal_integration.inc b/tests/ext/integrations/drupal/drupal_integration.inc new file mode 100644 index 00000000000..0b028413bb1 --- /dev/null +++ b/tests/ext/integrations/drupal/drupal_integration.inc @@ -0,0 +1,71 @@ +'; + $tags[$file] = (isset($tags[$file]) ? $tags[$file] : 0) + 1; + // Also reported so that a prehook exception swallowed by the sandbox cannot pass silently. + $engine = isset($span['meta']['drupal.render.engine']) ? $span['meta']['drupal.render.engine'] : ''; + $engines[$engine] = (isset($engines[$engine]) ? $engines[$engine] : 0) + 1; + } + ksort($tags); + ksort($engines); + foreach ($engines as $engine => $count) { + echo "drupal.render.engine[$engine] = $count\n"; + } + foreach ($tags as $file => $count) { + echo "drupal.template.file[$file] = $count\n"; + } + + // A leaked per-render hook shows up as an exhausted budget on its target. + foreach ($probeTargets as $target) { + $probe = DDTrace\install_hook($target, function () { + }); + echo "hook budget left [$target]: " . ($probe ? "yes" : "no") . "\n"; + if ($probe) { + DDTrace\remove_hook($probe); + } + } +} diff --git a/tests/ext/integrations/drupal/drupal_root.inc b/tests/ext/integrations/drupal/drupal_root.inc new file mode 100644 index 00000000000..4e6c0cf64aa --- /dev/null +++ b/tests/ext/integrations/drupal/drupal_root.inc @@ -0,0 +1,16 @@ +nested) { + $nested = $this->nested; + $this->nested = null; + $rendered .= $nested(); + } + return $rendered; + } +} diff --git a/tests/ext/integrations/drupal/theme_engine_service.phpt b/tests/ext/integrations/drupal/theme_engine_service.phpt new file mode 100644 index 00000000000..56cd01ce3e6 --- /dev/null +++ b/tests/ext/integrations/drupal/theme_engine_service.phpt @@ -0,0 +1,119 @@ +--TEST-- +Drupal 11.3+ renders through the theme engine service (APMS-20395) +--SKIPIF-- + +--ENV-- +DD_TRACE_AUTO_FLUSH_ENABLED=0 +DD_TRACE_GENERATE_ROOT_SPAN=0 +DD_CODE_ORIGIN_FOR_SPANS_ENABLED=0 +DD_TRACE_LOG_LEVEL=warn,startup=off +--INI-- +datadog.trace.hook_limit=20 +--FILE-- +engines = $engines; + } + + public function has($name) + { + return isset($this->engines[$name]); + } + + public function get($name) + { + return $this->engines[$name]; + } + } + + class ThemeManager + { + // Protected upstream, so the prehook only reaches it when rebound to this scope. + protected $themeEngines; + + public function __construct(ThemeEngineCollection $themeEngines) + { + $this->themeEngines = $themeEngines; + } + + public function getActiveTheme() + { + return new ActiveTheme(); + } + + public function render($hook, array $variables = []) + { + $render_function = [$this->themeEngines->get('twig'), 'renderTemplate']; + return $render_function("core/themes/claro/templates/$hook", $variables); + } + } +} + +namespace +{ + // twig.engine is still included on 11.3+, so the deprecated global exists but is dead. + function twig_render_template($template_file, array $variables) + { + return "legacy $template_file"; + } + + include __DIR__ . '/drupal_integration.inc'; + + DDTrace\Integrations\Drupal\DrupalIntegration::init(); + + // Only touched after init(), so the engine class is autoloaded mid-request. + var_dump(class_exists('Drupal\Core\Template\TwigThemeEngine', false)); + $engines = new Drupal\Core\Theme\ThemeEngineCollection([ + 'twig' => new Drupal\Core\Template\TwigThemeEngine(), + ]); + + $themeManager = new Drupal\Core\Theme\ThemeManager($engines); + for ($i = 0; $i < 30; ++$i) { + $themeManager->render('page'); + } + + dd_drupal_render_report(dd_trace_serialize_closed_spans(), [ + 'Drupal\Core\Template\TwigThemeEngine::renderTemplate', + 'twig_render_template', + ]); +} +?> +--EXPECT-- +bool(false) +drupal.render.engine[twig] = 30 +drupal.template.file[core/themes/claro/templates/page.html.twig] = 30 +hook budget left [Drupal\Core\Template\TwigThemeEngine::renderTemplate]: yes +hook budget left [twig_render_template]: yes diff --git a/tests/ext/integrations/drupal/theme_engine_service_preprocess.phpt b/tests/ext/integrations/drupal/theme_engine_service_preprocess.phpt new file mode 100644 index 00000000000..9804a4b1fdb --- /dev/null +++ b/tests/ext/integrations/drupal/theme_engine_service_preprocess.phpt @@ -0,0 +1,112 @@ +--TEST-- +Drupal render spans keep their own template when a sub-element renders first (APMS-20395) +--SKIPIF-- + +--ENV-- +DD_TRACE_AUTO_FLUSH_ENABLED=0 +DD_TRACE_GENERATE_ROOT_SPAN=0 +DD_CODE_ORIGIN_FOR_SPANS_ENABLED=0 +DD_TRACE_LOG_LEVEL=warn,startup=off +--INI-- +datadog.trace.hook_limit=20 +--FILE-- +engines = $engines; + } + + public function has($name) + { + return isset($this->engines[$name]); + } + + public function get($name) + { + return $this->engines[$name]; + } + } + + class ThemeManager + { + protected $themeEngines; + + public function __construct(ThemeEngineCollection $themeEngines) + { + $this->themeEngines = $themeEngines; + } + + public function getActiveTheme() + { + return new ActiveTheme(); + } + + public function render($hook, array $variables = []) + { + // A preprocess callback renders a sub-element before the outer template itself + // (ThemeManager.php:366 vs :428), so the child's renderTemplate() fires first. + // This is why the tag cannot be first-write-wins. + if ($hook === 'page') { + $this->render('child'); + } + $render_function = [$this->themeEngines->get('twig'), 'renderTemplate']; + return $render_function("core/themes/claro/templates/$hook", $variables); + } + } +} + +namespace +{ + include __DIR__ . '/drupal_integration.inc'; + + DDTrace\Integrations\Drupal\DrupalIntegration::init(); + + $engines = new Drupal\Core\Theme\ThemeEngineCollection([ + 'twig' => new Drupal\Core\Template\TwigThemeEngine(), + ]); + $themeManager = new Drupal\Core\Theme\ThemeManager($engines); + + for ($i = 0; $i < 10; ++$i) { + $themeManager->render('page'); + } + + dd_drupal_render_report(dd_trace_serialize_closed_spans(), [ + 'Drupal\Core\Template\TwigThemeEngine::renderTemplate', + ]); +} +?> +--EXPECT-- +drupal.render.engine[twig] = 20 +drupal.template.file[core/themes/claro/templates/child.html.twig] = 10 +drupal.template.file[core/themes/claro/templates/page.html.twig] = 10 +hook budget left [Drupal\Core\Template\TwigThemeEngine::renderTemplate]: yes diff --git a/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt b/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt new file mode 100644 index 00000000000..ce6f10dc2a4 --- /dev/null +++ b/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt @@ -0,0 +1,118 @@ +--TEST-- +Drupal render span is not tagged with a nested template past the span limit (APMS-20395) +--SKIPIF-- + +--ENV-- +DD_TRACE_AUTO_FLUSH_ENABLED=0 +DD_TRACE_GENERATE_ROOT_SPAN=0 +DD_CODE_ORIGIN_FOR_SPANS_ENABLED=0 +DD_TRACE_LOG_LEVEL=warn,startup=off +--INI-- +datadog.trace.hook_limit=20 +--FILE-- +engines = $engines; + } + + public function has($name) + { + return isset($this->engines[$name]); + } + + public function get($name) + { + return $this->engines[$name]; + } + } + + class ThemeManager + { + protected $themeEngines; + + public function __construct(ThemeEngineCollection $themeEngines) + { + $this->themeEngines = $themeEngines; + } + + public function getActiveTheme() + { + return new ActiveTheme(); + } + + public function render($hook, array $variables = []) + { + $render_function = [$this->themeEngines->get('twig'), 'renderTemplate']; + return $render_function("core/themes/claro/templates/$hook", $variables); + } + } +} + +namespace +{ + include __DIR__ . '/drupal_integration.inc'; + + DDTrace\Integrations\Drupal\DrupalIntegration::init(); + + $engine = new Drupal\Core\Template\TwigThemeEngine(); + $engines = new Drupal\Core\Theme\ThemeEngineCollection(['twig' => $engine]); + $themeManager = new Drupal\Core\Theme\ThemeManager($engines); + + // Twig renders an embedded theme hook from inside the outer template, i.e. a nested + // ThemeManager::render() during the outer renderTemplate(). + $engine->nested = function () use ($themeManager) { + return $themeManager->render('block'); + }; + + // install_hook's callback is not gated by the span limit, but trace_method is. Leave room + // for exactly one span so the outer render is traced and the nested one is not: the nested + // renderTemplate() then still fires the interface hook while active_span() is the outer + // render's span. init() forces spans_limit >= 1500, so shrink it afterwards. + ini_set('datadog.trace.spans_limit', 2); + DDTrace\start_span(); + DDTrace\close_span(); + + $themeManager->render('page'); + + echo "limited: ", var_export((bool) dd_trace_tracer_is_limited(), true), "\n"; + dd_drupal_render_report(dd_trace_serialize_closed_spans(), [ + 'Drupal\Core\Template\TwigThemeEngine::renderTemplate', + ]); +} +?> +--EXPECT-- +limited: true +drupal.render.engine[twig] = 1 +drupal.template.file[] = 1 +hook budget left [Drupal\Core\Template\TwigThemeEngine::renderTemplate]: yes diff --git a/tests/ext/integrations/drupal/theme_render_early_return.phpt b/tests/ext/integrations/drupal/theme_render_early_return.phpt new file mode 100644 index 00000000000..f2005302f1f --- /dev/null +++ b/tests/ext/integrations/drupal/theme_render_early_return.phpt @@ -0,0 +1,90 @@ +--TEST-- +Drupal render hooks are removed when the render function is never called (APMS-20395) +--SKIPIF-- + +--ENV-- +DD_TRACE_AUTO_FLUSH_ENABLED=0 +DD_TRACE_GENERATE_ROOT_SPAN=0 +DD_CODE_ORIGIN_FOR_SPANS_ENABLED=0 +DD_TRACE_LOG_LEVEL=warn,startup=off +--INI-- +datadog.trace.hook_limit=20 +--FILE-- +render('block'); + } + return $rendered; + } + } +} + +namespace +{ + function twig_render_template($template_file, array $variables) + { + return "rendered $template_file"; + } + + include __DIR__ . '/drupal_integration.inc'; + + DDTrace\Integrations\Drupal\DrupalIntegration::init(); + + $themeManager = new Drupal\Core\Theme\ThemeManager(); + // Renders that never reach the render function must not exhaust the hook budget... + for ($i = 0; $i < 30; ++$i) { + $themeManager->render('missing'); + } + // ...nor leave a hook behind that later mis-tags an unrelated render. + for ($i = 0; $i < 30; ++$i) { + $themeManager->render('page'); + } + + // Only the legacy global is installed on this shape, so it is the only budget to probe. + dd_drupal_render_report(dd_trace_serialize_closed_spans(), ['twig_render_template']); +} +?> +--EXPECT-- +drupal.render.engine[twig] = 90 +drupal.template.file[] = 30 +drupal.template.file[core/themes/olivero/templates/block.html.twig] = 30 +drupal.template.file[core/themes/olivero/templates/page.html.twig] = 30 +hook budget left [twig_render_template]: yes From 18183bf07b6d3353c5a7865770323afff3842d60 Mon Sep 17 00:00:00 2001 From: Alexandre Rulleau Date: Fri, 28 Aug 2026 16:22:04 +0200 Subject: [PATCH 2/7] fix(drupal): remove the render hook before resetting its slot The ThemeManager::render prehook reset $renderHookIds[$spanKey] to 0 unconditionally. A dropped span skips the posthook (dd_uhook_end gates the end hook on !dyn->dropped_span in tracer/hook/uhook_legacy.c), so the slot could still hold a live, installed hook id. Once the freed SpanData's object handle was recycled, spl_object_hash collided and the reset discarded that id without removing the hook, orphaning it for the rest of the request. Remove the hook before resetting the slot. remove_hook() on an already removed id is a no-op, so the common self-removal path is unaffected. APMS-20395 --- src/DDTrace/Integrations/Drupal/DrupalIntegration.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php index 0056184525f..0049687c4cd 100644 --- a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php +++ b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php @@ -259,8 +259,11 @@ static function (HookData $hook) { [ 'recurse' => true, 'prehook' => function (SpanData $span, $args) use (&$renderHookIds) { - // Reset first, so a stale entry left by a dropped span can never be reused. + // A dropped span skips the posthook, so its hook may still be installed. $spanKey = \spl_object_hash($span); + if (!empty($renderHookIds[$spanKey])) { + remove_hook($renderHookIds[$spanKey]); + } $renderHookIds[$spanKey] = 0; $span->name = 'drupal.theme.render'; From 2e0987cd3cd91f606e1b5bf291161ddd2a1e25c8 Mon Sep 17 00:00:00 2001 From: Alexandre Rulleau Date: Mon, 31 Aug 2026 11:30:18 +0200 Subject: [PATCH 3/7] refactor(drupal): carry the legacy render hook id via HookData::data Per bwoebi's review on #4145: use install_hook() and pass the legacy {engine}_render_template() hook id through $hook->data instead of keeping a map keyed by spl_object_hash($span). $hook->data is per-invocation, so recursion needs no keying at all, and spl_object_hash (deprecated in PHP 8.6) is gone without reaching for spl_object_id, which is PHP 7.2+ while this package supports PHP 7.0. The install_hook end hook also runs where the tracing posthook does not: it is gated only on the begin hook having run (tracer/hook/uhook.c:424-430), whereas the tracing posthook is skipped for a dropped span (tracer/hook/uhook_legacy.c:208-226) and for a span-limited call (uhook_legacy.c:102-105). Removal is therefore unconditional rather than best-effort, which theme_render_dropped_span.phpt now covers. install_hook callbacks are not span-limit gated while trace_method is, so the tag write keeps the active-span guard, now shared by both engine paths; theme_render_span_limit.phpt guards the legacy path against the nested-render mis-tagging that the guard prevents. APMS-20395 --- .../Integrations/Drupal/DrupalIntegration.php | 95 ++++++++-------- .../drupal/theme_render_dropped_span.phpt | 101 ++++++++++++++++++ .../drupal/theme_render_span_limit.phpt | 89 +++++++++++++++ 3 files changed, 241 insertions(+), 44 deletions(-) create mode 100644 tests/ext/integrations/drupal/theme_render_dropped_span.phpt create mode 100644 tests/ext/integrations/drupal/theme_render_span_limit.phpt diff --git a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php index 0049687c4cd..9248b142f59 100644 --- a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php +++ b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php @@ -228,44 +228,74 @@ static function ($registry, $scope, $args) { } ); + // install_hook's callbacks are not gated by the span limit while trace_method is, so + // past the limit a nested render gets no span and active_span() is the outer render's: + // tagging it would attribute the wrong template. + $tagTemplateFile = static function ($file) { + $span = active_span(); + if (!$span || $span->name !== 'drupal.theme.render' || \dd_trace_tracer_is_limited()) { + return; + } + $span->meta['drupal.template.file'] = $file; + }; + // Drupal 11.3+ renders through a theme_engine service instead of {engine}_render_template(). install_hook( 'Drupal\Core\Theme\ThemeEngineInterface::renderTemplate', - static function (HookData $hook) { - $span = active_span(); - // Past the span limit a nested render gets no span, so active_span() would be - // the outer render's and tagging it would attribute the wrong template. - if (!$span || $span->name !== 'drupal.theme.render' || \dd_trace_tracer_is_limited()) { - return; - } - + static function (HookData $hook) use ($tagTemplateFile) { $file = $hook->args[0]; // Core passes the path without its extension here; re-append it so the tag keeps // the value it had before 11.3. if (isset($hook->instance) && $hook->instance instanceof \Drupal\Core\Template\TwigThemeEngine) { $file .= '.html.twig'; } - $span->meta['drupal.template.file'] = $file; + $tagTemplateFile($file); } ); - // Legacy per-render hook ids, keyed by render span: the posthook is skipped for a - // dropped span, which would desync a positional stack. - $renderHookIds = []; + // Drupal <= 11.2 renders through the {engine}_render_template() global. The per-render + // hook id travels in $hook->data, which is per-invocation and therefore nests without + // bookkeeping; and this end hook still runs when the tracing posthook is skipped, + // i.e. for a dropped or span-limited render. + install_hook( + 'Drupal\Core\Theme\ThemeManager::render', + function (HookData $hook) use ($tagTemplateFile) { + /** @var \Drupal\Core\Theme\ThemeManager $this */ + $themeEngine = $this->getActiveTheme()->getEngine(); + // The theme engine may use a different extension and a different renderer + // Moreover, Drupal can use different themes in the same application + if (empty($themeEngine) || !\function_exists("{$themeEngine}_render_template")) { + return; + } + // Take the legacy branch only when no engine service resolves, as core does. + if (\property_exists($this, 'themeEngines') && $this->themeEngines->has($themeEngine)) { + return; + } + + $hook->data = install_hook( + "{$themeEngine}_render_template", + static function (HookData $renderHook) use ($tagTemplateFile) { + $tagTemplateFile($renderHook->args[0]); + // Self-removal keeps outer/inner render pairing correct. + remove_hook($renderHook->id); + } + ); + }, + static function (HookData $hook) { + // render() can return without ever calling the render function (unknown theme + // hook, exception), so the callback's self-removal cannot be the only one. + if (!empty($hook->data)) { + remove_hook($hook->data); + } + } + ); trace_method( 'Drupal\Core\Theme\ThemeManager', 'render', [ 'recurse' => true, - 'prehook' => function (SpanData $span, $args) use (&$renderHookIds) { - // A dropped span skips the posthook, so its hook may still be installed. - $spanKey = \spl_object_hash($span); - if (!empty($renderHookIds[$spanKey])) { - remove_hook($renderHookIds[$spanKey]); - } - $renderHookIds[$spanKey] = 0; - + 'prehook' => function (SpanData $span, $args) { $span->name = 'drupal.theme.render'; $span->service = \ddtrace_config_app_name('drupal'); Integration::tagFrameworkServiceSource($span, 'drupal'); @@ -283,32 +313,9 @@ static function (HookData $hook) { if (!empty($themeEngine)) { $span->meta['drupal.render.engine'] = $themeEngine; - // The theme engine may use a different extension and a different renderer - // Moreover, Drupal can use different themes in the same application - // Take the legacy branch only when no engine service resolves, as core does. - $hasEngineService = \property_exists($this, 'themeEngines') - && $this->themeEngines->has($themeEngine); - if (!$hasEngineService && \function_exists("{$themeEngine}_render_template")) { - $renderHookIds[$spanKey] = install_hook( - "{$themeEngine}_render_template", - static function (HookData $hook) use ($span) { - $span->meta['drupal.template.file'] = $hook->args[0]; - // Self-removal keeps outer/inner render pairing correct. - remove_hook($hook->id); - } - ); - } } }, - 'posthook' => function (SpanData $span, $args) use (&$renderHookIds) { - // render() can return without ever calling the render function (unknown theme - // hook, exception), so the callback's self-removal cannot be the only one. - $spanKey = \spl_object_hash($span); - if (!empty($renderHookIds[$spanKey])) { - remove_hook($renderHookIds[$spanKey]); - } - unset($renderHookIds[$spanKey]); - + 'posthook' => function (SpanData $span, $args) { /** @var null|\Drupal\Core\Theme\Registry $themeRegistry */ $themeRegistry = ObjectKVStore::get($this, 'theme_registry'); if ($themeRegistry) { diff --git a/tests/ext/integrations/drupal/theme_render_dropped_span.phpt b/tests/ext/integrations/drupal/theme_render_dropped_span.phpt new file mode 100644 index 00000000000..f1af9cf178f --- /dev/null +++ b/tests/ext/integrations/drupal/theme_render_dropped_span.phpt @@ -0,0 +1,101 @@ +--TEST-- +Drupal render hook is removed when the render span is dropped (APMS-20395) +--SKIPIF-- + +--ENV-- +DD_TRACE_AUTO_FLUSH_ENABLED=0 +DD_TRACE_GENERATE_ROOT_SPAN=0 +DD_CODE_ORIGIN_FOR_SPANS_ENABLED=0 +DD_TRACE_LOG_LEVEL=warn,startup=off +--INI-- +datadog.trace.hook_limit=20 +--FILE-- +name = 'test.root'; + + $themeManager = new Drupal\Core\Theme\ThemeManager(); + for ($i = 0; $i < 30; ++$i) { + $themeManager->render('dropped'); + } + // A leak above exhausts the budget, so this render can no longer tag its own template. + $themeManager->render('page'); + + DDTrace\close_span(); + + $spans = dd_trace_serialize_closed_spans(); + $names = []; + foreach ($spans as $span) { + $names[$span['name']] = (isset($names[$span['name']]) ? $names[$span['name']] : 0) + 1; + } + ksort($names); + foreach ($names as $name => $count) { + echo "span[$name] = $count\n"; + } + + dd_drupal_render_report($spans, ['twig_render_template']); +} +?> +--EXPECTF-- +[ddtrace] [error] [%d] Cannot run tracing closure for render(); spans out of sync; This message is only displayed once. Specify DD_TRACE_ONCE_LOGS=0 to show all messages. +span[drupal.theme.render] = 1 +span[test.root] = 1 +drupal.render.engine[twig] = 1 +drupal.template.file[core/themes/olivero/templates/page.html.twig] = 1 +hook budget left [twig_render_template]: yes diff --git a/tests/ext/integrations/drupal/theme_render_span_limit.phpt b/tests/ext/integrations/drupal/theme_render_span_limit.phpt new file mode 100644 index 00000000000..1af6904502a --- /dev/null +++ b/tests/ext/integrations/drupal/theme_render_span_limit.phpt @@ -0,0 +1,89 @@ +--TEST-- +Drupal legacy render span is not tagged with a nested template past the span limit (APMS-20395) +--SKIPIF-- + +--ENV-- +DD_TRACE_AUTO_FLUSH_ENABLED=0 +DD_TRACE_GENERATE_ROOT_SPAN=0 +DD_CODE_ORIGIN_FOR_SPANS_ENABLED=0 +DD_TRACE_LOG_LEVEL=warn,startup=off +--INI-- +datadog.trace.hook_limit=20 +--FILE-- += 1500, so shrink it afterwards. + ini_set('datadog.trace.spans_limit', 2); + DDTrace\start_span(); + DDTrace\close_span(); + + $themeManager->render('page', ['nested' => function () use ($themeManager) { + return $themeManager->render('block'); + }]); + + echo "limited: ", var_export((bool) dd_trace_tracer_is_limited(), true), "\n"; + dd_drupal_render_report(dd_trace_serialize_closed_spans(), ['twig_render_template']); +} +?> +--EXPECT-- +limited: true +drupal.render.engine[twig] = 1 +drupal.template.file[] = 1 +hook budget left [twig_render_template]: yes From 5acdc2466d350bfd63f5fe1f2ec867a8b15b67ba Mon Sep 17 00:00:00 2001 From: Alexandre Rulleau Date: Mon, 31 Aug 2026 12:20:37 +0200 Subject: [PATCH 4/7] fix(drupal): match the render span by identity, not name The template tag was targeted with a `$span->name === 'drupal.theme.render'` check, but under `'recurse' => true` an ancestor render span carries that same name, so the check cannot tell this frame's span from an outer frame's. When a nested render's span is hard-dropped after its tracing prehook ran, active_span() reverts to the ancestor and the inner template overwrote the outer render's correct tag. This is not purely a regression from carrying the hook id via HookData::data: the 11.3+ theme engine service path has been exposed since 4e02a93b1 added it, because that hook has always resolved its target with active_span(). The refactor widened the same latent bug to the legacy path. Capture the frame's own span once in the ThemeManager::render begin hook and compare by identity at the tag site, saving and restoring the enclosing value through $hook->data so nesting needs no keyed storage. The install_hook is now registered after the trace_method deliberately: begin hooks run in installation order, so active_span() there is this render's own span. The span-limit check stays, relocated to the capture point -- past the limit the frame has no span of its own and active_span() is the parent's, which identity alone cannot reject. APMS-20395 --- .../Integrations/Drupal/DrupalIntegration.php | 101 ++++++++------ ...me_engine_service_dropped_nested_span.phpt | 128 ++++++++++++++++++ .../theme_render_dropped_nested_span.phpt | 100 ++++++++++++++ 3 files changed, 284 insertions(+), 45 deletions(-) create mode 100644 tests/ext/integrations/drupal/theme_engine_service_dropped_nested_span.phpt create mode 100644 tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt diff --git a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php index 9248b142f59..dbec1a7e55e 100644 --- a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php +++ b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php @@ -228,15 +228,17 @@ static function ($registry, $scope, $args) { } ); - // install_hook's callbacks are not gated by the span limit while trace_method is, so - // past the limit a nested render gets no span and active_span() is the outer render's: - // tagging it would attribute the wrong template. - $tagTemplateFile = static function ($file) { - $span = active_span(); - if (!$span || $span->name !== 'drupal.theme.render' || \dd_trace_tracer_is_limited()) { - return; + // The span of the ThemeManager::render frame currently executing, or null when that + // frame has none of its own. An ancestor render span carries the same name under + // 'recurse' => true, so the tag target can only be matched by identity. + $renderSpan = null; + + $tagTemplateFile = static function ($file) use (&$renderSpan) { + // A nested render whose span was dropped leaves active_span() on its ancestor, + // which would otherwise take the inner template. + if ($renderSpan && $renderSpan === active_span()) { + $renderSpan->meta['drupal.template.file'] = $file; } - $span->meta['drupal.template.file'] = $file; }; // Drupal 11.3+ renders through a theme_engine service instead of {engine}_render_template(). @@ -253,43 +255,6 @@ static function (HookData $hook) use ($tagTemplateFile) { } ); - // Drupal <= 11.2 renders through the {engine}_render_template() global. The per-render - // hook id travels in $hook->data, which is per-invocation and therefore nests without - // bookkeeping; and this end hook still runs when the tracing posthook is skipped, - // i.e. for a dropped or span-limited render. - install_hook( - 'Drupal\Core\Theme\ThemeManager::render', - function (HookData $hook) use ($tagTemplateFile) { - /** @var \Drupal\Core\Theme\ThemeManager $this */ - $themeEngine = $this->getActiveTheme()->getEngine(); - // The theme engine may use a different extension and a different renderer - // Moreover, Drupal can use different themes in the same application - if (empty($themeEngine) || !\function_exists("{$themeEngine}_render_template")) { - return; - } - // Take the legacy branch only when no engine service resolves, as core does. - if (\property_exists($this, 'themeEngines') && $this->themeEngines->has($themeEngine)) { - return; - } - - $hook->data = install_hook( - "{$themeEngine}_render_template", - static function (HookData $renderHook) use ($tagTemplateFile) { - $tagTemplateFile($renderHook->args[0]); - // Self-removal keeps outer/inner render pairing correct. - remove_hook($renderHook->id); - } - ); - }, - static function (HookData $hook) { - // render() can return without ever calling the render function (unknown theme - // hook, exception), so the callback's self-removal cannot be the only one. - if (!empty($hook->data)) { - remove_hook($hook->data); - } - } - ); - trace_method( 'Drupal\Core\Theme\ThemeManager', 'render', @@ -387,6 +352,52 @@ static function (HookData $hook) { ] ); + // Must follow the trace_method: begin hooks run in install order, so active_span() is our own span. + // Drupal <= 11.2 renders through the {engine}_render_template() global; the per-render + // hook id travels in $hook->data, which is per-invocation and therefore nests without + // bookkeeping, and this end hook still runs when the tracing posthook is skipped, + // i.e. for a dropped or span-limited render. + install_hook( + 'Drupal\Core\Theme\ThemeManager::render', + function (HookData $hook) use (&$renderSpan, $tagTemplateFile) { + $enclosing = $renderSpan; + // Past the span limit this frame gets no span of its own and active_span() + // would be its parent's, so claim nothing. + $span = \dd_trace_tracer_is_limited() ? null : active_span(); + $renderSpan = ($span && $span->name === 'drupal.theme.render') ? $span : null; + $hook->data = [$enclosing, null]; + + /** @var \Drupal\Core\Theme\ThemeManager $this */ + $themeEngine = $this->getActiveTheme()->getEngine(); + // The theme engine may use a different extension and a different renderer + // Moreover, Drupal can use different themes in the same application + if (empty($themeEngine) || !\function_exists("{$themeEngine}_render_template")) { + return; + } + // Take the legacy branch only when no engine service resolves, as core does. + if (\property_exists($this, 'themeEngines') && $this->themeEngines->has($themeEngine)) { + return; + } + + $hook->data = [$enclosing, install_hook( + "{$themeEngine}_render_template", + static function (HookData $renderHook) use ($tagTemplateFile) { + $tagTemplateFile($renderHook->args[0]); + // Self-removal keeps outer/inner render pairing correct. + remove_hook($renderHook->id); + } + )]; + }, + static function (HookData $hook) use (&$renderSpan) { + $renderSpan = $hook->data[0]; + // render() can return without ever calling the render function (unknown theme + // hook, exception), so the callback's self-removal cannot be the only one. + if (!empty($hook->data[1])) { + remove_hook($hook->data[1]); + } + } + ); + hook_method( 'Drupal\Core\EventSubscriber\MainContentViewSubscriber', '__construct', diff --git a/tests/ext/integrations/drupal/theme_engine_service_dropped_nested_span.phpt b/tests/ext/integrations/drupal/theme_engine_service_dropped_nested_span.phpt new file mode 100644 index 00000000000..ce701a873fc --- /dev/null +++ b/tests/ext/integrations/drupal/theme_engine_service_dropped_nested_span.phpt @@ -0,0 +1,128 @@ +--TEST-- +Drupal render span is not tagged with a dropped nested render's template, engine service (APMS-20395) +--SKIPIF-- + +--ENV-- +DD_TRACE_AUTO_FLUSH_ENABLED=0 +DD_TRACE_GENERATE_ROOT_SPAN=0 +DD_CODE_ORIGIN_FOR_SPANS_ENABLED=0 +DD_TRACE_LOG_LEVEL=warn,startup=off +--INI-- +datadog.trace.hook_limit=20 +--FILE-- +engines = $engines; + } + + public function has($name) + { + return isset($this->engines[$name]); + } + + public function get($name) + { + return $this->engines[$name]; + } + } + + class ThemeManager + { + protected $themeEngines; + + public function __construct(ThemeEngineCollection $themeEngines) + { + $this->themeEngines = $themeEngines; + } + + public function getActiveTheme() + { + return new ActiveTheme(); + } + + public function render($hook, array $variables = []) + { + if ($hook === 'block') { + // Dropped after the tracing prehook ran, so the tracer is not limited; + // active_span() now points at the OUTER render. + \DDTrace\try_drop_span(\DDTrace\active_span()); + } + + $render_function = [$this->themeEngines->get('twig'), 'renderTemplate']; + return $render_function("core/themes/claro/templates/$hook", $variables); + } + } +} + +namespace +{ + include __DIR__ . '/drupal_integration.inc'; + + DDTrace\Integrations\Drupal\DrupalIntegration::init(); + + $engine = new Drupal\Core\Template\TwigThemeEngine(); + $engines = new Drupal\Core\Theme\ThemeEngineCollection(['twig' => $engine]); + $themeManager = new Drupal\Core\Theme\ThemeManager($engines); + + // An outer span is required: dropping a stack's root span closes it instead. + $root = DDTrace\start_span(); + $root->name = 'test.root'; + + // Twig renders an embedded theme hook from inside the outer template. + $engine->nested = function () use ($themeManager) { + return $themeManager->render('block'); + }; + + $themeManager->render('page'); + + DDTrace\close_span(); + + $spans = dd_trace_serialize_closed_spans(); + foreach ($spans as $span) { + if ($span['name'] !== 'drupal.theme.render') { + continue; + } + echo "surviving render span template.file = ", + isset($span['meta']['drupal.template.file']) ? $span['meta']['drupal.template.file'] : '', + "\n"; + } + dd_drupal_render_report($spans, ['Drupal\Core\Template\TwigThemeEngine::renderTemplate']); +} +?> +--EXPECTF-- +[ddtrace] [error] [%d] Cannot run tracing closure for render(); spans out of sync; This message is only displayed once. Specify DD_TRACE_ONCE_LOGS=0 to show all messages. +surviving render span template.file = core/themes/claro/templates/page.html.twig +drupal.render.engine[twig] = 1 +drupal.template.file[core/themes/claro/templates/page.html.twig] = 1 +hook budget left [Drupal\Core\Template\TwigThemeEngine::renderTemplate]: yes diff --git a/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt b/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt new file mode 100644 index 00000000000..5bd1d9fd1d3 --- /dev/null +++ b/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt @@ -0,0 +1,100 @@ +--TEST-- +Drupal legacy render span is not tagged with a dropped nested render's template (APMS-20395) +--SKIPIF-- + +--ENV-- +DD_TRACE_AUTO_FLUSH_ENABLED=0 +DD_TRACE_GENERATE_ROOT_SPAN=0 +DD_CODE_ORIGIN_FOR_SPANS_ENABLED=0 +DD_TRACE_LOG_LEVEL=warn,startup=off +--INI-- +datadog.trace.hook_limit=20 +--FILE-- +name = 'test.root'; + + $themeManager = new Drupal\Core\Theme\ThemeManager(); + $themeManager->render('page', ['nested' => function () use ($themeManager) { + return $themeManager->render('block'); + }]); + + DDTrace\close_span(); + + $spans = dd_trace_serialize_closed_spans(); + foreach ($spans as $span) { + if ($span['name'] !== 'drupal.theme.render') { + continue; + } + echo "surviving render span template.file = ", + isset($span['meta']['drupal.template.file']) ? $span['meta']['drupal.template.file'] : '', + "\n"; + } + dd_drupal_render_report($spans, ['twig_render_template']); +} +?> +--EXPECTF-- +[ddtrace] [error] [%d] Cannot run tracing closure for render(); spans out of sync; This message is only displayed once. Specify DD_TRACE_ONCE_LOGS=0 to show all messages. +surviving render span template.file = core/themes/olivero/templates/page.html.twig +drupal.render.engine[twig] = 1 +drupal.template.file[core/themes/olivero/templates/page.html.twig] = 1 +hook budget left [twig_render_template]: yes From baef8479d65936306574afca7af5eaabaaee6fdb Mon Sep 17 00:00:00 2001 From: Alexandre Rulleau Date: Mon, 31 Aug 2026 12:38:03 +0200 Subject: [PATCH 5/7] style(drupal): trim comments to the non-obvious why --- .../Integrations/Drupal/DrupalIntegration.php | 26 +++++++------------ .../drupal/drupal_integration.inc | 10 ++----- tests/ext/integrations/drupal/drupal_root.inc | 3 +-- .../drupal/drupal_twig_engine.inc | 9 ++----- .../drupal/theme_engine_service.phpt | 3 +-- ...me_engine_service_dropped_nested_span.phpt | 3 +-- .../theme_engine_service_preprocess.phpt | 5 ++-- .../theme_engine_service_span_limit.phpt | 9 +++---- .../theme_render_dropped_nested_span.phpt | 3 +-- .../drupal/theme_render_dropped_span.phpt | 4 +-- .../drupal/theme_render_span_limit.phpt | 8 +++--- 11 files changed, 27 insertions(+), 56 deletions(-) diff --git a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php index dbec1a7e55e..28be83ffc51 100644 --- a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php +++ b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php @@ -228,14 +228,12 @@ static function ($registry, $scope, $args) { } ); - // The span of the ThemeManager::render frame currently executing, or null when that - // frame has none of its own. An ancestor render span carries the same name under - // 'recurse' => true, so the tag target can only be matched by identity. + // The span of the executing ThemeManager::render frame, or null when it has none of its own. + // Ancestors share its name under 'recurse' => true, so it can only be matched by identity. $renderSpan = null; $tagTemplateFile = static function ($file) use (&$renderSpan) { - // A nested render whose span was dropped leaves active_span() on its ancestor, - // which would otherwise take the inner template. + // A nested render whose span was dropped leaves active_span() on an ancestor. if ($renderSpan && $renderSpan === active_span()) { $renderSpan->meta['drupal.template.file'] = $file; } @@ -246,8 +244,7 @@ static function ($registry, $scope, $args) { 'Drupal\Core\Theme\ThemeEngineInterface::renderTemplate', static function (HookData $hook) use ($tagTemplateFile) { $file = $hook->args[0]; - // Core passes the path without its extension here; re-append it so the tag keeps - // the value it had before 11.3. + // Core passes the path without its extension here; re-append it to keep the pre-11.3 value. if (isset($hook->instance) && $hook->instance instanceof \Drupal\Core\Template\TwigThemeEngine) { $file .= '.html.twig'; } @@ -352,25 +349,21 @@ static function (HookData $hook) use ($tagTemplateFile) { ] ); + // Drupal <= 11.2 renders through the {engine}_render_template() global; unlike the tracing + // posthook, this end hook also runs for a dropped or span-limited render. + // Must follow the trace_method: begin hooks run in install order, so active_span() is our own span. - // Drupal <= 11.2 renders through the {engine}_render_template() global; the per-render - // hook id travels in $hook->data, which is per-invocation and therefore nests without - // bookkeeping, and this end hook still runs when the tracing posthook is skipped, - // i.e. for a dropped or span-limited render. install_hook( 'Drupal\Core\Theme\ThemeManager::render', function (HookData $hook) use (&$renderSpan, $tagTemplateFile) { $enclosing = $renderSpan; - // Past the span limit this frame gets no span of its own and active_span() - // would be its parent's, so claim nothing. + // Past the span limit this frame gets no span of its own, so claim nothing. $span = \dd_trace_tracer_is_limited() ? null : active_span(); $renderSpan = ($span && $span->name === 'drupal.theme.render') ? $span : null; $hook->data = [$enclosing, null]; /** @var \Drupal\Core\Theme\ThemeManager $this */ $themeEngine = $this->getActiveTheme()->getEngine(); - // The theme engine may use a different extension and a different renderer - // Moreover, Drupal can use different themes in the same application if (empty($themeEngine) || !\function_exists("{$themeEngine}_render_template")) { return; } @@ -390,8 +383,7 @@ static function (HookData $renderHook) use ($tagTemplateFile) { }, static function (HookData $hook) use (&$renderSpan) { $renderSpan = $hook->data[0]; - // render() can return without ever calling the render function (unknown theme - // hook, exception), so the callback's self-removal cannot be the only one. + // render() can return without calling the render function, so self-removal is not enough. if (!empty($hook->data[1])) { remove_hook($hook->data[1]); } diff --git a/tests/ext/integrations/drupal/drupal_integration.inc b/tests/ext/integrations/drupal/drupal_integration.inc index 0b028413bb1..acc37175613 100644 --- a/tests/ext/integrations/drupal/drupal_integration.inc +++ b/tests/ext/integrations/drupal/drupal_integration.inc @@ -4,8 +4,7 @@ require_once __DIR__ . '/drupal_root.inc'; $root = dd_drupal_tracer_root(); -// Declared on demand, never eagerly: Drupal autoloads the engine mid-request, so this is -// what exercises hook re-registration on class declaration rather than early binding. +// Declared on demand: only a mid-request class declaration exercises hook re-registration. spl_autoload_register(function ($class) { if ($class === 'Drupal\Core\Template\TwigThemeEngine') { require __DIR__ . '/drupal_twig_engine.inc'; @@ -30,12 +29,7 @@ spl_autoload_register(function ($class) use ($root) { echo "autoload miss: $class\n"; }); -/** - * @param array $spans Serialized closed spans. - * @param array $probeTargets Hook targets whose remaining budget should be reported. Each - * install site needs its own entry: the budget is per target, so - * probing only the legacy global would miss a leak on the method. - */ +// The hook budget is per target, so $probeTargets needs an entry per install site. function dd_drupal_render_report(array $spans, array $probeTargets) { $tags = []; diff --git a/tests/ext/integrations/drupal/drupal_root.inc b/tests/ext/integrations/drupal/drupal_root.inc index 4e6c0cf64aa..190ad2188d3 100644 --- a/tests/ext/integrations/drupal/drupal_root.inc +++ b/tests/ext/integrations/drupal/drupal_root.inc @@ -1,7 +1,6 @@ render('child'); } diff --git a/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt b/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt index ce6f10dc2a4..687218aa0a6 100644 --- a/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt +++ b/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt @@ -89,16 +89,13 @@ namespace $engines = new Drupal\Core\Theme\ThemeEngineCollection(['twig' => $engine]); $themeManager = new Drupal\Core\Theme\ThemeManager($engines); - // Twig renders an embedded theme hook from inside the outer template, i.e. a nested - // ThemeManager::render() during the outer renderTemplate(). + // Twig renders an embedded theme hook, i.e. a nested render during the outer renderTemplate(). $engine->nested = function () use ($themeManager) { return $themeManager->render('block'); }; - // install_hook's callback is not gated by the span limit, but trace_method is. Leave room - // for exactly one span so the outer render is traced and the nested one is not: the nested - // renderTemplate() then still fires the interface hook while active_span() is the outer - // render's span. init() forces spans_limit >= 1500, so shrink it afterwards. + // install_hook's callback is not span-limit gated, but trace_method is: with room for one span + // the nested render fires the hook while active_span() is the outer's. init() forces >= 1500. ini_set('datadog.trace.spans_limit', 2); DDTrace\start_span(); DDTrace\close_span(); diff --git a/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt b/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt index 5bd1d9fd1d3..4e498e97366 100644 --- a/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt +++ b/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt @@ -43,8 +43,7 @@ namespace Drupal\Core\Theme public function render($hook, array $variables = []) { if ($hook === 'block') { - // Dropped after the tracing prehook ran, so the render hook is installed and - // the tracer is not limited; active_span() now points at the OUTER render. + // Dropped after the prehook ran, so the hook is installed and active_span() is the OUTER render. \DDTrace\try_drop_span(\DDTrace\active_span()); } diff --git a/tests/ext/integrations/drupal/theme_render_dropped_span.phpt b/tests/ext/integrations/drupal/theme_render_dropped_span.phpt index f1af9cf178f..e86cc36e96f 100644 --- a/tests/ext/integrations/drupal/theme_render_dropped_span.phpt +++ b/tests/ext/integrations/drupal/theme_render_dropped_span.phpt @@ -43,8 +43,8 @@ namespace Drupal\Core\Theme public function render($hook, array $variables = []) { if ($hook === 'dropped') { - // A sampling filter or another integration can drop the render span. The - // tracing posthook is then skipped, so it cannot be what removes the hook. + // A sampling filter or another integration can drop the render span; the tracing + // posthook is then skipped, so it cannot be what removes the hook. \DDTrace\try_drop_span(\DDTrace\active_span()); return false; } diff --git a/tests/ext/integrations/drupal/theme_render_span_limit.phpt b/tests/ext/integrations/drupal/theme_render_span_limit.phpt index 1af6904502a..b39c1c42a04 100644 --- a/tests/ext/integrations/drupal/theme_render_span_limit.phpt +++ b/tests/ext/integrations/drupal/theme_render_span_limit.phpt @@ -53,8 +53,7 @@ namespace { $rendered = "rendered $template_file"; if (isset($variables['nested'])) { - // A template embedding another theme hook, i.e. a nested ThemeManager::render() - // from inside the render function. + // A template embedding another theme hook: a nested render from the render function. $nested = $variables['nested']; $rendered .= $nested(); } @@ -67,9 +66,8 @@ namespace $themeManager = new Drupal\Core\Theme\ThemeManager(); - // The legacy hook is installed from an install_hook callback, which is not gated by the - // span limit, so the nested render still fires it while active_span() is the outer - // render's span. init() forces spans_limit >= 1500, so shrink it afterwards. + // The legacy hook is installed from an install_hook callback, which is not span-limit gated, + // so the nested render fires it while active_span() is the outer's. init() forces >= 1500. ini_set('datadog.trace.spans_limit', 2); DDTrace\start_span(); DDTrace\close_span(); From 698fc72af9e164f830db55d29eb2fde81fc6ce03 Mon Sep 17 00:00:00 2001 From: Alexandre Rulleau Date: Mon, 31 Aug 2026 13:46:05 +0200 Subject: [PATCH 6/7] refactor(drupal): create the render span from the hook itself Fold the ThemeManager::render trace_method into the install_hook that already wrapped it, so a single hook owns the drupal.theme.render span. allowNestedHook() is not needed: it escapes reentrancy initiated from a hook callback (uhook.stub.php:106-110, uhook.c:1090-1103), not self-recursion. install_hook has no persistent reentrancy guard -- def->running is set only around the callback (uhook.c:361-363, :482-490) -- so it is unconditionally recursive. trace_method instead holds def->active across the whole call (uhook_legacy.c:107 -> :250), which is why it needed 'recurse' => true. Measured on a 3-deep plus sibling nest: begin 4 / end 4 either way. HookData::span() is keyed by invocation (uhook.c:870-905) and uses the same allocator as trace_method (span.c:513-578), so it hands each frame its own span. That removes the two guards the split design required: dd_trace_tracer_is_limited() and the $renderSpan === active_span() identity check. Past the span limit a nested render now gets a dummy span that is never pushed (uhook.c:875-879) instead of no span at all, so it can no longer leave active_span() on an ancestor -- and the outer render, whose span was allocated before the limit was reached, keeps its own template instead of losing the tag. $tagTemplateFile and the $renderSpan save/restore via HookData::data stay: the theme-engine hook fires in renderTemplate's frame, so its own span() would be the wrong span. The "spans out of sync" LOG_ONCE (uhook_legacy.c:216) can no longer be emitted, as it lives on trace_method's dropped-span path; three tests drop that expectation. The two span-limit tests now assert the outer render keeps page.html.twig, and gained a probe proving the nested render really ran so the assertion cannot pass vacuously. --- .../Integrations/Drupal/DrupalIntegration.php | 222 +++++++++--------- .../drupal/drupal_integration.inc | 2 +- ...me_engine_service_dropped_nested_span.phpt | 6 +- .../theme_engine_service_span_limit.phpt | 11 +- .../theme_render_dropped_nested_span.phpt | 6 +- .../drupal/theme_render_dropped_span.phpt | 9 +- .../drupal/theme_render_span_limit.phpt | 11 +- 7 files changed, 130 insertions(+), 137 deletions(-) diff --git a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php index 28be83ffc51..2e088375d5d 100644 --- a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php +++ b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php @@ -228,13 +228,12 @@ static function ($registry, $scope, $args) { } ); - // The span of the executing ThemeManager::render frame, or null when it has none of its own. - // Ancestors share its name under 'recurse' => true, so it can only be matched by identity. + // The span of the executing ThemeManager::render frame. The render function is hooked in its + // own frame, so it cannot reach that span on its own. $renderSpan = null; $tagTemplateFile = static function ($file) use (&$renderSpan) { - // A nested render whose span was dropped leaves active_span() on an ancestor. - if ($renderSpan && $renderSpan === active_span()) { + if ($renderSpan) { $renderSpan->meta['drupal.template.file'] = $file; } }; @@ -252,127 +251,44 @@ static function (HookData $hook) use ($tagTemplateFile) { } ); - trace_method( - 'Drupal\Core\Theme\ThemeManager', - 'render', - [ - 'recurse' => true, - 'prehook' => function (SpanData $span, $args) { - $span->name = 'drupal.theme.render'; - $span->service = \ddtrace_config_app_name('drupal'); - Integration::tagFrameworkServiceSource($span, 'drupal'); - $span->type = Type::WEB_SERVLET; - $span->meta[Tag::COMPONENT] = DrupalIntegration::NAME; - - /** @var \Drupal\Core\Theme\ThemeManager $this */ - $activeTheme = $this->getActiveTheme(); - $themeName = $activeTheme->getName(); - $themeEngine = $activeTheme->getEngine(); - - if (!empty($themeName)) { - $span->meta['drupal.render.theme'] = $themeName; - } - - if (!empty($themeEngine)) { - $span->meta['drupal.render.engine'] = $themeEngine; - } - }, - 'posthook' => function (SpanData $span, $args) { - /** @var null|\Drupal\Core\Theme\Registry $themeRegistry */ - $themeRegistry = ObjectKVStore::get($this, 'theme_registry'); - if ($themeRegistry) { - $runtimeThemeRegistry = $themeRegistry->getRuntime(); - $hook = $args[0]; - - if (is_array($hook)) { - foreach ($hook as $candidate) { - if ($runtimeThemeRegistry->has($candidate)) { - break; - } - } - $hook = $candidate; - } - - $originalHook = $hook; - - if (!$runtimeThemeRegistry->has($hook)) { - // Iteratively strip everything after the last '__' delimiter, until an - // implementation is found - while ($pos = strrpos($hook, '__')) { - $hook = substr($hook, 0, $pos); - if ($runtimeThemeRegistry->has($hook)) { - break; - } - } - } - - if ($runtimeThemeRegistry->has($hook)) { - $span->meta['drupal.render.hook'] = $span->resource = $hook; - $info = $runtimeThemeRegistry->get($hook); - - if (isset($info['base hook'])) { - $span->meta['drupal.render.base_hook'] = $info['base hook']; - } - - if (isset($info['type'])) { - $span->meta['drupal.render.type'] = $info['type']; - } - - if (isset($info['render element'])) { - $span->meta['drupal.render.element'] = $info['render element']; - } + install_hook( + 'Drupal\Core\Theme\ThemeManager::render', + function (HookData $hookData) use (&$renderSpan, $tagTemplateFile) { + $span = $hookData->span(); + $enclosing = $renderSpan; + $renderSpan = $span; + $hookData->data = [$enclosing, null]; - if (isset($info['template'])) { - $span->meta['drupal.template.template'] = $info['template']; - } + $span->name = 'drupal.theme.render'; + $span->service = \ddtrace_config_app_name('drupal'); + Integration::tagFrameworkServiceSource($span, 'drupal'); + $span->type = Type::WEB_SERVLET; + $span->meta[Tag::COMPONENT] = DrupalIntegration::NAME; - if (isset($info['function'])) { - $span->meta['drupal.render.theme_function'] = $info['function']; - } + /** @var \Drupal\Core\Theme\ThemeManager $this */ + $activeTheme = $this->getActiveTheme(); + $themeName = $activeTheme->getName(); + $themeEngine = $activeTheme->getEngine(); - if (isset($info['path'])) { - // The template can be from a different theme than the active one - // Format: '.../themes//...' - $path = $info['path']; - $themePathStart = strpos($path, '/themes/'); - if ($themePathStart !== false) { - $themePath = substr($path, $themePathStart + 8); // Between '/themes/', 8 = strlen('/themes/') - $themePath = substr($themePath, 0, strpos($themePath, '/')); // Until the next '/' - $span->meta['drupal.template.theme'] = $themePath; - } - } - } else { - $span->meta['drupal.render.hook'] = $span->resource = $originalHook; - } - } + if (!empty($themeName)) { + $span->meta['drupal.render.theme'] = $themeName; } - ] - ); - - // Drupal <= 11.2 renders through the {engine}_render_template() global; unlike the tracing - // posthook, this end hook also runs for a dropped or span-limited render. - // Must follow the trace_method: begin hooks run in install order, so active_span() is our own span. - install_hook( - 'Drupal\Core\Theme\ThemeManager::render', - function (HookData $hook) use (&$renderSpan, $tagTemplateFile) { - $enclosing = $renderSpan; - // Past the span limit this frame gets no span of its own, so claim nothing. - $span = \dd_trace_tracer_is_limited() ? null : active_span(); - $renderSpan = ($span && $span->name === 'drupal.theme.render') ? $span : null; - $hook->data = [$enclosing, null]; + if (empty($themeEngine)) { + return; + } + $span->meta['drupal.render.engine'] = $themeEngine; - /** @var \Drupal\Core\Theme\ThemeManager $this */ - $themeEngine = $this->getActiveTheme()->getEngine(); - if (empty($themeEngine) || !\function_exists("{$themeEngine}_render_template")) { + // Drupal <= 11.2 renders through the {engine}_render_template() global. + if (!\function_exists("{$themeEngine}_render_template")) { return; } - // Take the legacy branch only when no engine service resolves, as core does. + // Take that branch only when no engine service resolves, as core does. if (\property_exists($this, 'themeEngines') && $this->themeEngines->has($themeEngine)) { return; } - $hook->data = [$enclosing, install_hook( + $hookData->data = [$enclosing, install_hook( "{$themeEngine}_render_template", static function (HookData $renderHook) use ($tagTemplateFile) { $tagTemplateFile($renderHook->args[0]); @@ -381,11 +297,83 @@ static function (HookData $renderHook) use ($tagTemplateFile) { } )]; }, - static function (HookData $hook) use (&$renderSpan) { - $renderSpan = $hook->data[0]; + function (HookData $hookData) use (&$renderSpan) { + $renderSpan = $hookData->data[0]; // render() can return without calling the render function, so self-removal is not enough. - if (!empty($hook->data[1])) { - remove_hook($hook->data[1]); + if (!empty($hookData->data[1])) { + remove_hook($hookData->data[1]); + } + + /** @var null|\Drupal\Core\Theme\Registry $themeRegistry */ + $themeRegistry = ObjectKVStore::get($this, 'theme_registry'); + if (!$themeRegistry) { + return; + } + + $span = $hookData->span(); + $runtimeThemeRegistry = $themeRegistry->getRuntime(); + $hook = $hookData->args[0]; + + if (is_array($hook)) { + foreach ($hook as $candidate) { + if ($runtimeThemeRegistry->has($candidate)) { + break; + } + } + $hook = $candidate; + } + + $originalHook = $hook; + + if (!$runtimeThemeRegistry->has($hook)) { + // Iteratively strip everything after the last '__' delimiter, until an + // implementation is found + while ($pos = strrpos($hook, '__')) { + $hook = substr($hook, 0, $pos); + if ($runtimeThemeRegistry->has($hook)) { + break; + } + } + } + + if (!$runtimeThemeRegistry->has($hook)) { + $span->meta['drupal.render.hook'] = $span->resource = $originalHook; + return; + } + + $span->meta['drupal.render.hook'] = $span->resource = $hook; + $info = $runtimeThemeRegistry->get($hook); + + if (isset($info['base hook'])) { + $span->meta['drupal.render.base_hook'] = $info['base hook']; + } + + if (isset($info['type'])) { + $span->meta['drupal.render.type'] = $info['type']; + } + + if (isset($info['render element'])) { + $span->meta['drupal.render.element'] = $info['render element']; + } + + if (isset($info['template'])) { + $span->meta['drupal.template.template'] = $info['template']; + } + + if (isset($info['function'])) { + $span->meta['drupal.render.theme_function'] = $info['function']; + } + + if (isset($info['path'])) { + // The template can be from a different theme than the active one + // Format: '.../themes//...' + $path = $info['path']; + $themePathStart = strpos($path, '/themes/'); + if ($themePathStart !== false) { + $themePath = substr($path, $themePathStart + 8); // Between '/themes/', 8 = strlen('/themes/') + $themePath = substr($themePath, 0, strpos($themePath, '/')); // Until the next '/' + $span->meta['drupal.template.theme'] = $themePath; + } } } ); diff --git a/tests/ext/integrations/drupal/drupal_integration.inc b/tests/ext/integrations/drupal/drupal_integration.inc index acc37175613..e065e051666 100644 --- a/tests/ext/integrations/drupal/drupal_integration.inc +++ b/tests/ext/integrations/drupal/drupal_integration.inc @@ -40,7 +40,7 @@ function dd_drupal_render_report(array $spans, array $probeTargets) } $file = isset($span['meta']['drupal.template.file']) ? $span['meta']['drupal.template.file'] : ''; $tags[$file] = (isset($tags[$file]) ? $tags[$file] : 0) + 1; - // Also reported so that a prehook exception swallowed by the sandbox cannot pass silently. + // Also reported so that a begin hook exception swallowed by the sandbox cannot pass silently. $engine = isset($span['meta']['drupal.render.engine']) ? $span['meta']['drupal.render.engine'] : ''; $engines[$engine] = (isset($engines[$engine]) ? $engines[$engine] : 0) + 1; } diff --git a/tests/ext/integrations/drupal/theme_engine_service_dropped_nested_span.phpt b/tests/ext/integrations/drupal/theme_engine_service_dropped_nested_span.phpt index 2589f5623f9..f4024575b61 100644 --- a/tests/ext/integrations/drupal/theme_engine_service_dropped_nested_span.phpt +++ b/tests/ext/integrations/drupal/theme_engine_service_dropped_nested_span.phpt @@ -74,7 +74,8 @@ namespace Drupal\Core\Theme public function render($hook, array $variables = []) { if ($hook === 'block') { - // Dropped after the prehook ran (so not span-limited); active_span() is the OUTER render. + // Dropped after the begin hook ran, so the interface hook still fires, against a + // dropped span rather than the surviving outer one. \DDTrace\try_drop_span(\DDTrace\active_span()); } @@ -119,8 +120,7 @@ namespace dd_drupal_render_report($spans, ['Drupal\Core\Template\TwigThemeEngine::renderTemplate']); } ?> ---EXPECTF-- -[ddtrace] [error] [%d] Cannot run tracing closure for render(); spans out of sync; This message is only displayed once. Specify DD_TRACE_ONCE_LOGS=0 to show all messages. +--EXPECT-- surviving render span template.file = core/themes/claro/templates/page.html.twig drupal.render.engine[twig] = 1 drupal.template.file[core/themes/claro/templates/page.html.twig] = 1 diff --git a/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt b/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt index 687218aa0a6..6003425f772 100644 --- a/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt +++ b/tests/ext/integrations/drupal/theme_engine_service_span_limit.phpt @@ -94,14 +94,16 @@ namespace return $themeManager->render('block'); }; - // install_hook's callback is not span-limit gated, but trace_method is: with room for one span - // the nested render fires the hook while active_span() is the outer's. init() forces >= 1500. + // Room for exactly one more span: the outer render gets a real one, the nested render only a + // dummy that is never pushed, so it must not reach the outer's tag. init() forces >= 1500. ini_set('datadog.trace.spans_limit', 2); DDTrace\start_span(); DDTrace\close_span(); - $themeManager->render('page'); + $rendered = $themeManager->render('page'); + // Proves the nested render really ran, so the tag assertion below cannot pass vacuously. + echo "nested rendered: ", var_export(strpos($rendered, 'block.html.twig') !== false, true), "\n"; echo "limited: ", var_export((bool) dd_trace_tracer_is_limited(), true), "\n"; dd_drupal_render_report(dd_trace_serialize_closed_spans(), [ 'Drupal\Core\Template\TwigThemeEngine::renderTemplate', @@ -109,7 +111,8 @@ namespace } ?> --EXPECT-- +nested rendered: true limited: true drupal.render.engine[twig] = 1 -drupal.template.file[] = 1 +drupal.template.file[core/themes/claro/templates/page.html.twig] = 1 hook budget left [Drupal\Core\Template\TwigThemeEngine::renderTemplate]: yes diff --git a/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt b/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt index 4e498e97366..21323d191d0 100644 --- a/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt +++ b/tests/ext/integrations/drupal/theme_render_dropped_nested_span.phpt @@ -43,7 +43,8 @@ namespace Drupal\Core\Theme public function render($hook, array $variables = []) { if ($hook === 'block') { - // Dropped after the prehook ran, so the hook is installed and active_span() is the OUTER render. + // Dropped after the begin hook ran, so this frame's render hook is installed and + // fires against a dropped span rather than the surviving outer one. \DDTrace\try_drop_span(\DDTrace\active_span()); } @@ -91,8 +92,7 @@ namespace dd_drupal_render_report($spans, ['twig_render_template']); } ?> ---EXPECTF-- -[ddtrace] [error] [%d] Cannot run tracing closure for render(); spans out of sync; This message is only displayed once. Specify DD_TRACE_ONCE_LOGS=0 to show all messages. +--EXPECT-- surviving render span template.file = core/themes/olivero/templates/page.html.twig drupal.render.engine[twig] = 1 drupal.template.file[core/themes/olivero/templates/page.html.twig] = 1 diff --git a/tests/ext/integrations/drupal/theme_render_dropped_span.phpt b/tests/ext/integrations/drupal/theme_render_dropped_span.phpt index e86cc36e96f..6a0da868005 100644 --- a/tests/ext/integrations/drupal/theme_render_dropped_span.phpt +++ b/tests/ext/integrations/drupal/theme_render_dropped_span.phpt @@ -43,8 +43,8 @@ namespace Drupal\Core\Theme public function render($hook, array $variables = []) { if ($hook === 'dropped') { - // A sampling filter or another integration can drop the render span; the tracing - // posthook is then skipped, so it cannot be what removes the hook. + // A sampling filter or another integration can drop the render span; the end hook + // must still run and remove the render hook. \DDTrace\try_drop_span(\DDTrace\active_span()); return false; } @@ -66,7 +66,7 @@ namespace DDTrace\Integrations\Drupal\DrupalIntegration::init(); // The render spans must not be root spans: dropping a root span rejects the trace instead - // of marking the span dropped, which would leave the posthook running. + // of marking the span dropped. $root = DDTrace\start_span(); $root->name = 'test.root'; @@ -92,8 +92,7 @@ namespace dd_drupal_render_report($spans, ['twig_render_template']); } ?> ---EXPECTF-- -[ddtrace] [error] [%d] Cannot run tracing closure for render(); spans out of sync; This message is only displayed once. Specify DD_TRACE_ONCE_LOGS=0 to show all messages. +--EXPECT-- span[drupal.theme.render] = 1 span[test.root] = 1 drupal.render.engine[twig] = 1 diff --git a/tests/ext/integrations/drupal/theme_render_span_limit.phpt b/tests/ext/integrations/drupal/theme_render_span_limit.phpt index b39c1c42a04..dae38375260 100644 --- a/tests/ext/integrations/drupal/theme_render_span_limit.phpt +++ b/tests/ext/integrations/drupal/theme_render_span_limit.phpt @@ -66,22 +66,25 @@ namespace $themeManager = new Drupal\Core\Theme\ThemeManager(); - // The legacy hook is installed from an install_hook callback, which is not span-limit gated, - // so the nested render fires it while active_span() is the outer's. init() forces >= 1500. + // Room for exactly one more span: the outer render gets a real one, the nested render only a + // dummy that is never pushed, so it must not reach the outer's tag. init() forces >= 1500. ini_set('datadog.trace.spans_limit', 2); DDTrace\start_span(); DDTrace\close_span(); - $themeManager->render('page', ['nested' => function () use ($themeManager) { + $rendered = $themeManager->render('page', ['nested' => function () use ($themeManager) { return $themeManager->render('block'); }]); + // Proves the nested render really ran, so the tag assertion below cannot pass vacuously. + echo "nested rendered: ", var_export(strpos($rendered, 'block.html.twig') !== false, true), "\n"; echo "limited: ", var_export((bool) dd_trace_tracer_is_limited(), true), "\n"; dd_drupal_render_report(dd_trace_serialize_closed_spans(), ['twig_render_template']); } ?> --EXPECT-- +nested rendered: true limited: true drupal.render.engine[twig] = 1 -drupal.template.file[] = 1 +drupal.template.file[core/themes/olivero/templates/page.html.twig] = 1 hook budget left [twig_render_template]: yes From 730f721565bedd30883747e63a5567cc050ef454 Mon Sep 17 00:00:00 2001 From: Alexandre Rulleau Date: Mon, 31 Aug 2026 14:12:16 +0200 Subject: [PATCH 7/7] perf(drupal): install the legacy render hook once per engine The trace_method to install_hook conversion lost trace_method's cheap bail: install_hook runs both callbacks past the span limit, where span() only hands out a dummy. Over 200 renders past a 2-span limit that turned 1 getActiveTheme() call into 200 and 2 runtime registry lookups into 400. The begin callback now returns early when the tracer is limited, clearing $renderSpan so a limited frame's template cannot leak onto the enclosing render's span; the end callback reads the frame span back from $renderSpan and skips its work when the frame bailed out. The {engine}_render_template hook was also installed and self-removed on every render. Now that $renderSpan owns the attribution, one hook per engine name for the whole request is enough, so the per-render install/remove pair, the [$enclosing, $hookId] tuple and the themeEngines discriminator are gone. Seeding twig_render_template at init() closes two gaps: core falls back to twig's render function when the active engine has no {engine}_render_template() of its own, and the deprecated global does not delegate to the 11.3+ theme engine service. Dropping the themeEngines discriminator also fixes a mismatch with core, which resolves the engine of the template's own extension type rather than the active theme's. --- .../Integrations/Drupal/DrupalIntegration.php | 53 +++++++------ .../drupal/theme_render_dropped_span.phpt | 6 +- .../drupal/theme_render_early_return.phpt | 2 +- .../drupal/theme_render_engine_fallback.phpt | 79 +++++++++++++++++++ 4 files changed, 112 insertions(+), 28 deletions(-) create mode 100644 tests/ext/integrations/drupal/theme_render_engine_fallback.phpt diff --git a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php index 2e088375d5d..358e09bb24d 100644 --- a/src/DDTrace/Integrations/Drupal/DrupalIntegration.php +++ b/src/DDTrace/Integrations/Drupal/DrupalIntegration.php @@ -231,6 +231,7 @@ static function ($registry, $scope, $args) { // The span of the executing ThemeManager::render frame. The render function is hooked in its // own frame, so it cannot reach that span on its own. $renderSpan = null; + $legacyRenderHooks = []; $tagTemplateFile = static function ($file) use (&$renderSpan) { if ($renderSpan) { @@ -238,6 +239,13 @@ static function ($registry, $scope, $args) { } }; + $tagLegacyRender = static function (HookData $renderHook) use ($tagTemplateFile) { + $tagTemplateFile($renderHook->args[0]); + }; + // Twig is both the default engine and core's fallback when {engine}_render_template() is + // missing, and the deprecated global does not delegate to the 11.3+ service. + $legacyRenderHooks['twig'] = install_hook('twig_render_template', $tagLegacyRender); + // Drupal 11.3+ renders through a theme_engine service instead of {engine}_render_template(). install_hook( 'Drupal\Core\Theme\ThemeEngineInterface::renderTemplate', @@ -253,11 +261,18 @@ static function (HookData $hook) use ($tagTemplateFile) { install_hook( 'Drupal\Core\Theme\ThemeManager::render', - function (HookData $hookData) use (&$renderSpan, $tagTemplateFile) { + function (HookData $hookData) use (&$renderSpan, &$legacyRenderHooks, $tagLegacyRender) { + // install_hook, unlike trace_method, still runs both callbacks past the span limit; + // clearing $renderSpan keeps this frame's template off the enclosing render's span. + if (\dd_trace_tracer_is_limited()) { + $hookData->data = $renderSpan; + $renderSpan = null; + return; + } + $span = $hookData->span(); - $enclosing = $renderSpan; + $hookData->data = $renderSpan; $renderSpan = $span; - $hookData->data = [$enclosing, null]; $span->name = 'drupal.theme.render'; $span->service = \ddtrace_config_app_name('drupal'); @@ -279,29 +294,20 @@ function (HookData $hookData) use (&$renderSpan, $tagTemplateFile) { } $span->meta['drupal.render.engine'] = $themeEngine; - // Drupal <= 11.2 renders through the {engine}_render_template() global. - if (!\function_exists("{$themeEngine}_render_template")) { - return; + // Drupal <= 11.2 renders through the {engine}_render_template() global. One hook per + // engine name for the whole request; $renderSpan does the attribution. + if (!isset($legacyRenderHooks[$themeEngine])) { + $legacyRenderHooks[$themeEngine] = + install_hook("{$themeEngine}_render_template", $tagLegacyRender); } - // Take that branch only when no engine service resolves, as core does. - if (\property_exists($this, 'themeEngines') && $this->themeEngines->has($themeEngine)) { - return; - } - - $hookData->data = [$enclosing, install_hook( - "{$themeEngine}_render_template", - static function (HookData $renderHook) use ($tagTemplateFile) { - $tagTemplateFile($renderHook->args[0]); - // Self-removal keeps outer/inner render pairing correct. - remove_hook($renderHook->id); - } - )]; }, function (HookData $hookData) use (&$renderSpan) { - $renderSpan = $hookData->data[0]; - // render() can return without calling the render function, so self-removal is not enough. - if (!empty($hookData->data[1])) { - remove_hook($hookData->data[1]); + // $renderSpan is this frame's span, or null when the begin callback bailed out + // past the span limit. $hookData->data holds the enclosing render's span. + $span = $renderSpan; + $renderSpan = $hookData->data; + if (!$span) { + return; } /** @var null|\Drupal\Core\Theme\Registry $themeRegistry */ @@ -310,7 +316,6 @@ function (HookData $hookData) use (&$renderSpan) { return; } - $span = $hookData->span(); $runtimeThemeRegistry = $themeRegistry->getRuntime(); $hook = $hookData->args[0]; diff --git a/tests/ext/integrations/drupal/theme_render_dropped_span.phpt b/tests/ext/integrations/drupal/theme_render_dropped_span.phpt index 6a0da868005..a2c4a53587b 100644 --- a/tests/ext/integrations/drupal/theme_render_dropped_span.phpt +++ b/tests/ext/integrations/drupal/theme_render_dropped_span.phpt @@ -1,5 +1,5 @@ --TEST-- -Drupal render hook is removed when the render span is dropped (APMS-20395) +Drupal render tagging survives a dropped render span (APMS-20395) --SKIPIF-- render('dropped'); } - // A leak above exhausts the budget, so this render can no longer tag its own template. + // Were a hook leaked above, the budget would be exhausted and this render left untagged. $themeManager->render('page'); DDTrace\close_span(); diff --git a/tests/ext/integrations/drupal/theme_render_early_return.phpt b/tests/ext/integrations/drupal/theme_render_early_return.phpt index f2005302f1f..0812c00357a 100644 --- a/tests/ext/integrations/drupal/theme_render_early_return.phpt +++ b/tests/ext/integrations/drupal/theme_render_early_return.phpt @@ -1,5 +1,5 @@ --TEST-- -Drupal render hooks are removed when the render function is never called (APMS-20395) +Drupal renders that never call the render function do not mis-tag later ones (APMS-20395) --SKIPIF-- +--ENV-- +DD_TRACE_AUTO_FLUSH_ENABLED=0 +DD_TRACE_GENERATE_ROOT_SPAN=0 +DD_CODE_ORIGIN_FOR_SPANS_ENABLED=0 +DD_TRACE_LOG_LEVEL=warn,startup=off +--INI-- +datadog.trace.hook_limit=20 +--FILE-- +render('fallback'); + } + + dd_drupal_render_report(dd_trace_serialize_closed_spans(), [ + 'twig_render_template', + 'phptemplate_render_template', + ]); +} +?> +--EXPECT-- +drupal.render.engine[phptemplate] = 30 +drupal.template.file[core/themes/claro/templates/fallback.html.twig] = 30 +hook budget left [twig_render_template]: yes +hook budget left [phptemplate_render_template]: yes