Skip to content

Upgrade to cms13 and aspire#41

Merged
ivanmarkovic1402 merged 15 commits into
upgrade/cms13from
upgrade-to-cms13-and-aspire
Jun 1, 2026
Merged

Upgrade to cms13 and aspire#41
ivanmarkovic1402 merged 15 commits into
upgrade/cms13from
upgrade-to-cms13-and-aspire

Conversation

@ivanmarkovic1402

Copy link
Copy Markdown
Contributor

Upgrade to cms 13. Aspire added.

@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 upgrades the solution to .NET 10 and Optimizely CMS 13, replacing the sandbox Alloy project with a new web project utilizing the Foundation core. The changes include refactoring the backing type resolution logic and updating property loading to align with CMS 13's lazy value handling. Technical feedback identifies a critical reflection bug in the NoOpSyncClientProxy, unsafe service registration logic in the initialization module, and security concerns regarding the disabling of NuGet auditing. Furthermore, improvements are suggested to replace fragile relative paths with anchored paths and to optimize the backing type resolver by avoiding redundant repository scans for cached types.

Comment thread src/Geta.Optimizely.GenericLinks.Web/Program.cs
Comment thread src/Geta.Optimizely.GenericLinks.Web/Startup.cs
@valdisiljuconoks valdisiljuconoks requested a review from Copilot May 28, 2026 04:47

Copilot AI 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.

Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again.

Comment thread src/Geta.Optimizely.GenericLinks/Geta.Optimizely.GenericLinks.csproj Outdated

@mariajemaria mariajemaria 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.

Cross-cutting concern that doesn't anchor neatly on a single line, plus a release-pipeline check below — the rest is inline.

Split Geta.Optimizely.GenericLinks.ContentDeliveryApi removal into a separate PR

Geta.Optimizely.GenericLinks.ContentDeliveryApi is a separately published NuGet — deleting it isn't just a SemVer event on the main package, it strands every consumer of that package ID with no migration path. Bundling it with the CMS 13 / .NET 10 upgrade makes consumer bisection impossible if anything regresses.

Asks:

  1. Split into two PRs: (a) CMS 13 + .NET 10 upgrade, (b) ContentDeliveryApi removal.
  2. Major version bump on the main package + changelog entry calling out the deletion.
  3. Deprecate/unlist the old .ContentDeliveryApi NuGet with a readme pointing at the replacement path (Optimizely Graph appears to be the CMS 13 story — .Web already pulls Optimizely.Graph.Cms/.Query).
  4. Document the migration for existing consumers in the repo readme.

If there's a separate Graph-based integration package already, please link it — I couldn't find a CMS 13 replacement story spelled out anywhere in this PR.

Comment thread src/Geta.Optimizely.GenericLinks/PropertyLinkBase.cs
Comment thread tests/Geta.Optimizely.GenericLinks.Tests/Services/NullBackingTypeResolver.cs Outdated
Comment thread src/Geta.Optimizely.GenericLinks/PropertyLinkDataCollection.cs
Comment thread .gitmodules
Comment thread src/Geta.Optimizely.GenericLinks/Geta.Optimizely.GenericLinks.csproj Outdated
Comment thread .github/workflows/release.yml
update changelog and readme
update relese.yml
remove content-delivery-api.md
Dropped Newtonsoft.Json entirely — consolidated on System.Text.Json
.csproj file updated
handling exceptions fixed
@ivanmarkovic1402

Copy link
Copy Markdown
Contributor Author

Cross-cutting concern that doesn't anchor neatly on a single line, plus a release-pipeline check below — the rest is inline.

Split Geta.Optimizely.GenericLinks.ContentDeliveryApi removal into a separate PR

Geta.Optimizely.GenericLinks.ContentDeliveryApi is a separately published NuGet — deleting it isn't just a SemVer event on the main package, it strands every consumer of that package ID with no migration path. Bundling it with the CMS 13 / .NET 10 upgrade makes consumer bisection impossible if anything regresses.

Asks:

  1. Split into two PRs: (a) CMS 13 + .NET 10 upgrade, (b) ContentDeliveryApi removal.
  2. Major version bump on the main package + changelog entry calling out the deletion.
  3. Deprecate/unlist the old .ContentDeliveryApi NuGet with a readme pointing at the replacement path (Optimizely Graph appears to be the CMS 13 story — .Web already pulls Optimizely.Graph.Cms/.Query).
  4. Document the migration for existing consumers in the repo readme.

If there's a separate Graph-based integration package already, please link it — I couldn't find a CMS 13 replacement story spelled out anywhere in this PR.

After looking into this, splitting into two PRs isn't practical — Geta.Optimizely.GenericLinks.ContentDeliveryApi depends on EPiServer.ContentDeliveryApi.Core [3.3.0, 4.0.0), which requires EPiServer.CMS.UI.Core < 13.0.0. There is no CMS 13 compatible version and Optimizely hasn't announced one — Content Delivery API was replaced by Graph at the platform level. The project physically can't compile once CMS is upgraded to 13, so the removal is a direct consequence of the upgrade, not a separate decision.

That said, if you'd prefer two PRs, it's technically doable: PR (a) upgrades to CMS 13 and removes the project from the .sln so the solution builds, PR (b) deletes the source files and cleans up the release pipeline. The result between the two PRs would just be dead, unbuildable source sitting on disk.

Major version bump and changelog are already in place — v3.0.0 changelog explicitly calls out the ContentDeliveryApi deletion as a breaking change. Deprecating the old 2.x package on the feed after release makes sense, we'll do that. No migration docs needed — there's no replacement package; Graph indexes GenericLinks properties at the platform level without custom converters.

So, should I split it in two or keep as-is?

@ivanmarkovic1402 ivanmarkovic1402 merged commit d721aa9 into upgrade/cms13 Jun 1, 2026
2 checks passed
@ivanmarkovic1402 ivanmarkovic1402 deleted the upgrade-to-cms13-and-aspire branch June 1, 2026 10:52
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.

4 participants