Generic versions of BeginUmbracoForm do not have their additionalRouteVals parameter marked as nullable

Nysosis commented 1 year ago

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

10.0.0

Bug summary

Within the HtmlHelperRenderExtensions methods, the final method of RenderForm that is actually called from the various BeginUmbracoForm versions has additionalRouteVals as a nullable value.

However none of the versions of the BeginUmbracoForm with the generic SurfaceController signature allow for a nullable value. This seems odd, given that it feeds through into the method mentioned above?

If your project has nullables on, and compiled View files, then you either need to just live with the compiler messages (unless you are treating all nullables as errors, in which case I assume that they would also be treated as errors and break the build, I'm not currently doing this so can't confirm that point), or do null! every place you are calling it

Specifics

No response

Steps to reproduce

Within a new Umbraco project, create dummy SurfaceController to use in your view.

using Microsoft.AspNetCore.Mvc;
using Umbraco.Cms.Web.Common.Filters;
using Umbraco.Cms.Web.Website.Controllers;

public class DummySurfaceController : SurfaceController {
    [HttpPost, ValidateUmbracoFormRouteString]
    public IActionResult PerformAction() => NoContent();
}

Within a View, start a new call to BeginUmbracoForm referencing this Controller

<div>
    @using(Html.BeginUmbracoForm<DummySurfaceController >(nameof(DummySurfaceController .PerformAction), additionalRouteVals: null, htmlAttributes: new Dictionary<string, object?> {
        { "autocomplete", "off" },
        { "class", "justify-content-center" },
    })) {
        <input type="submit" value="Perform Action" />
    }
</div>

If your project is set to nullables and warnings, you should see that the value for additionalRouteVals is marked as an issue

But the equivalent non-generic method does not have the same issue

Expected result / actual result

Ideally all the methods should match in parameter nullablitly, and even more ideally, default nulls on the additionalRouteValues so that we don't need to specify the parameter if just needing to use htmlAttributes parameter

github-actions[bot] commented 1 year ago

Hi there @Nysosis!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
We'll replicate the issue to ensure that the problem is as described.
We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

kjac commented 1 year ago

Hi there @Nysosis,

Thank you for reaching out 😄

I can see how this would be annoying. For the time being I would assume an anonymous object would do the trick instead of the null:

Html.BeginUmbracoForm<DummySurfaceController>(nameof(DummySurfaceController.PerformAction), additionalRouteVals: new { }, htmlAttributes: new Dictionary<string, object?>())

We'd love some help aligning the generic BeginUmbracoForm methods in HtmlHelperRenderExtensions to match the non-generic ones in input parameters. This may be a slightly tricky task though, as there are quite a lot of overloads to the non-generic BeginUmbracoForm 😄

github-actions[bot] commented 1 year ago

Hi @Nysosis,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post.

Thanks muchly, from your friendly Umbraco GitHub bot :-)

miski commented 1 year ago

Hi @Nysosis and @kjac,

I fixed this for additionalRouteVals, so I'll make a pull request shortly.

umbraco / Umbraco-CMS