[BR-5446] Migrate Gibe.Umbraco.Blog to .NET 10.0#32
[BR-5446] Migrate Gibe.Umbraco.Blog to .NET 10.0#32MDuncan98 wants to merge 3 commits intov13/masterfrom
Conversation
MDuncan98
changed the title
MDuncan98
requested review from
a team,
Reaction77,
ThomasAdams2,
tristanjthompson and
zade107
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
This PR migrates the Gibe.Umbraco.Blog package from .NET 6/Umbraco 10 to Umbraco 13, though the PR title incorrectly refers to ".NET 10" which does not exist. The migration includes API updates, removal of legacy multi-targeting code, and modernization of the configuration system from hardcoded settings to the options pattern.
Changes:
- Updated target framework and Umbraco package reference to version 13.13.0
- Migrated from
GetSearcher()method toSearcherproperty for Examine index API - Removed conditional compilation directives for .NET Framework 4.7.2 and .NET 6
- Replaced
HardCodedBlogSettingswithBlogSettingsusing the options pattern for configuration - Migrated solution file from .sln to .slnx format
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| VERSION.txt | Updated version from 10.0.0 to 13.0.0 |
| Gibe.Umbraco.Blog/Wrappers/NewsIndex.cs | Changed from GetSearcher() method to Searcher property |
| Gibe.Umbraco.Blog/Wrappers/ISearchIndex.cs | Changed from GetSearcher() method to Searcher property in FakeSearchIndex |
| Gibe.Umbraco.Blog/Service.cs | Removed HardCodedBlogSettings registration, added options pattern configuration, removed unused import |
| Gibe.Umbraco.Blog/Repositories/IBlogContentRepository.cs | Removed conditional compilation directives for .NET Framework |
| Gibe.Umbraco.Blog/Repositories/BlogContentRepository.cs | Removed conditional compilation directives for .NET Framework |
| Gibe.Umbraco.Blog/Models/IBlogPostModel.cs | Removed conditional compilation directives for .NET Framework |
| Gibe.Umbraco.Blog/Models/HardCodedBlogSettings.cs | Deleted file - replaced by BlogSettings |
| Gibe.Umbraco.Blog/Models/BlogSettings.cs | New file implementing IBlogSettings with settable properties for options pattern |
| Gibe.Umbraco.Blog/Models/BlogPostBase.cs | Removed conditional compilation and .NET Framework constructor |
| Gibe.Umbraco.Blog/Models/BlogAuthor.cs | Removed conditional compilation directives for .NET Framework |
| Gibe.Umbraco.Blog/Gibe.Umbraco.Blog.csproj | Updated to net10.0 target, Umbraco 13.13.0, removed conditional compilation |
| Gibe.Umbraco.Blog.slnx | New XML-based solution file replacing the old .sln |
| Gibe.Umbraco.Blog.sln | Deleted traditional solution file |
| Gibe.Umbraco.Blog.Tests/Gibe.Umbraco.Blog.Tests.csproj | Updated to net10.0 target, Umbraco 13.13.0, removed conditional compilation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <PropertyGroup Condition="'$(TargetFramework)' == 'net6.0'"> | ||
| <DefineConstants>NET6,NET6</DefineConstants> | ||
| <PropertyGroup> | ||
| <DefineConstants>NET10,NET10</DefineConstants> |
There was a problem hiding this comment.
The define constant "NET10" is duplicated. The DefineConstants property should be "NET10" (or the correct .NET version identifier once the framework version is corrected), not "NET10,NET10".
| <DefineConstants>NET10,NET10</DefineConstants> | |
| <DefineConstants>NET10</DefineConstants> |
| { | ||
| services.AddOptions<BlogSettings>() | ||
| .BindConfiguration("Gibe:Umbraco:Blog") | ||
| .ValidateOnStart(); |
There was a problem hiding this comment.
The service registration is missing for IBlogSettings. While BlogSettings is configured using the options pattern (lines 23-25), there is no registration that binds IBlogSettings to BlogSettings for dependency injection. Classes like BlogSearch, NewsIndex, IndexEventsComponent, and BlogSections inject IBlogSettings directly, not IOptions<BlogSettings>. You need to add a service registration such as: services.AddSingleton<IBlogSettings>(sp => sp.GetRequiredService<IOptions<BlogSettings>>().Value) or modify all consuming classes to inject IOptions<BlogSettings> instead.
| .ValidateOnStart(); | |
| .ValidateOnStart(); | |
| services.AddSingleton<IBlogSettings>(sp => sp.GetRequiredService<IOptions<BlogSettings>>().Value); |
| @@ -1 +1 @@ | |||
| 10.0.0 No newline at end of file | |||
| 13.0.0 No newline at end of file | |||
There was a problem hiding this comment.
There's a discrepancy between the PR title and the code changes. The PR title states "Migrate Gibe.Umbraco.Blog to .NET 10" but the version number changes (from 10.0.0 to 13.0.0) and package references (Umbraco.Cms.Web.Website Version="13.13.0") suggest this is actually a migration to Umbraco 13, not .NET 10. Additionally, .NET 10 does not exist yet. This appears to be confusion between Umbraco version numbers and .NET version numbers. Please clarify the actual intent of this migration.
| <PropertyGroup> | ||
| <DefineConstants>NET10</DefineConstants> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
Do we need this constant at all any more?
| <TargetFramework>net10.0</TargetFramework> | ||
| </PropertyGroup> | ||
| <PropertyGroup> | ||
| <PackageId>Gibe.Umbraco.V10.Blog</PackageId> | ||
| <PackageVersion>10.0.0</PackageVersion> | ||
| <PackageId>Gibe.Umbraco.V13.Blog</PackageId> | ||
| <PackageVersion>13.0.0</PackageVersion> |
There was a problem hiding this comment.
Nitpick: technically, v13 is net8...though I suppose it should be able to run in a net10 runtime, but you'd have to make sure you updated the net version to 10 - if we use this in the content store we should validate that?
| <PropertyGroup> | ||
| <DefineConstants>NET10,NET10</DefineConstants> | ||
| </PropertyGroup> |
There was a problem hiding this comment.
Do we still need the constants now we're not dual targetting?
2 participants
Jira Ticket: https://jira.gibedigital.net/browse/BR-5446
Description
Migrates Gibe.Umbraco.Blog to .NET 10.0.
Checklist
Please confirm each item before assigning people to this PR.