umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.48k stars 2.69k forks source link

BeginUmbracoForm doesn't work with custom umbraco routes #13017

Closed ReallyHeadlessRick closed 1 year ago

ReallyHeadlessRick commented 2 years ago

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

10.2.0

Bug summary

Surface controller actions are not hit when rendering an Umbraco form from an Umbraco custom routes.

This used to work with the v8 equivalent setup (UmbracoVirtualNodeRouteHandler).

In v10 (and presumably v9 also) the alternate routing that takes place when a ufprt is present in the request only takes place for the dynamic controller route and not custom Umbraco routes.

Specifics

N/A

Steps to reproduce

  1. Create a new Umbraco 10 site & initialize.
  2. Create a document type (with template) named Home (no properties required).
  3. Create an instance of Home at root.
  4. Copy code samples below into project
  5. Form is handled appropriately from / but not from /demo/

Home.cshtml

@using Umbraco.Cms.Web.Common.PublishedModels;
@inherits Umbraco.Cms.Web.Common.Views.UmbracoViewPage<ContentModels.Home>
@using ContentModels = Umbraco.Cms.Web.Common.PublishedModels;
@{
    Layout = null;
}

<h1>@Model.Name</h1>

@using (Html.BeginUmbracoForm<DemoSurfaceController>("Handle"))
{
    @Html.TextBox("name")
    <input type="submit"/>
}

Demo.cs

public class DemoComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.Services.Configure<UmbracoPipelineOptions>(options =>
        {
            options.AddFilter(new UmbracoPipelineFilter(nameof(DemoPageController))
            {
                Endpoints = app => app.UseEndpoints(endpoints =>
                {
                    endpoints.MapControllerRoute(
                        "DemoPageController",
                        "demo",
                        new {Controller = "DemoPage", Action = "Index"});
                })
            });
        });
    }
}

public class DemoPageController : UmbracoPageController, IVirtualPageController
{
    private readonly UmbracoHelper _umbracoHelper;

    public DemoPageController(
        ILogger<UmbracoPageController> logger,
        ICompositeViewEngine compositeViewEngine,
        UmbracoHelper umbracoHelper) : base(logger, compositeViewEngine) =>
        _umbracoHelper = umbracoHelper;

    public IPublishedContent? FindContent(ActionExecutingContext actionExecutingContext) =>
        _umbracoHelper.ContentAtRoot().First();

    public IActionResult Index() =>
        View("~/Views/Home.cshtml", CurrentPage);
}

public class DemoSurfaceController : SurfaceController
{
    public DemoSurfaceController(
        IUmbracoContextAccessor umbracoContextAccessor,
        IUmbracoDatabaseFactory databaseFactory,
        ServiceContext services,
        AppCaches appCaches,
        IProfilingLogger profilingLogger,
        IPublishedUrlProvider publishedUrlProvider)
        : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider)
    { }

    public IActionResult Handle(string name) =>
        Ok(new {name});
}

Expected result / actual result

Should see output from DemoSurfaceController Handle() on submission of from from /demo/

github-actions[bot] commented 2 years ago

Hi there @ReallyHeadlessRick!

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 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:

andyfelton-equatedigital commented 2 years ago

Hi,

I've just hit this exact same issue. Has anybody else experienced it?

Thanks Andy

justin-nevitech commented 2 years ago

Hi @ReallyHeadlessRick

I've made a PR for this as it is due to the order the custom route endpoints are added (see https://github.com/umbraco/Umbraco-CMS/pull/13058).

There is a workaround if you need it which is to add your custom routes in Startup.cs after UseWebsiteEndpoints() is called:

            app.UseUmbraco()
                .WithMiddleware(u =>
                {
                    u.UseBackOffice();
                    u.UseWebsite();
                })
                .WithEndpoints(u =>
                {
                    u.UseInstallerEndpoints();
                    u.UseBackOfficeEndpoints();
                    u.UseWebsiteEndpoints();

                    u.EndpointRouteBuilder.MapControllerRoute(
                        "DemoPageController",
                        "demo",
                        new { Controller = "DemoPage", Action = "Index" });
                });
ReallyHeadlessRick commented 2 years ago

Hey @justin-nevitech - Cheers but there are still a few more issues to resolve.

If the surface controller is updated to the following, the response to the post on the /demo route is a 404 but works as expected for /.

// Demo.cs
public class DemoSurfaceController : SurfaceController
{
    public IActionResult Handle(string name)
    {
        ViewData["name"] = name;
        return CurrentUmbracoPage();
    }
}
// Home.cshtml
@using (Html.BeginUmbracoForm<DemoSurfaceController>("Handle"))
{
    @Html.TextBox("name")
    <input type="submit"/>
}

@ViewData["name"]
justin-nevitech commented 2 years ago

Hi @ReallyHeadlessRick

Ok, I will take another look.

justin-nevitech commented 2 years ago

Hi @ReallyHeadlessRick

I'm not getting far on this, the problem is that the surface controller doesn't know about the virtual page controller due to the order the controllers are executed which is dictated by the routing.

I will have to leave this for someone else to look into (possibly HQ) as changing the routing could have lots of unintended consequences!

I can get something working by adding HttpGet/HttpPost to the actions so the surface controller handles the POST but then it still has an issue displaying the results using CurrentUmbracoPage() as it thinks the current page is from a normal render controller not a virtual page controller. A redirect to the current URL works but then you loose temp data and model, etc.

Not sure if any of this helps of not as a workaround?:

    public class DemoComposer : IComposer
    {
        public void Compose(IUmbracoBuilder builder)
        {
            builder.Services.Configure<UmbracoPipelineOptions>(options =>
            {
                options.AddFilter(new UmbracoPipelineFilter(nameof(DemoPageController))
                {
                    Endpoints = app => app.UseEndpoints(endpoints =>
                    {
                        endpoints.MapControllerRoute(
                            "DemoPageController",
                            "demo",
                            new { Controller = "DemoPage", Action = "Index" });
                    })
                });
            });
        }
    }

    public class DemoPageController : UmbracoPageController, IVirtualPageController
    {
        private readonly UmbracoHelper _umbracoHelper;

        public DemoPageController(
            ILogger<UmbracoPageController> logger,
            ICompositeViewEngine compositeViewEngine,
            UmbracoHelper umbracoHelper) : base(logger, compositeViewEngine) =>
            _umbracoHelper = umbracoHelper;

        public IPublishedContent? FindContent(ActionExecutingContext actionExecutingContext) =>
            _umbracoHelper.ContentAtRoot().First();

        [HttpGet]
        public IActionResult Index() =>
            View("~/Views/HomePage.cshtml", CurrentPage);
    }

    public class DemoSurfaceController : SurfaceController
    {
        public DemoSurfaceController(
            IUmbracoContextAccessor umbracoContextAccessor,
            IUmbracoDatabaseFactory databaseFactory,
            ServiceContext services,
            AppCaches appCaches,
            IProfilingLogger profilingLogger,
            IPublishedUrlProvider publishedUrlProvider)
            : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider)
        { }

        //public IActionResult Handle(string name) =>
        //    Ok(new { name });

        [HttpPost]
        public IActionResult Handle(string name)
        {
            var currentPage = CurrentPage;
            ViewData["name"] = name;
            TempData["name"] = name;
            return RedirectToCurrentUmbracoUrl();
        }

    }
ReallyHeadlessRick commented 2 years ago

@justin-nevitech thanks for your efforts - I have a workaround that works fine for now (middleware that switches endpoint when ufprt is present and current endpoint matches some criteria).

justin-nevitech commented 2 years ago

Hi @ReallyHeadlessRick

Is it worth sharing your solution on here for others that may have the same issue?

justin-nevitech commented 2 years ago

Hi @ReallyHeadlessRick

I've had a another look at this issue (as it was bugging me!) and I think I've got a fully working solutions tested across normal Umbraco routes, hijacked routes and virtual pages. Fingers crossed!