fix(drupal): hook the theme engine service and guarantee render hook removal - #4145
fix(drupal): hook the theme engine service and guarantee render hook removal#4145Leiyks wants to merge 2 commits into
Conversation
…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
|
Benchmarks [ tracer ]Benchmark execution time: 2026-08-28 15:47:19 Comparing candidate commit b4c5969 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 192 metrics, 1 unstable metrics.
|
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
| 'prehook' => function (SpanData $span, $args) { | ||
| '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); |
There was a problem hiding this comment.
spl_object_hash is deprecated since PHP 8.6. You can simply use spl_object_id.
You might want to simply use install_hook() to transfer the hook id via $hook->data from begin to end.
2 participants
Description
Fixes APMS-20395.
On Drupal >= 11.3,
drupal.template.fileis never set on anydrupal.theme.renderspan, and hooks accumulate ontwig_render_templateuntilDD_TRACE_HOOK_LIMITis reached. A customer measured the tag present on 0 of 61,160 render spans over 2 days across 7 services — including services that never reached the hook limit.Root cause
DrupalIntegrationinstalls a hook on{$engine}_render_templatefrom inside the prehook oftrace_method('Drupal\Core\Theme\ThemeManager', 'render', ['recurse' => true])— once per render, nested renders included — and the onlyremove_hooklives inside that hook's own callback. The now-deleted comment stated the invariant it relied on:That invariant broke in Drupal 11.3.0.
ThemeManager::render()now prefers atheme_engine-tagged service (ThemeManager.php:377-380@ 11.4.4):while
ThemeInitialization.php:157-158still unconditionallyinclude_oncestwig.engine, which still defines the deprecatedtwig_render_template(). Sofunction_exists()returns true, the hook installs on every render, the function is never called, the hook is never removed, and the tag is never set.function_exists()is useless as a version discriminator here — that is the bug.Verified boundary (md5 / HTTP probes of upstream tags):
ThemeManager.phpis byte-identical 11.3.0 ↔ 11.4.4;TwigThemeEngine.php404s through 11.2.14 and appears at 11.3.0. Deprecated in drupal:11.3.0, removed from drupal:12.0.0.twig_render_template()definedmain)twig.enginedeleted)A second defect, affecting every Drupal version
ThemeManager::render()has an earlyreturn FALSEat 11.4.4:178 (theme hook not found), where the render function is never reached and today's hook is never removed. This is not merely a leak — the leaked hooks fire on later renders and retroactively mis-tag unrelated spans. From the new regression test on master:19 stale hooks re-tagged 19 unrelated spans. This affects Drupal <= 10.1 too, i.e. versions CI already covers.
Changes
Install one hook at
init()onDrupal\Core\Theme\ThemeEngineInterface::renderTemplate. Interface-targeted, so it covers Twig, contrib engines, and the$info['type'] == 'module'case (where core forces Twig) in a single install. Installed once, it cannot accumulate. On Drupal <= 11.2 the interface does not exist and the hook simply never resolves.Core passes the template path without its extension on the service path (
$extension = '';TwigThemeEngine::renderTemplateappendsself::EXTENSIONitself), so.html.twigis re-appended for Twig to keep the tag value byte-identical to what Drupal <= 10 emitted. On Drupal 12 the$extensionvariable is gone upstream entirely, so this stays correct.Take the legacy branch only when core itself would — when no engine service resolves — instead of probing
function_exists().Remove the legacy hook unconditionally in the posthook. Self-removal inside the callback is kept, because it is load-bearing for correct outer/inner render pairing, but it can no longer be the only removal path.
Ids are held in a map keyed by
spl_object_hash($span)rather than a positional stack: the posthook is skipped for a dropped span (tracer/hook/uhook_legacy.c:226) while the prehook still runs, which would permanently desync a stack. The prehook resets the entry before use, so a recycled object handle cannot resurface a stale id; the map is bounded (measured: 21 entries at both 60 and 400 dropped renders).themeEngines->has()is used rather thangetThemeEngine(), because the latter's->get()instantiates the engine service — including on the early-return path where core never resolves it — and adds a throw surface to a hot prehook.has()is side-effect-free; core itself guardsget()behind it.