Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BeginUmbracoForm in custom routes still doesn't work #13836

Closed
callumbwhyte opened this issue Feb 15, 2023 · 6 comments · Fixed by #13910
Closed

BeginUmbracoForm in custom routes still doesn't work #13836

callumbwhyte opened this issue Feb 15, 2023 · 6 comments · Fixed by #13910

Comments

@callumbwhyte
Copy link
Contributor

callumbwhyte commented Feb 15, 2023

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

10.4.0 / 11.1.0

Bug summary

As raised in #13017 and #13333 BeginUmbracoForm doesn't work in custom routes.

This was supposedly fixed in #13103 and released in v10.4.0 and v11.1.0, however this isn't quite the case...

Attempting to use post to a SurfaceController in a custom route will result in an exception.

Specifics

The exception that is thrown states the route's controller can't be found in the DI container.

image

UmbracoVirtualPageRoute calls GetRequiredService on the controller type. This throws an exception if the service can't be found. As far as I understand, controllers of type UmbracoPageController aren't automatically registered so this will blow up in most cases... Clearly another approach is needed to get an instance of the controller type.

For now this can be worked around by registering the controller in the DI container:

builder.Services.AddTransient<MyUmbracoPageController>();

This allows the form to be submitted and breakpoints in the SurfaceController are hit 🙌

However, returning CurrentUmbracoPage() from inside the SurfaceController does not work – the page just refreshes and all modelstate is lost. This is particularly unhelpful when it's necessary to return a validation message or set something in TempData.

Further investigation has shown this works correctly only when an [HttpGet] attribute is added to the route's target action.

Steps to reproduce

Code samples to replicate can be found in the description of #13103.

Upon copying the exact code into a v10.4.0 / v11.1.0 project and submitting the form, the DI error is present.

Removing the [HttpGet] attribute from the UmbracoPageController actions means the page just refreshes, modelstate is lost, etc.

Expected result / actual result

It shouldn't be a requirement to register controllers in DI or apply specific HTTP verbs to the outer page controller in order to post to a SurfaceController within a custom route.


This item has been added to our backlog AB#26881

@github-actions
Copy link

Hi there @callumbwhyte!

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 🤖 🙂

@Zeegaan Zeegaan added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Feb 20, 2023
@bergmania
Copy link
Member

Hi @callumbwhyte..

Thanks for reporting. I understand why this not work. Sadly we was hit by our development environment actually adds the controllers to DI to detect ambiguous constructors 🤦‍♂️ .

Ill make a fix that fall back to use ActivatorUtilities if the service cannot be resolved from DI.

@callumbwhyte
Copy link
Contributor Author

Thanks @bergmania!

What about the case with requiring [HttpGet] for CurrentUmbracoPage to work? This wasn't previously the case in <V9...

@bergmania
Copy link
Member

@callumbwhyte
I'll look into the the other thing with [HttpGet], but Im not sure I completely understand it.

I found another problem, when submitting a form using HttpGet to a surface controller from a page with virtual route, it did not hit the surface controller (because the virtual route has higher priority). Not sure if this is the same, but I doubt. But I can't seem to replicate the other problem.

@huwred
Copy link

huwred commented Mar 7, 2023

don't know if this helps at all, but I was able to get around this issue by overiding the action in htmlattributs in the form definition

@using (Html.BeginUmbracoForm<MembershipSurfaceController>("SubmitSigninForm", null, new { id = "submitsigninform", @action = "/" }))

@callumbwhyte
Copy link
Contributor Author

callumbwhyte commented Mar 7, 2023

@bergmania If my virtual route action looks like this:

public IActionResult MyRoute()
{
    return View(...);
}

...and my surface controller does this:

[HttpPost]
public IActionResult HandleForm()
{
    return CurrentUmbracoPage();
}

...it's as if the page has been loaded blank, i.e. with no state. TempData, ModelState etc are all as if the page has been loaded from scratch.

Adding [HttpGet] to that virtual route, and it all works again.

I think you've hit the issue there with the priority thing. After return CurrentUmbracoPage the HTTP verb is still a POST and the request probably still includes the Umbraco Request for the surface controller.

bergmania added a commit that referenced this issue Mar 7, 2023
Now surface controller requests always has higher priority than other. So if this is both a surface controller request and a virtual page request, it will execute the surface controller.
bergmania added a commit that referenced this issue Mar 8, 2023
* Fixed issue mentioned in #13836

Now surface controller requests always has higher priority than other. So if this is both a surface controller request and a virtual page request, it will execute the surface controller.

* Moved some login into private methods

* More clean up

* Clean up

* Only handle valid candidates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants