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.47k stars 2.69k forks source link

BeginUmbracoForm does not work correctly with custom routes/UmbracoPageController #14868

Open jhenkes opened 1 year ago

jhenkes commented 1 year ago

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

11.5.0

Bug summary

When using an UmbracoPageController to submit a form to a surface controller, the surface controller action method is called but "CurrentUmbracoPage" and "RedirectToCurrentUmbracoPage" do not work as expected.

Specifics

No response

Steps to reproduce

This issue is related to #13103. I've reused the testing code and made minor modifications to showcase the issue.

Steps to reproduce:

Demo.cs ``` using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.ViewEngines; using Umbraco.Cms.Core.Cache; using Umbraco.Cms.Core.Composing; using Umbraco.Cms.Core.Logging; using Umbraco.Cms.Core.Models.PublishedContent; using Umbraco.Cms.Core.Routing; using Umbraco.Cms.Core.Services; using Umbraco.Cms.Core.Web; using Umbraco.Cms.Infrastructure.Persistence; using Umbraco.Cms.Web.Common; using Umbraco.Cms.Web.Common.ApplicationBuilder; using Umbraco.Cms.Web.Common.Controllers; using Umbraco.Cms.Web.Website.Controllers; namespace ToDelete5 { public class DemoComposer : IComposer { public void Compose(IUmbracoBuilder builder) { builder.Services.Configure(options => { options.AddFilter(new UmbracoPipelineFilter(nameof(DemoPageController)) { Endpoints = app => app.UseEndpoints(endpoints => { endpoints.MapControllerRoute( "DemoPageController", "demo/{action}", new { Controller = "DemoPage", Action = "Index" }); endpoints.MapControllerRoute( "TestController", "test/{action}", new { Controller = "Test", Action = "Index" }) .ForUmbracoPage((actionExecutingContext) => { var umbracoHelper = actionExecutingContext.HttpContext.RequestServices.GetService(); return umbracoHelper?.ContentAtRoot().First()!; }); }) }); }); } } public class TestController : UmbracoPageController { public TestController( ILogger logger, ICompositeViewEngine compositeViewEngine) : base(logger, compositeViewEngine) { } [HttpGet] public IActionResult Index() => View("~/Views/HomePage.cshtml", CurrentPage); [HttpGet] public IActionResult Subpage() => View("~/Views/HomePage.cshtml", CurrentPage); } public class DemoPageController : UmbracoPageController, IVirtualPageController { private readonly UmbracoHelper _umbracoHelper; public DemoPageController( ILogger 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); [HttpGet] public IActionResult Subpage() => 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) { } [HttpPost] public IActionResult HandleCurrentPage(string name) { var currentPage = CurrentPage; ViewData["name"] = name; TempData["name"] = name; return CurrentUmbracoPage(); } [HttpPost] public IActionResult HandleRedirectToCurrentPage(string name) { var currentPage = CurrentPage; ViewData["name"] = name; TempData["name"] = name; return RedirectToCurrentUmbracoPage(); } [HttpPost] public IActionResult HandleRedirectToCurrentUrl(string name) { var currentPage = CurrentPage; ViewData["name"] = name; TempData["name"] = name; return RedirectToCurrentUmbracoUrl(); } } } ```
HomePage.cshtml ``` @using Umbraco.Cms.Web.Common.PublishedModels; @inherits Umbraco.Cms.Web.Common.Views.UmbracoViewPage @using ContentModels = Umbraco.Cms.Web.Common.PublishedModels; @{ Layout = null; }

@ViewContext.RouteData.Values["action"]: @Model.Name

@using (Html.BeginUmbracoForm("HandleCurrentPage")) { @Html.TextBox("name") } @using (Html.BeginUmbracoForm("HandleRedirectToCurrentPage")) { @Html.TextBox("name") } @using (Html.BeginUmbracoForm("HandleRedirectToCurrentUrl")) { @Html.TextBox("name") }

View Data: @ViewData["name"]

Temp Data: @TempData["name"]

```

Call either "/demo/subpage" (= UmbracoPageController, IVirtualPageController) or "/test/subpage" (= UmbracoPageController). The "Subpage" action is called as expected. Now click on:

  1. Handle Current Page (CurrentUmbracoPage): Returns the "Index" action instead of "Subpage".
  2. Handle Redirect To Current Page (RedirectToCurrentUmbracoPage): Returns 404 instead of "Subpage".
  3. Handle Redirect To Current URL (HandleRedirectToCurrentUrl): No issue here, works as expected.

The difference between the original testing code and the modified version is the inclusion of the "{action}" for the routing pattern. Testing it with any action other than "Index" showcases N°1. N°2 is broken with or without the additional "{action}" pattern.

Expected result / actual result

Expected: "CurrentUmbracoPage" and "RedirectToCurrentUmbracoPage" returns/calls the original action where the form submit came from. Actual: "CurrentUmbracoPage" and "RedirectToCurrentUmbracoPage" returns the "Index" action and "404" respectively.


This item has been added to our backlog AB#34096

github-actions[bot] commented 1 year ago

Hi there @jhenkes!

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:

jhenkes commented 1 year ago

Adding one additional test case for attribute routing, because it has similiar issues:

    [Route("Attribute/{action=Index}")]
    public class AttributeController : UmbracoPageController, IVirtualPageController
    {
        private readonly UmbracoHelper _umbracoHelper;

        public AttributeController(
            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);

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

Call "/attribute/subpage". The "Subpage" action is called as expected. Now click on:

  1. Handle Current Page (CurrentUmbracoPage): Returns 404 instead of "Subpage".
  2. Handle Redirect To Current Page (RedirectToCurrentUmbracoPage): Returns Exception page (InvalidOperationException: Cannot redirect, no entity was found for key 00000000-0000-0000-0000-000000000000) instead of "Subpage"
  3. Handle Redirect To Current URL (HandleRedirectToCurrentUrl): No issue here, works as expected.

This specific test case is also related to the second issue in #14165 with the "Cannot redirect ..." exception, where I already left a comment about it. But this test case here is simpler and thus better for showcasing the issue.

Migaroez commented 1 year ago

✅ (partially) Reproduced on 11.5.0 ☑️ Similar behavior on 11/dev (e263db7) ☑️ Similar behavior on 12 ☑️ Similar behavior on 10

Strange behaviour confirmed for attribute_Current attribute_RedirectPage demo_RedirectPage test_RedirectPage

The others seem to work as you described

attribute_RedirectUrl attribute_RedirectUrl

bad_attribute_Current bad_attribute_Current

bad_attribute_RedirectPage bad_attribute_RedirectPage

bad_demo_RedirectPage bad_demo_RedirectPage

bad_test_RedirectPage bad_test_RedirectPage

demo_Current demo_Current

demo_RedirectUrl demo_RedirectUrl

test_Curren test_Current

test_RedirectUrl test_RedirectUrl

Migaroez commented 1 year ago

@jhenkes As I am not 100% sure on some of the expected behavior, I will take this up with the team next Monday.

jhenkes commented 1 year ago

@Migaroez Any updates on this issue and when it might see a fix? It's currently a blocker in my projects and I'd like to know if I should start looking/working towards a workaround (basically not using forms in UmbracoPageController) or if a fix might be nearby.

Migaroez commented 1 year ago

Hey @jhenkes I'm sorry for the late reply, we will be taking this up in a future sprint, but we currently do not have an exact time.

jhenkes commented 9 months ago

Hey @Migaroez Do you have an estimate by now when this problem will be tackled? It'd be enough to know if we're talking about weeks or months so that I know if I have to workaround this issue or if I can wait it out. Thanks in advance!

Migaroez commented 9 months ago

I would say months as we are currently focusing on getting the new management api/backoffice ready for v14.