Add parent() to render a shadowed template - #95
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThis change adds shadowed-template inheritance through ChangesTemplate inheritance and resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant View
participant AbstractView
participant TemplateRegistry
View->>AbstractView: render template
AbstractView->>TemplateRegistry: getResolvedPath(name)
AbstractView->>TemplateRegistry: getNext(name, resolvedPath)
TemplateRegistry-->>AbstractView: next ResolvedTemplate
AbstractView->>AbstractView: captureTemplate(template, vars)
AbstractView-->>View: rendered parent output
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/AbstractView.php`:
- Around line 396-408: Update captureTemplate to record the initial
output-buffer and capture/section-frame depths before invoking the template,
then restore both stacks in the Throwable catch path. Ensure all buffers and the
stale $capture entry created after those initial depths are removed before
rethrowing, while preserving normal successful capture behavior.
In `@src/TemplateRegistry.php`:
- Around line 248-259: Normalize configured paths by trimming trailing directory
separators in the path assignment logic used by setPaths() and setNamespaces(),
ensuring the canonical values stored in $paths and namespace path lists match
getNext()’s afterPath lookup. Preserve root-path handling, and add a regression
test covering parent() when a configured path has a trailing separator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a32088b-0a26-4ebe-959b-65b9c4cbd389
📒 Files selected for processing (21)
CHANGELOG.mddocs/interfaces.mddocs/templates.mdsrc/AbstractView.phpsrc/ResolvedTemplate.phpsrc/SearchPathInterface.phpsrc/TemplateRegistry.phpsrc/View.phptests/FakeRegistryWithoutPaths.phptests/ParentTest.phptests/TemplateRegistryTest.phptests/fixtures/parent/app/boom.phptests/fixtures/parent/app/orphan.phptests/fixtures/parent/app/outer.phptests/fixtures/parent/app/read.phptests/fixtures/parent/app/vars.phptests/fixtures/parent/core/read.phptests/fixtures/parent/core/vars.phptests/fixtures/parent/module/read.phptests/fixtures/parent/ns-app/read.phptests/fixtures/parent/ns-core/read.php
Search paths are first-hit-wins, so a template in an earlier directory shadows one of the same name later in the list and the shadowed version is unreachable: find() returns the first hit and drops the rest of the chain. Changing one part of a package's template therefore meant copying the whole file, which then stops tracking the original. TemplateRegistry now records which directory satisfied each name, and exposes two primitives on SearchPathInterface: getResolvedPath(), which answers "which package's template won?", and getNext(), which resumes the search after a given directory. getNext() returns a ResolvedTemplate rather than a bare Closure. Walking a chain past the first step needs the path the parent itself was found in, and a closure does not carry that. AbstractView gains a render stack -- render() pushes (name, path) and pops in a finally -- so parent() resolves against the template actually executing when renders nest, and a throwing template leaves no frame behind. The output buffering render() did inline moves to captureTemplate() so parent() shares it, and so the blocks port can reuse the same stack rather than introducing a second one. parent() returns '' rather than throwing when there is nothing further to render: overriding a template that turns out to shadow nothing is a normal state during development, not an error.
prependPath() and appendPath() have always trimmed trailing directory separators; setPaths() and setNamespaces() stored whatever they were given, so one directory had two spellings inside the registry depending on which setter registered it. That is not merely untidy. The path recorded for a found template is handed straight back to getNext() to resume the search, and getNext() trims before comparing, so a directory registered with a trailing slash never matched its own entry in the path list. parent() then returned '' -- reporting "this template shadows nothing" for a template that plainly does, and going quiet instead of failing loudly. Normalizing at the setters keeps one spelling internally, which is what both the search and the resumption assume.
The docs listed the exception alongside the three empty-string cases, which reads as though a template author might hit it. They cannot: parent() is protected, so reaching it means a template is running, and a running template was rendered, and a render always has a frame. The distinction is that '' answers a well-formed question -- there is a current template and nothing follows it -- while the exception says there is no current template to resume from, which only happens off the normal render path. The case that justifies keeping it is a subclass that reimplements render() without maintaining the render stack. Without the throw, every parent() in the application would quietly return '' and every override would go back to replacing instead of extending: wrong output, nothing raised. Add the test for that case, since the docs now name it.
parent() returns '' when it finds nothing to resume into, which is right for production: an override written before the template it overrides exists should not take a page down. But the same '' is what a misconfiguration produces. A typo in a search path, paths registered in the wrong order, or a registry with no paths at all are indistinguishable from "this template legitimately shadows nothing" -- overrides quietly stop composing, the page still renders, and the only symptom is missing markup. Both bugs found in this branch had exactly that shape. setStrictParent(true) turns the three cases into Exception\ParentNotFound, naming the template and why the lookup came up empty. Off by default. It takes a bool rather than reading the environment. Aura.View has no config layer and no runtime dependencies, so deciding what "development" means belongs to whatever wires the View up.
The guard is still worth documenting -- strict parent mode covers a different exception, so nothing here became redundant -- but two thirds of the passage was a code block spelling out pushRender()/popRender()/captureTemplate(). That is protected API aimed at subclass authors, in a page written for template authors, and it has to be kept in step with View::render() or it goes stale and misleads. Point at the real method instead, and keep the one thing a subclass author cannot guess: overriding render() without the stack silently disables parent() application-wide.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/AbstractView.php (1)
353-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated bind-and-fallback logic between
getTemplate()andparent().
getTemplate()(Line 353-357) andparent()(Line 499) both do$tmpl->bindTo($this, static::class) ?? $tmpl. Consider extracting a small private helper (e.g.bindTemplate(\Closure $tmpl): \Closure) to keep the binding rule in one place.♻️ Proposed refactor
+ protected function bindTemplate(\Closure $tmpl): \Closure + { + return $tmpl->bindTo($this, static::class) ?? $tmpl; + } + protected function getTemplate(string $name): \Closure { $tmpl = $this->template_registry->get($name); - return $tmpl->bindTo($this, static::class) ?? $tmpl; + return $this->bindTemplate($tmpl); }- $template = $next->template->bindTo($this, static::class) ?? $next->template; + $template = $this->bindTemplate($next->template);Also applies to: 465-507
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/AbstractView.php` around lines 353 - 357, Extract the duplicated closure-binding expression from getTemplate() and parent() into a private helper such as bindTemplate(\Closure $tmpl): \Closure. Update both methods to call the helper while preserving the existing bindTo($this, static::class) behavior and fallback to the original closure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 241-243: Update the changelog entry for $capture and $section to
retain the explicit [] initialization while correcting the rationale: do not
claim that appending to null is deprecated in PHP 8.3; reference the PHP 8.1
deprecation of autovivification from false instead.
In `@docs/templates.md`:
- Around line 165-168: Add the `text` language tag to the fenced diagnostic
block containing the plain-text exception message in the documentation, without
changing the message content.
---
Nitpick comments:
In `@src/AbstractView.php`:
- Around line 353-357: Extract the duplicated closure-binding expression from
getTemplate() and parent() into a private helper such as bindTemplate(\Closure
$tmpl): \Closure. Update both methods to call the helper while preserving the
existing bindTo($this, static::class) behavior and fallback to the original
closure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 679ebad7-73fd-4c71-9b1e-a8b1a2af128b
📒 Files selected for processing (22)
CHANGELOG.mddocs/interfaces.mddocs/templates.mdsrc/AbstractView.phpsrc/Exception/ParentNotFound.phpsrc/ResolvedTemplate.phpsrc/SearchPathInterface.phpsrc/TemplateRegistry.phpsrc/View.phptests/FakeRegistryWithoutPaths.phptests/ParentTest.phptests/TemplateRegistryTest.phptests/fixtures/parent/app/boom.phptests/fixtures/parent/app/orphan.phptests/fixtures/parent/app/outer.phptests/fixtures/parent/app/read.phptests/fixtures/parent/app/vars.phptests/fixtures/parent/core/read.phptests/fixtures/parent/core/vars.phptests/fixtures/parent/module/read.phptests/fixtures/parent/ns-app/read.phptests/fixtures/parent/ns-core/read.php
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/fixtures/parent/core/read.php
- src/ResolvedTemplate.php
- tests/fixtures/parent/app/boom.php
- tests/fixtures/parent/app/vars.php
- tests/fixtures/parent/app/outer.php
- tests/fixtures/parent/ns-app/read.php
- tests/fixtures/parent/core/vars.php
- src/SearchPathInterface.php
- docs/interfaces.md
- src/TemplateRegistry.php
The CHANGELOG claimed appending to null is deprecated as of PHP 8.3. It is not: null autovivifies to array silently, and it was autovivification from false that PHP 8.1 deprecated -- verified on 8.4. Reword to say the explicit [] just avoids depending on either. Tag the strict-parent diagnostic block as text so it stops tripping markdown lint.
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
1 participant
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.
Search paths are first-hit-wins, so a template in an earlier directory shadows one of the same name later in the list and the shadowed version is unreachable: find() returns the first hit and drops the rest of the chain. Changing one part of a package's template therefore meant copying the whole file, which then stops tracking the original.
TemplateRegistry now records which directory satisfied each name, and exposes two primitives on SearchPathInterface: getResolvedPath(), which answers "which package's template won?", and getNext(), which resumes the search after a given directory.
getNext() returns a ResolvedTemplate rather than a bare Closure. Walking a chain past the first step needs the path the parent itself was found in, and a closure does not carry that.
AbstractView gains a render stack -- render() pushes (name, path) and pops in a finally -- so parent() resolves against the template actually executing when renders nest, and a throwing template leaves no frame behind. The output buffering render() did inline moves to captureTemplate() so parent() shares it, and so the blocks port can reuse the same stack rather than introducing a second one.
parent() returns '' rather than throwing when there is nothing further to render: overriding a template that turns out to shadow nothing is a normal state during development, not an error.
Summary by CodeRabbit
parent()can continue rendering shadowed templates across search paths, including nested inheritance.setStrictParent()/isStrictParent()) with a newParentNotFoundexception when no further parent exists.SearchPathInterfaceAPIs to inspect resolution:getResolvedPath()andgetNext().