Skip to content

switch from SiteDefinition to the new Application framework#145

Open
OddSkancke wants to merge 1 commit into
Geta:masterfrom
Visma-SmartSkill:newvssmain
Open

switch from SiteDefinition to the new Application framework#145
OddSkancke wants to merge 1 commit into
Geta:masterfrom
Visma-SmartSkill:newvssmain

Conversation

@OddSkancke

Copy link
Copy Markdown

This PR replaces the use of SiteDefinition with the new Application framework. Removes the last of the obsolete warnings.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/Geta.Optimizely.Sitemaps/Repositories/SitemapRepository.cs
Comment thread src/Geta.Optimizely.Sitemaps/Models/SitemapViewModel.cs Outdated
Comment thread src/Geta.Optimizely.Sitemaps/XML/SitemapXmlGenerator.cs Outdated
Comment thread src/Geta.Optimizely.Sitemaps/XML/SitemapXmlGenerator.cs Outdated
@OddSkancke OddSkancke force-pushed the newvssmain branch 2 times, most recently from f5bc256 to 22cdd21 Compare June 24, 2026 09:08

@ivanmarkovic1402 ivanmarkovic1402 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/Geta.Optimizely.Sitemaps/Models/SitemapViewModel.cs Outdated
Comment thread src/Geta.Optimizely.Sitemaps/XML/SitemapXmlGenerator.cs Outdated
{
var siteUrl = siteInformation.SiteUrl.ToString();
siteUrls.Add(new()
foreach (var host in site.Hosts.Where(x => x.Url != null))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two behaviour changes worth a look:

  1. The application's primary URL (IRoutableApplication.Url) is no longer added — only per-host URLs are listed. The old code added SiteUrl explicitly, so a routable app whose primary URL isn't also represented as a Host entry will drop out of the dropdown.
  2. The de-dup that the old ShouldAddToSiteHosts provided (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).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() });

Comment thread src/Geta.Optimizely.Sitemaps/Utils/IContentFilter.cs Outdated
@OddSkancke

OddSkancke commented Jun 25, 2026

Copy link
Copy Markdown
Author

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.

@ivanmarkovic1402

Copy link
Copy Markdown
Contributor

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants