[6.x] Include Tag - #15182
Conversation
|
Looking forward to using this! After a review and playing around, got a couple things I want to make sure are intentional: Blade → Antlers cascade — If I do scope punching out — Using
|
Refactored a few things so the
Added a test to codify it. It's a neat use of existing behaviors and I don't see the harm in it; it'd be a lot more work to shut this down and its kind of cool!
The existing behavior is correct IMO. The @if (Statamic::tag('include:exists')->src('cards/author')->fetch())
@endif
@if (Statamic::tag('partial:exists')->src('cards/author')->fetch())
@endif{{ if {include:exists src="cards/author"} }} ... {{ /if }}
{{ if {partial:exists src="cards/author"} }} ... {{ /if }}
|
|
Awesome, thanks man! |
jasonvarga
left a comment
There was a problem hiding this comment.
Really nice piece of work — the scoping model is the right call, the try/finally discipline around cascade and prefix state is careful, and the test suite (9 files, ~1,680 lines, one test per closed issue plus a dedicated SandboxTest for scope-escape attempts) is unusually thorough. The randomized nowdoc terminator is a good catch on its own: slot content containing a literal COMPILED; line would previously have broken out of the hoisted heredoc.
A couple of things need fixing before this ships, since once the behaviour is out it becomes BC. Details are on the relevant lines; the rest are notes below.
Notes (not line-specific)
Missing Antlers coverage for include:exists / include:if_exists. The PR description documents <s-include:if_exists src="cards/{type}" />, and RendersViews::exists() / ifExists() are new code paths for IncludeTag. Blade covers them (IncludeCompilerTest::it_forwards_exists_method_calls and it_forwards_if_exists_method_calls), but there's nothing equivalent under tests/Antlers/Runtime/Includes/. Same gap in the other direction for the src form — Antlers has test_it_renders_a_view_using_the_src_form, Blade doesn't test <s:include src="..." />.
New trait members are private in an otherwise protected trait. CompilesPartials was entirely protected; the six new members (compileSlotOutput, compileIncludeSlot, rawSlotName, isValidSlotName, compileInclude, compileViewTag) are private. compilePartial() stays protected and delegates, so the existing extension point survives — consistency nit only.
GlobalRuntimeState::captureRuntimeState() going from 3 to 4 elements is handled safely. restoreState() uses $capturedState[3] ?? true, and list-destructuring in third-party code ignores the extra element. Just flagging that I checked.
| throw new RuntimeException('The [params] parameter on the include tag must be an associative array.'); | ||
| } | ||
|
|
||
| return Arr::except($spread, self::CONTROL); |
There was a problem hiding this comment.
Arr::except($spread, self::CONTROL) silently drops any spread key named src, when, unless, cascade, params, or handle_prefix. <s-include:card :params="entry" /> where the entry has a field called src (plausible for image/video/embed fields) loses it with no exception and no warning — the variable is just absent in the view. Compare RESERVED, which correctly throws (test_reserved_params_cannot_be_spread).
None of the control params are ever read from the spread: shouldRender() reads $this->params, cascade comes from $this->params->bool('cascade'), handle_prefix from $parameters['handle_prefix'], and src from wildcard(). So this except() isn't protecting anything — it only loses data.
It's also inconsistent with the prefix aliasing right below it. With handle_prefix="hero_", a spread key hero_src becomes src in unprefixedAliases() and is kept, while a directly-passed src is dropped.
Suggest dropping the except() entirely, or throwing the way RESERVED does so the loss is at least visible.
| $val = $val->get()->all(); | ||
| } | ||
|
|
||
| if ($val instanceof Slot) { |
There was a problem hiding this comment.
This continue jumps past the modifier-handling block that starts immediately below at $executedParamModifiers = false;, so modifiers on a slot are never applied.
Worse, modifiers live in $node->parameters (see the ModifierManager::isModifier($param) loop a few lines down at ~2186), so for {{ slot | upper }}:
$node->hasParametersistrue, sogetSlotOutputProps()evaluatesupperas a parameter and passes it into the slot's render data as a variable;- the
continuemeansupperis never applied to the output.
Net result is silently wrong output plus a junk upper variable in the slot scope. Same for {{ slot:footer | markdown }}, | trim, and so on.
getSlotOutputProps() should filter out params where ModifierManager::isModifier() is true and let the modifier chain run on the rendered string — or slot nodes should reject modifiers loudly rather than absorbing them as props.
I traced this by reading rather than executing, so if the parser classifies modifier params differently here I'd like to be corrected — but either way a test for {{ slot | upper }} is worth adding.
| $context = '$'.IncludeTag::CONTEXT_KEY.' ?? false'; | ||
| $output = '\Statamic\View\Slot::output('.$slot.', '.$this->compileParameters($component->parameters).')'; | ||
|
|
||
| return '<?php if ('.$context.') { echo '.$output.'; } else { ?>'.$this->compileComponent($component).'<?php } ?>'; |
There was a problem hiding this comment.
The include branch emits only Slot::output(...) and discards the component's inner content, so a paired <s:slot:footer>Default</s:slot:footer> in an included Blade view renders nothing when the slot isn't supplied — Slot::output(null, []) returns '' — rather than falling back to the body.
Antlers has a working idiom for this ({{ if slot:header }}…{{ else }}Default{{ /if }}, covered by SlotsTest::test_a_named_slot_falls_back_to_the_views_default_when_not_provided). Blade users can reach for @if(isset($footer)) via the named-slot alias, but only when canAliasSlot() permits it — if the slot name collides with a data key or is one of PROTECTED_ALIASES (app, errors, params, view), there's no supported way to test for the slot, since $__statamic_include_slots is internal.
Either way, no test covers a paired <s:slot> inside an included Blade view. Worth deciding whether pair bodies are fallbacks or are intentionally ignored, and pinning it with a test.
| $this->stopMeasuringTag(); | ||
|
|
||
| if ($suspendedData != null) { | ||
| if ($capturedRuntimeState !== null) { |
There was a problem hiding this comment.
Worth calling out in the release notes: this guard change applies to every isolated tag, not just include. Previously an isolated tag invoked with empty data never reached GlobalRuntimeState::restoreState(), leaking requiresRuntimeIsolation and isCascadeEnabled.
The fix is correct and CascadeTest::test_any_isolated_tag_restores_the_previous_cascade_state covers it, but third-party isolated tags that (knowingly or not) relied on the cascade being force-re-enabled after they ran will now see the caller's state instead. No change requested — just shouldn't ship silently.
|
|
||
| $existingView = $this->view; | ||
|
|
||
| $suspendedData = $this->nodeProcessor->getAllData(); |
There was a problem hiding this comment.
Same category as the NodeProcessor guard change: parseView() is the shared view-render path, so snapshotting and restoring the node processor's data here affects partial too, not just include.
The full suite is green so nothing regressed, but the PR description says partial semantics are deliberately unchanged — worth a second look to confirm that holds here, since this is exactly the mechanism behind the #8175-style leak.
| return $this->render(); | ||
| } | ||
|
|
||
| public function __serialize(): array |
There was a problem hiding this comment.
This renders eagerly with no props on serialize. For a scoped slot — one the view feeds data back into, e.g. {{ slot:row :cell="value" }} — serialization into a nocache fragment or the static cache bakes in a props-less render.
InteropTest::test_the_cache_tag_works_around_and_inside_an_include_with_slots covers the cache tag, and there isn't much alternative once a closure has to cross a serialize boundary. Mostly flagging that it degrades quietly rather than loudly — a scoped slot that ends up serialized will render wrong with no signal.
Fixes #8175
Fixes #10703
Fixes #11486
Fixes #12709
Overview
This PR adds a new
includetag: a strictly-scoped alternative topartialfor rendering another view, available in both Antlers and Blade.The
partialtag automatically shares every variable from the template using it with the partial being rendered. That convenience is the root cause of a long line of historical scoping issues and little paper-cuts. Variables set inside a partial leaking back out (but only sometimes), parameters and front matter showing up in other partials rendered later on the page, and behavior changing depending on which syntax was used.Fixing these issues with the existing
partialtag would absolutely break a ton of sites, soincludeis here!How it differs from
partialpartialincludecascade="true"Passing data
You must pass data to the include tag explicitly. Parameters become variables inside the view, and you can spread an entire array using
:params. Inside the view, useparamsto check what was passed in:Use
handle_prefixto make prefixed keys likehero_titleavailable as bothhero_titleandtitle:Slots
Content between the tag pair becomes the default slot, and you can define named slots with
slot:namepairs.Slots only render when the view actually uses them, and the view can pass data back to your slot content. A view can render a slot once per item in a loop, for example:
You can also forward a slot you received on to another include:
The Cascade
Included views don't see the Cascade by default. Pass
cascade="true"when you want it:Conditionals and existence
Use
whenandunlessto control whether anything renders.existsandif_existswork the same way they do onpartial:The issues
includethey never do.view:data of the next one rendered on the page. Each include only sees its own.{{ if }}and using the?=shorthand isolate variables differently. Both behave the same withinclude.Notes for reviewers
handle_prefix, the prefix rewrites variable lookups in everything rendered inside it. The include tag suspends this while it renders, so an enclosing partial's prefix never reaches the included view. Components rendered inside such a partial still inherit the prefix. That is probably unintentional, but changing it could break existing sites, so it's left alone for now.