fix(drupal): hook the theme engine service and guarantee render hook removal - #4145
Open
Leiyks wants to merge 7 commits into
Open
fix(drupal): hook the theme engine service and guarantee render hook removal#4145Leiyks wants to merge 7 commits into
Leiyks wants to merge 7 commits into
Conversation
|
Benchmarks [ tracer ]Benchmark execution time: 2026-08-31 13:30:50 Comparing candidate commit 730f721 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 192 metrics, 1 unstable metrics.
|
bwoebi
reviewed
Aug 28, 2026
…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
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
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
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 4e02a93 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
Leiyks
force-pushed
the
leiyks/fix-apms-20395
branch
from
August 31, 2026 10:29
b4c5969 to
5acdc24
Compare
bwoebi
reviewed
Aug 31, 2026
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes APMS-20395.
On Drupal >= 11.3,
drupal.template.filewas never set on anydrupal.theme.renderspan, and hooks accumulated ontwig_render_templateuntilDD_TRACE_HOOK_LIMIT. A customer measured the tag present on 0 of 61,160 render spans.Root cause. Drupal 11.3.0 switched
ThemeManager::render()to a theme-engine service ([$engine, 'renderTemplate']), falling back to the global{engine}_render_template()only when no service is registered. Core ships a Twig service, so the global is never called — but it is still defined, becauseThemeInitializationunconditionally includestwig.engine. Sofunction_exists()returned true, the integration installed a hook on every render, the hook never fired, and it was never removed.A second defect, on every Drupal version:
render()can return early (theme hook not found, exception) without calling the render function. Those hooks leaked and then fired on later renders, overwriting unrelated spans' tags — measured 19 spans mis-tagged in a 30-render test.Changes
ThemeEngineInterface::renderTemplateonce atinit()— covers Twig, contrib engines, and module templates in one install. The service path passes the template path without its extension, so.html.twigis re-appended for Twig to keep the tag value identical to pre-11.3.{engine}_render_templatehook once per engine instead of once per render, seeded ontwig_render_templateatinit(). This also fixes a pre-existing gap: core falls back totwig_render_templatewhen{engine}_render_templateis absent, which was previously untagged.drupal.theme.renderspan frominstall_hook+$hook->span()rather thantrace_method.$hook->span()is per-invocation, so the frame's own span is known directly — noactive_span()lookup and no guessing which frame a tag belongs to.dd_trace_tracer_is_limited(). Unliketrace_method,install_hookcallbacks are not limit-gated, so without this the whole body ran on every render past the span limit (measured 200x on a 200-render probe).Reviewer notes
allowNestedHook()is not needed: it escapes reentrancy initiated from inside a callback, not the hooked function recursing.install_hookholdsrunningonly across the callback, so it is already recursive — measured 7/7 begins on a nested render chain, versus 1/1 fortrace_methodwithrecurse => false.