Skip to content
This repository was archived by the owner on Dec 14, 2018. It is now read-only.

Potential clarity problems with IUrlHelper #4245

Closed
strohhut opened this issue Mar 7, 2016 · 2 comments
Closed

Potential clarity problems with IUrlHelper #4245

strohhut opened this issue Mar 7, 2016 · 2 comments

Comments

@strohhut
Copy link

strohhut commented Mar 7, 2016

This might be nitpicking or totally irrelevant, or I just don't get it but maybe this can provide some helpful input so here we go.

IUrlHelper

Why is IUrlHelper not called something like IUriProvider when it can return not only urls but also absolute paths (=uri). Many people mix the terms uri and url but usually url is a subset of uri and means absolute uris.
Also there is the UriHelper class in the HttpAbstractions namespace. The way to distinguish between those two would be based on a false premise of IUrlHelper. The existing UriHelper only has extension methods, so the class name won’t be used that often but this might still cause some confusion.

IUrlHelper "main" methods

There is IUrlHelper.Action vs IUrlHelper.RouteUrl. Both generate uris as I understand it. Only one has an Url suffix.

IUrlHelper.Link

I find this method very confusing. It sounds like now all of a sudden html a-tags come into play. I thought we are dealing with addresses here. Then reading the comment I see that it generates an absolute uri. Ah ok, wouldn’t that be a fit for UrlHelperExtensions then? Wait there is already

string RouteUrl(this IUrlHelper helper, string routeName, object values)

vs.

string Link(string routeName, object values);

Which one do I use?

IUrlHelper.IsLocalUrl

When reading the method name I think: "OK, I can pass any uri (because otherwhere the term url is also used for uris) and then I know if that uri points to a resource of my web app (may this resource really exist or not). After a quick glance at the implementation I thought that maybe this method is mixing up internal vs outgoing uris and absolute vs relative uris. But it turns out it doesn't deal with urls (in the correct sense) at all. Other methods of the type seem to understand the concept of hosting context. For instance I can get me an url when I don't provide the host:

Url.Action("MyAction", "My", null, "http");

gives http://localhost:1507/my/myaction

Now IsLocalUrl is completely different. It only checks if the provided string is an absolute path. What makes matters even worse it considers both / and ~/ as local. So now all of a sudden virtual paths come into play (since ~ shouldn't occur in real addresses)? The method is called IsLocalUrlbut deals with absolute (possibly virtual) paths. The method cannot say at all if an url is "local" or not. Why even throw in a method that is so different. The other methods are about providing uris based on mvc concepts like controllers + actions or routes. Why is a method that has no knowledge of the hosting context in a service type. Why do I need a service for this static functionality?

UrlHelperExtensions xml docs

All methods have the comment „Generates a fully qualified or absolute URL“. Whether it’s an absolute path or an url depends on the overload. The comment is only true for the IUrlHelper interface.

HttpAbstractions

I also opened a somewhat related issue for UriHelper which I find even more confusing.

@danroth27
Copy link
Member

The UrlHelper property on Controller is intended to mirror the API from ASP.NET MVC 5 so that porting code to ASP.NET Core MVC is straightforward. While the API names may not be perfect, we prefer to leave them unchanged for this reason.

That said, the doc comments should be cleaned up.

@dougbu
Copy link
Member

dougbu commented Jul 5, 2016

15f25d5

@dougbu dougbu closed this as completed Jul 5, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants