1

Closed

T4MVC ActionLink helper does not work with MVC 4 beta

description

T4MVC generates code to invoke the wrong method overload for the ActionLink extensions that take an htmlAttributes parameter.

Although the links are correct, attributes with underscores are not correctly transformed to use dashes before being rendered (e.g. data_id does not become data-id).

I've modified my local .tt file with a fix. You can see the code changes made here: http://stackoverflow.com/questions/9593631/is-usage-of-html-5-data-attributes-broken-in-asp-net-mvc-4-beta
Closed Mar 10, 2012 at 8:01 AM by davidebbo
Closing again. Now about that beer... :)

comments

davidebbo wrote Mar 7, 2012 at 5:11 AM

I took a quick look at the StackOverflow question. So the problem is that doing 'new RouteValueDictionary(htmlAttributes)' doesn't quite do the right thing?

davidebbo wrote Mar 7, 2012 at 5:11 AM

Also, could the 1st T4MVC overload call the 2nd?

mertner wrote Mar 7, 2012 at 5:32 AM

Yes, it would appear to be the case. The transformation inside MVC probably occurs when then processing the properties on the htmlAttributes object, rather than during rendering, since in a dictionary there would be no need to encode dashes as underscores.

davidebbo wrote Mar 7, 2012 at 6:04 AM

Seems your change is not quite correct, as your second overload ignores a bunch of params (protocol/hostname/fragment). Could you fix that up in your SO answer and do some basic testing? Then I'll try to get that in T4MVC soon. Thanks!

mertner wrote Mar 7, 2012 at 6:39 AM

Fixed. Incidentally, the code in the second overload is now identical to the unmodified code in the method just below it (in the T4MVC.tt file). As such, I don't think it needs any additional testing.

mertner wrote Mar 7, 2012 at 6:42 AM

Come to think of it, isn't the first overload superfluous? The signature is identical to the second overload if you ignore the optional parameters. I'd suggest to remove it entirely and only fix the second overload.

davidebbo wrote Mar 7, 2012 at 7:17 AM

Ok, fixed in 2.6.69, and up on nuget. Would be great if you could verify. I yanked the first overload as you suggested, as it indeed seemed superfluous.

mertner wrote Mar 7, 2012 at 8:08 AM

Thanks, awesome response times here. I'll update later today and let you know.

mertner wrote Mar 7, 2012 at 8:11 AM

PS: Just had a look at the changeset diff and will buy you a beer if that doesn't work :)

davidebbo wrote Mar 7, 2012 at 5:10 PM

Ok, just reopen if you run any issues.

** Closed by davidebbo 03/07/2012 10:10AM

mertner wrote Mar 7, 2012 at 9:33 PM

Ok, so where would you like the beer sent? :o)

The correct code (which does exactly what MVC does internally) for the method should be:
    public static MvcHtmlString ActionLink(this HtmlHelper htmlHelper, string linkText, ActionResult result, object htmlAttributes, string protocol = null, string hostName = null, string fragment = null) {
        return htmlHelper.RouteLink(linkText, null, protocol, hostName, fragment, result.GetRouteValueDictionary(), HtmlHelper.AnonymousObjectToHtmlAttributes(htmlAttributes));
    }
Apologies for submiting an untested fix, wont happen again.

davidebbo wrote Mar 9, 2012 at 4:57 PM

Oh darn, I totally missed your comment and someone reported the break: http://stackoverflow.com/questions/9636859/t4mvc-creates-incorrect-url-for-actionlink-when-using-areas/9638737

I'll try the new change.

davidebbo wrote Mar 9, 2012 at 5:16 PM

Crap, looks like AnonymousObjectToHtmlAttributes was added in Mvc3, so this would break anyone still using Mvc2. We could do some conditional logic based in 'if (MvcVersion >= 3)'. There is similar condition logic for RenderAction (a few lines down from that code), except in that case it was between Mvc 1 and 2.

davidebbo wrote Mar 9, 2012 at 7:53 PM

Ok, 2.7 is out with the fix. I also removed legacy MVC/Fx support per twitter poll. http://twtpoll.com/rjr914

mertner wrote Mar 12, 2012 at 1:03 PM

Sorry, was away for a few days and so couldn't participate here. Glad to see the proper fix get merged :)

That said, a few comments:
  • The superfluous overload made its way back into the .tt file (you based the change on 2.6.68)
  • There are still places where you're doing the wrong thing (search for "new RouteValueDictionary(htmlAttributes)" to find them). The same fix should be applied to all instances in the code file.
Let me know if you ever come to Copenhagen, and I'll have a cold beer waiting for you :)

davidebbo wrote Mar 12, 2012 at 4:47 PM

I don't think the superfluous overload came back. We used to have 4, and we now have 3, same as in the broken fix.

mertner wrote Mar 13, 2012 at 11:20 PM

You are indeed right - the overload is gone. Reading diffs must not be my strong side ;)

I pinpointed the remaining methods that should have the same fix applied:
line 120 (BeginForm)
line 156 (ActionLink)
line 168 (BeginForm)

In all cases the fix can be applied by searching for "new RouteValueDictionary(htmlAttributes)" and replacing it with "HtmlHelper.AnonymousObjectToHtmlAttributes(htmlAttributes)".

Also, in 2.7.0 the unused definition for HtmlStringType (around line 560) can be removed (removes a warning when compiling too).

davidebbo wrote Mar 14, 2012 at 12:33 AM

To avoid any typos on my part, if you already have the file, could you attach it here (or send pull request)? I'll get this in the next build.

mertner wrote Mar 14, 2012 at 12:05 PM

I'm not sure if you really want my local version. It's reformatted to use tab indents and includes a few helper methods that I like to have in my controllers (although they no longer work in MVC 4, so maybe I should get rid of them again).

I'm a bit pressed for time but can prepare you a cleanly modified file later this week, assuming you're not in a rush to get the changes.

davidebbo wrote Mar 14, 2012 at 5:35 PM

No, no rush at all, as no one else has run into this yet that I know of.

Sign in to add a comment