Get template content from the template service, and fix the compiled-views fallback - #1059
Merged
Merged
Conversation
…views fallback
When importing a template we now ask Umbraco's ITemplateService for the file
content first (the way Umbraco does it), before falling back to the view
filesystem, and finally to a placeholder when the views are compiled.
Two things had to be fixed for that fallback to actually work:
* TemplateService.GetFileContentStreamAsync never returns null - the repository
hands back Stream.Null when the file is missing. Checking for null meant we
always took the "found it" branch, read an empty string, and created the
template with no content - so the view filesystem fallback and the
compiled-views placeholder below it were unreachable, and a genuinely missing
template file was silently imported as an empty one instead of failing.
* The placeholder we hand Umbraco is parsed by TemplateContentParserService to
work out the master template, and its regex requires a trailing semi-colon.
"{ Layout = "master" }" never matched, so the parent was never set. It is now
written as valid razor - @{ Layout = "master.cshtml"; } - and root templates
get Layout = null; rather than an empty alias.
Also aligns ViewPath (and the handler's equivalent) with how Umbraco names the
view file - the alias verbatim, see TemplateRepository.SetVirtualPath - rather
than stripping spaces, so we don't look for or delete the wrong file.
Tidy up while in here:
* remove the CleanseNode override - it looked for a "Content" element, but the
element is "Contents", so it has never done anything. TemplateTracker
explicitly tracks /Contents, so stripping it now would make the hash and the
tracker disagree - behaviour is unchanged, the dead code is gone.
* drop the unused _shortStringHelper field (constructor signature kept)
* flatten the dead null checks in ShouldGetContentFromNode
* make GetContentFromFile private and tidy its stream disposal
* remove a stray Lucene using
Tests: covers creating a child template when there is no file on disk, asserting
the placeholder parses back to the parent alias using Umbraco's own parser, plus
the root-template and views-not-compiled cases. BuildFileSystems now sets up
IIOHelper.PathStartsWith, without which PhysicalFileSystem throws instead of
reporting a missing file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
What
When importing a template, we now ask Umbraco's
ITemplateServicefor the file content first — the way Umbraco itself does it — before falling back to the view filesystem, and finally to a placeholder when the views are compiled into the site.Getting that fallback chain to actually work needed two fixes:
GetFileContentStreamAsyncnever returns null.TemplateRepository.GetFileContentStreamhands backStream.Nullwhen the file is missing (the signature isTask<Stream>, notTask<Stream?>). Checking fornullmeant we always took the "found it" branch, read an empty string, and created the template with no content. The view filesystem fallback and the compiled-views placeholder below it were unreachable, and a genuinely missing template file was silently imported as an empty template rather than failing with the "local file is missing" warning.The placeholder wasn't parseable. Umbraco derives the master template by running
TemplateContentParserServiceover the content, and its regex —\s*Layout\s*=\s*"?(?<layout>[\w\s\.]*)"?;— requires the trailing semi-colon.{ Layout = "master" }never matched, so the parent was never set on a template created this way. It's now written as valid razor:Root templates get
Layout = null;instead of pointing Umbraco at a template called"".Also aligns
ViewPath(and the equivalent inTemplateHandler) with how Umbraco names the view file — the alias verbatim, seeTemplateRepository.SetVirtualPath— rather than stripping spaces, so we don't look for, or delete, the wrong file for an alias containing a space.Tidy-ups in the same files
CleanseNodeoverride. It removed a"Content"element, but the element is"Contents"— it has never done anything in any shipped version. Rather than "fix" the name, it's deleted:TemplateTrackerexplicitly tracks/Contents, so making the strip work would have the change hash ignore a change the tracker reports. Behaviour is unchanged; the dead code is gone._shortStringHelperfield. The constructor parameter stays so the signature isn't broken.ShouldGetContentFromNode— it had null checks on non-nullable injected services. All four input combinations give the same answers as before.GetContentFromFileis nowprivate(nothing in OpenSource or Products calls it) with tidier stream disposal.using Lucene.Net.Queries.Function.ValueSources;.Tests
Three new tests in
TemplateSerializerTests, covering the no-file-on-disk path: creating a child template whose parent must be resolved from the placeholder, the root-template case, and the views-not-compiled case that should fail rather than silently create an empty template.The parent test asserts the generated content parses back to
"master"using Umbraco's realTemplateContentParserService, rather than re-implementing the regex — that parse is the actual mechanism by which the master gets set.Both bugs above are covered: reverting the fixes fails these tests with
Expected: "master" But was: null/ content<string.Empty>, andExpected: False But was: Truewith message"Created".Full suite: 144 passed, 0 failed.
Note for reviewers
BuildFileSystemsin the test fixture now sets upIIOHelper.PathStartsWith. These are the first tests to reach_viewFileSystem.FileExists, andPhysicalFileSystem.GetFullPathuses that call to check a path is inside its root — unmocked it returnsfalse, so every path looked out-of-root andFileExiststhrewUnauthorizedAccessExceptioninstead of returning false. Test-harness gap, not a product issue, but worth knowing for future tests that touch the view filesystem.TemplateSerializer.csexists inv18/mainwith the same code, so this needs forward-porting.🤖 Generated with Claude Code