aspnet / Mvc

[Archived] ASP.NET Core MVC is a model view controller framework for building dynamic web sites with clean separation of concerns, including the merged MVC, Web API, and Web Pages w/ Razor. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
5.61k stars 2.14k forks source link

IUrlHelper.Action should support use of the nameof() operator for controller names #5853

Closed DevJohnC closed 7 years ago

DevJohnC commented 7 years ago

Building URLs from within views, controllers or anywhere else should support the use of the nameof() operator for better compile time safety of generated URLs.

If you provide "HomeController" (ie. the result of nameof(HomeController)) to IUrlHelper or anything that uses it, such as the anchor tag helper, you will not get the URL you would expect. Instead MVC expects you to provide "Home".

Eilon commented 7 years ago

Hi folks, I believe this suggestion has come up a few times before, and while I think many people agree it sounds good, it has never been clear how it should be implemented.

I mean, you actually can use nameof today, it's just a little ugly:

return RedirectToAction(..., nameof(HomeController).Substring(0, nameof(HomeController).Length - "Controller".Length), ...);

Beautiful, right? 😄 Of course, you could encapsulate that string trimming logic into a utility:

return RedirectToAction(..., ToSimpleControllerName(nameof(HomeController)), ...);

But the problem with this in general is that controller names do not need to end with Controller, and that controller names do not have to map to type names either. As such, always chopping off the word Controller from the end of a "controller name" could lead to incorrect behavior.

Doing this automatically in MVC would be troublesome because it would imply a requirement that isn't actually there.

Can anyone share thoughts on how they think this feature could be reasonably implemented?

danroth27 commented 7 years ago

What you really want here is the name of the controller in the MVC sense. The nameof operator will only give you the name of the type. For example, it's totally conceivable that we could have a ControllerNameAttribute at some point, which nameof would have no way to handle. I think what you really want is the value of ControllerModel.ControllerName after all of the conventions have been run, but I don't think we currently hang on to the application model anywhere that you can get at.

@rynowak Thoughts on this?

rynowak commented 7 years ago

What I'd really like is the equivalent of a 'named route' but for conventional routes. It would basically be a combination of route name and route values, as a Func<..., string>.

All of these concerns are fundamentally about avoiding putting the physical name of a controller/action lots of places in code. Having refactoring support is IMO a way to mitigate some of the drawbacks of that choice. It can help if you rename a controller/action because you want to change the URL, but it can't help if you change the purpose of the URL.

For everyone asking for this, sorry to disappoint you but we don't yet have the IDE support to make something like this actually useful (refactoring in Razor, Live Errors in Razor).


It would be better to express a logical name - route names do this pretty well for attribute routes, but don't scale for conventional routes.

You could write something like this yourself, but it's boilerplate and scaffolding doesn't know about it. We'd want to make this first class, maybe using scaffolding to drive it.

public static class RouteTable
{
    public string AccountSignUp(this IUrlHelper url, params ValueTuple<string, object>[] query)
    {
        var values = new RouteValueDictionary();
        values.Add("action", "SignUp");
        values.Add("controller", "Account");
        values.Add("area", null);

        for (var i = 0; i < query.Length; i++)
        {
            values.Add(query.Item1, Convert.ToString(query.Item2) ?? string.Empty);
        }

        return url.Route("default", values);
    }
}
PonchoPowers commented 7 years ago

Does this really have to be that complicated, just update the methods to remove the word Controller from the end of the string so that we can use typeof(Class).

Eilon commented 7 years ago

Closing this issue here because we have some broader ideas we're tracking in the Routing repo: https://github.com/aspnet/Routing/issues/436

Rather than passing in something like nameof(HomeController) and doing string manipulation, we'd rather have typeof(HomeController) passed in, and we can do much smarter matching.