Skip to content

Conversation

@AndyButland
Copy link
Contributor

@AndyButland AndyButland commented Jun 16, 2025

Prerequisites

  • I have added steps to test this contribution in the description below

Resolves: #19567

Description

The cause of this issue was another cases of issues involved in the fix #19229, where ISet in configuration classes aren't bound.

Note that I've added a compatibility suppression for the breaking change, which I don't see how we can otherwise avoid.

Testing

Prepare a custom 404 page as per the instructions in the documentation (though note that I found this didn't work when using the name "404" for the document type and template, and instead used "File Not Found"). With this PR in place, the custom 404 should be displayed as expected.

To check via debugging, put a breaking point at the start of NotFoundHandlerHelper.GetCurrentNotFoundPageId and verify the error404Collection parameter matches the configured value.

Release

Can consider for a 16.0.1, otherwise 16.1.0.

Copilot AI review requested due to automatic review settings June 16, 2025 20:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the binding regression for custom 404 pages by changing the configuration property type and updating affected tests, and suppresses a compatibility warning.

  • Changed Error404Collection from ISet<ContentErrorPage> to IEnumerable<ContentErrorPage> with a C# 12 collection expression default.
  • Updated unit test initializer syntax in ContentSettingsValidatorTests.cs to use [...].
  • Added a compatibility suppression for the breaking change in CompatibilitySuppressions.xml.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
tests/Umbraco.Tests.UnitTests/Umbraco.Core/Configuration/Models/Validation/ContentSettingsValidatorTests.cs Switched collection initializer syntax to C# 12 [...]
src/Umbraco.Core/Configuration/Models/ContentSettings.cs Changed property type from ISet to IEnumerable
src/Umbraco.Core/CompatibilitySuppressions.xml Added suppression for CP0002 on Error404Collection
Comments suppressed due to low confidence (2)

src/Umbraco.Core/Configuration/Models/ContentSettings.cs:53

  • Changing Error404Collection from ISet to IEnumerable removes set semantics and mutability; consider using IReadOnlySet or IReadOnlyCollection to preserve uniqueness and clearly express intended behavior.
public IEnumerable<ContentErrorPage> Error404Collection { get; set; } = [];

src/Umbraco.Core/Configuration/Models/ContentSettings.cs:50

  • [nitpick] The XML doc still implies set behavior; update the summary to reflect the change to an IEnumerable and clarify whether duplicates are permitted.
///     Gets or sets a value for the collection of error pages.

@henryson
Copy link

Will this also apply to version 15? I have the same problem in 15.4.1

@AndyButland
Copy link
Contributor Author

Will this also apply to version 15? I have the same problem in 15.4.1

No, this is specifically fixing a regression issue in 16, so if you are having issues with 15 it'll likely be unrelated.

@kjac kjac added category/breaking status/regression A previously working feature that has broken or changed behavior unexpectedly release/16.1.0 labels Jun 30, 2025
Copy link
Contributor

@kjac kjac left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Please note that this is a breaking change due to the change of public signature. It is, however, a strictly necessary change which fixes a regression, and thus I'm accepting it for 16.1 👍

I also went the code route to test this 🤓

Given this code:

using Microsoft.Extensions.Options;
using Umbraco.Cms.Core.Composing;
using Umbraco.Cms.Core.Configuration.Models;

namespace Umbraco.Cms.Web.UI.Custom;

public class TestError404CollectionComponent : IComponent
{
    private readonly ContentSettings _contentSettings;
    private readonly ILogger<TestError404CollectionComponent> _logger;

    public TestError404CollectionComponent(
        IOptions<ContentSettings> contentSettings,
        ILogger<TestError404CollectionComponent> logger)
    {
        _logger = logger;
        _contentSettings = contentSettings.Value;
    }

    public void Initialize()
        => _logger.LogInformation(
            "### Found {numberOf404Configs} 404 page configs ###",
            // the fix is breaking, so this line needs updating after applying the fix
            _contentSettings.Error404Collection.Count);

    public void Terminate()
    {
    }
}

public class TestError404CollectionComposer : ComponentComposer<TestError404CollectionComponent>
{
}

...and this 404 configuration:

"Error404Collection": [
  {
	"Culture": "en-us",
	"ContentKey": "575b9a71-28d2-47b0-bd94-ab98e0fc5f2d"
  },
  {
	"Culture": "da-dk",
	"ContentKey": "d35282e3-3a2d-4566-965a-c2d91b0fe46e"
  }
]

Without the fix I get: ### Found 0 404 page configs ###

With the fix I get the expected: ### Found 2 404 page configs ###

@kjac kjac merged commit cfbbfa0 into main Jun 30, 2025
26 checks passed
@kjac kjac deleted the v16/bugfix/fix-config-for-error-404-collection branch June 30, 2025 12:37
@umbracocommunity
Copy link

This pull request has been mentioned on Umbraco community forum. There might be relevant details there:

https://forum.umbraco.com/t/issue-with-404-page-configuration/4471/5

@thenexus00
Copy link

I do not believe this is fixed with any default behavior.
In two 16.1.1 builds nothing I have tried works.

  • I followed the documentation to the letter - does not work.
  • I just had a different document type page - does not work
  • I tried moving the custom 404 page across multiple locations - does not work
  • I have tried in appsettings the ContentKey, I tried the old ContentId as well both with GUID and old style ID which did not work. I tried content X Path as well in both URL and name - did not work.
  • I removed all I could from program.cs so it was bare and just loaded the site and that still did not work.
  • I tried my own custom middleware - still did not work.

I am about to try the code kjac l posted but if that is the only thing that works I do not consider this a working solution.

@thenexus00
Copy link

I do not believe this is fixed with any default behavior. In two 16.1.1 builds nothing I have tried works.

  • I followed the documentation to the letter - does not work.
  • I just had a different document type page - does not work
  • I tried moving the custom 404 page across multiple locations - does not work
  • I have tried in appsettings the ContentKey, I tried the old ContentId as well both with GUID and old style ID which did not work. I tried content X Path as well in both URL and name - did not work.
  • I removed all I could from program.cs so it was bare and just loaded the site and that still did not work.
  • I tried my own custom middleware - still did not work.

I am about to try the code kjac l posted but if that is the only thing that works I do not consider this a working solution.

I have only been able to solve the issue with custom middleware targeting
context.Response.StatusCode == 404

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

Labels

category/breaking release/16.1.0 status/regression A previously working feature that has broken or changed behavior unexpectedly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom 404 Error Page not working in umbraco 16

6 participants