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 1 commit 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-27 15:27:14 Comparing candidate commit 6ca1f71 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 192 metrics, 1 unstable metrics.
|
1 participant
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.Notes for reviewers
install_hook's begin callback is not gated by the span limit (dd_uhook_begin,uhook.c:311-324) whiletrace_methodis (uhook_legacy.c:102). Withoutdd_trace_tracer_is_limited()in the guard, a nested render past the limit has no span of its own,active_span()returns the still-open outer render span, and the outer span gets tagged with the nested template — turning "tag missing" into "tag confidently wrong". This was caught in review and is covered bytheme_engine_service_span_limit.phpt.isset($span->meta['drupal.template.file'])(first-write-wins) was tried and rejected: it breaks the preprocess ordering, where core renders sub-elements atThemeManager.php:366before the outer template at:428. Correctness depends on the render frame, not write order. An exact fix via depth tracking was deferred — it roughly doubles per-render hook invocations on a 100+/request path.renderTemplateordering. In the preprocess ordering the unguarded code was already correct, so the guard trades a small precision loss there (a correct value becomes<missing>at the limit boundary) for never being wrong.theme_render_early_return.phptthe "hook budget left" probe does not discriminate — master also printsyes, because leaked hooks self-remove once they finally fire. What catches that defect is the count skew above.install_hookper render and adds twospl_object_hashcalls, two array ops, and an O(1) membership test.Known limitations
drupal/core@11.4.4source. Adding aVersion_11_4fixture (~50k vendored files) is deferred.twig_render_template()does not delegate toTwigThemeEngine::renderTemplate; it duplicates the logic inline via\Drupal::service('twig'). Contrib calling it directly on 11.3+ is tagged by neither hook.{$engine}_render_templateonly and does not model core'stwig_render_templatefallback atThemeManager.php:384-386.Testing
Four
.phptundertests/ext/integrations/drupal/, chosen over the PHPUnit harness because the latter needs mysql plus a ~50k-file vendored fixture per version and only covers Drupal <= 10.1, so it cannot express the 11.3+ shape at all.theme_engine_service.phptclass_exists(..., false) === falsebefore use)theme_engine_service_span_limit.phpttheme_engine_service_preprocess.phptisset()approach cannot be reintroducedtheme_render_early_return.phptreturn FALSEpath and exception unwinding, version-independentEach defect test fails on the code it targets and passes with the fix. All four are also clean under valgrind (
Tests leaked: 0).Guards against false-green:
DD_TRACE_LOG_LEVEL=warn(the tracer sandbox otherwise swallows prehook exceptions — an early draft passed for exactly that reason),drupal.render.enginecount assertions, and hook-budget probes naming the actual target. The budget probe was validated by re-introducing the regression it is meant to catch and confirming the test goes red.Existing snapshots:
test_web_drupal_101andtest_web_drupal_95were run and A/B'd againstorigin/master— byte-identical results, withtestScenarioGetWithView(the 12 tagged render spans) passing in all runs.test_web_drupal_89was not run: the app does not boot on PHP 8.3 and CI runs it on 7.2–7.4 only.phpcson the changed file: same 6 pre-existing findings, zero new.