switch from SiteDefinition to the new Application framework#145
switch from SiteDefinition to the new Application framework#145OddSkancke wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the sitemap generation logic to migrate from the legacy EPiServer Web API (such as SiteDefinition and HostDefinition) to the newer EPiServer Applications API (such as IRoutableApplication and ApplicationHost). It also updates deprecated attributes for scheduled jobs and property definitions. The review feedback highlights several potential NullReferenceException risks where properties (like .Application or .Authority) are accessed on objects that could be null, and suggests using null-conditional operators or safer string comparison patterns to prevent runtime errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
f5bc256 to
22cdd21
Compare
ivanmarkovic1402
left a comment
There was a problem hiding this comment.
Thanks for this — the direction is right. SiteDefinition is deprecated in CMS 13 and the new Applications API (IApplicationRepository / IApplicationResolver / IRoutableApplication) is the sanctioned replacement, and it's used correctly throughout. A few notes inline.
The main functional concern is the GetByHostname(sitemapData.Host, …) / NOSITE/ path in SitemapViewModel, which looks like it won't resolve and falls back to a placeholder URL. There are also a couple of behaviour-preservation questions (the wildcard-host fallback and the host dropdown).
Finally, a versioning note: this is a breaking change to the public API (the IContentFilter signature, the generator constructors, and the protected members of SitemapXmlGenerator). Since 4.0.0 already shipped on the SiteDefinition API, anyone who implemented IContentFilter or subclassed a generator will no longer compile against this — so this should go out as a major release (5.0.0), not a minor, with the breaking changes listed in the CHANGELOG. A smoke test around multi-site / host resolution would also help.
| { | ||
| var siteUrl = siteInformation.SiteUrl.ToString(); | ||
| siteUrls.Add(new() | ||
| foreach (var host in site.Hosts.Where(x => x.Url != null)) |
There was a problem hiding this comment.
Two behaviour changes worth a look:
- The application's primary URL (
IRoutableApplication.Url) is no longer added — only per-host URLs are listed. The old code addedSiteUrlexplicitly, so a routable app whose primary URL isn't also represented as aHostentry will drop out of the dropdown. - The de-dup that the old
ShouldAddToSiteHostsprovided (skipping a host equal to the site URL) is gone, so duplicate entries are now possible.
Also, with ShouldAddToSiteHosts removed, UriComparer looks unused now — consider deleting it as well (like HostDefinitionExtensions).
There was a problem hiding this comment.
Reproduced locally on this branch. With the current LoadSiteHosts, the primary application entry still breaks sitemap generation, while the per-host entry works:
http://localhost:7235/sitemap.xml → Success
localhost:5127/sitemap.xml → An error occurred while generating sitemap: Invalid URI: The format of the URI could not be determined. (primary entry: scheme-less)
Root cause: the primary entry stores Value = site.Url.Host, which is a bare host with no scheme. That value is bound to SitemapViewModel.SiteUrl → SitemapData.SiteUrl, and SitemapXmlGenerator.Generate does new Uri(SitemapData.SiteUrl), which requires an absolute URI. The host entries work because they use the full host.Url.ToString().
Fix — store the full absolute URL for the primary entry too:
// keep .Host for the display Text if you want a shorter label, but Value must be absolute
urls.Add(new SelectListItem { Text = site.Url.Host, Value = site.Url.ToString() });
|
This is a breaking change for sure. The CHANGELOG.md is edited manually, or by the build-system? If manually, how is the version bumped? In any case, I think I have fixed the issues pointed out by @ivanmarkovic1402, but would like to get everything correct before force-pushing changes to this PR. EDIT: I have force-pushed changes fixing what I hope to be most of the issues pointed out above. |
The CHANGELOG is manual. I'll release it when merged. (just need to re-test few things before merge) |
This PR replaces the use of SiteDefinition with the new Application framework. Removes the last of the obsolete warnings.