Include the PluginController Area when searching for matching surface…

[vwa-software]

Prerequisites

Create an Controller derived from the SurfaceController, decoratie it with the PluginController attribute like this:

Then create a form with the BeginUmbracoForm method, make sure the area is filled.

This throws an exception with the message that the controller could not be found. This is true, becouse the area is not included in the route parameters searching for the controller.

(Also the typed version of the BeginUmbracoForm method is broken.)

This patch fixes this.

[nathanwoulfe]

@nul800sebastiaan would you treat this as a breaking change? PR modifies the interface, but does so with an optional parameter so won't break any implementations... Feels like it should be ok...

[KevinJump]

In framework (and I can't find anything to suggest this has changed in .net core but it might have).
adding optional parameters makes breaking change for code already compiled against the existing interface.

Optional parameters don't really exist at the compiled level and the code using them is essentially compiled down to something with the default value in the argument list. Existing code built against the old method, will not have the correct method signature internally. if it is rebuilt against the new interface it will 'just work' but only because the compiler will add the extra parameters.
(i've been stung by this one in the past #5822)

so from a compatibility point of view its better to keep the old method there, and call the new one.

 public ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action)
    => ControllerActionDescriptor(httpContext, controller, action, null) ; 
    
 public ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action, string area = null)

but doing this does then break the interface as it has a new method it it. - You could use a default interface methods to fix that, but it might be a bit ugly/

[nul800sebastiaan]

Not just the interface also a public constructor. Optional parameters are a binary breaking change so any packages/custom code built against this ControllerActionDescriptor method will need to recompile.

What we usually do in this case is add a method that overloads the existing one, make the old one obsolete with a message telling people to use the method with the added parameter, and redirect the method call to the new method.

However.. that would mean adding the method to the interface too, which would again be a breaking change. And then we'd need to add a new interface IControllerActionSearcher2 which inherits IControllerActionSearcher and has the additional method.

It gets a bit ugly but it will work and not break anything for anyone!

[nul800sebastiaan]

Haha, didn't see @KevinJump's comment while I was posting, but glad you agree! 😁

[nathanwoulfe]

Magical feedback, thanks both.

I've seen the IMyInterface2 pattern elsewhere in the codebase, bit messy but easy to merge the two interfaces in a later version...

Long story short though, sounds like a breaking change however we chop it...

[KevinJump]

Default interface methods isn't something i've used a lot, but in theory, it means you can keep one interface.

adding the new method, but with a default calling the old one.

 ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action, string area)
     => Find<T>(httpContext, controller, action) ; 

would mean that that bit isn't breaking 🤷 - because the interface is still implimented by everything and new methods can override this default

so in the ControllerActionSearcher class, it would then be the other way around, the original method would call the new method.

 public ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action)
    => ControllerActionDescriptor<T>(httpContext, controller, action, null) ; 
    
 public ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action, string area) .... 

[nul800sebastiaan]

Ah yes we have default interfaces now, I think it would be interesting to experiment with. The one thing I'm a bit unsure about is that we can't just add an [Obsolete] on the method in the interface then as there is no alternative. At least we can do it on IControllerActionSearcher2 saying it will be merged into IControllerActionSearcher eventually, signposting a breaking change to come. 🤔

[KevinJump]

I think you can add [obsolete] because the new method will also be there. ?

so [Obsolete("use the method that accepts the area.... this one will be removed")] ?

[nul800sebastiaan]

@KevinJump Just trying to wrap my head around it, so in order for the interface not to change, we'd use a default interface, meaning the current interface would change from:

public interface IControllerActionSearcher
{
    ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action);
}

to:

public interface IControllerActionSearcher
{
    ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action, string area)
     => Find<T>(httpContext, controller, action) ; 
}

Ah.. so maybe I don't understand how a default interface would help here. We're still changing the method signature and adding a parameter, isn't that a breaking change?

default interface newbie here 🙋

[KevinJump]

Hi, the interface gets a new property so has two methods in it.

public interface IControllerActionSearcher
{
    ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action);

    ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action, string area)
     => Find<T>(httpContext, controller, action) ; 
}

but because the second one has a default implementation anything implementing the interface doesn't have to have to define this method.

in this instance you then do the opposite in the class, where the old method, calls the new one.

 public ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action)
    => ControllerActionDescriptor<T>(httpContext, controller, action, null) ; 
    
 public ControllerActionDescriptor Find<T>(HttpContext httpContext, string controller, string action, string area) .... 

so anything still calling the old method will call the new one with area = null

[nul800sebastiaan]

🤓 Excellent, gotcha!

Sorry for the long chat here @vwa-software but I hope we're all learning! Would you be able to make the changes as @KevinJump is suggesting please? Then we can move on with this PR! 👍

[vwa-software]

Don't feel sorry, i am so glad it is taken seriously, and i learn a lot as wel. Thank you.
I made a new commit with the sugested implementation, i hope it's correct, if not please tell.

[umbrabot]

Hi there @vwa-software, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[nul800sebastiaan]

One more tip for the future to make our lives easier, screenshots of code are not great, I had to figure out which code I needed to write to do a most minimal test for this, of course the screenshots helped, but a copy/paste of code would have been a lot quicker. So for reference, this is the code I used:

View:

@using(Html.BeginUmbracoForm(action: "AddBasket", controllerName:"Basket", area: "Amazilia"))
{
    <input type="submit" />
}

Controller:

using Microsoft.AspNetCore.Mvc;
using Umbraco.Cms.Core.Cache;
using Umbraco.Cms.Core.Logging;
using Umbraco.Cms.Core.Routing;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Web;
using Umbraco.Cms.Infrastructure.Persistence;
using Umbraco.Cms.Web.Website.Controllers;

namespace Umbraco.Cms.Web.UI.Controllers
{
    [Area("Amazilia")]
    public class BasketController : SurfaceController
    {
        public BasketController(
            IUmbracoContextAccessor umbracoContextAccessor,
            IUmbracoDatabaseFactory databaseFactory,
            ServiceContext services,
            AppCaches appCaches,
            IProfilingLogger profilingLogger,
            IPublishedUrlProvider publishedUrlProvider)
            : base(umbracoContextAccessor, databaseFactory, services, appCaches, profilingLogger, publishedUrlProvider)
        {
        }

        public IActionResult AddBasket()
        {
            return CurrentUmbracoPage();
        }
    }
}

Do note: I used the Area attribute here instead of PluginController - both work but if you're not building a plugin then I think Area is recommended. I don't actually know what the benefits/differences between Area and PluginController are these days though.

Anyway, as I said, both will work now and they definitely do throw the error that you saw before this PR.

So all good now, merging! 👍

Thanks again for the help and your patience, and congrats on your shiny new contributor badge on Our https://our.umbraco.com/members/id:274727/ 🎉