Added default `UrlResolutionTagHelper` to resolve app relative URLs. #2875

NTaylorMullen · 2015-07-28T23:20:54Z

Razor removed the ability to automatically resolve URLs prefixed with ~/; therefore ScriptTagHelper, LinkTagHelper and ImageTagHelper have changed to take in IUrlHelpers and auto-resolve their URL based properties if they start with ~/.
Added a catch-all ~/ resolver for non TagHelper based HTML elements. Razor used to resolve any attribute value that started with ~/ now the behavior is restricted to attributes that can contain URLs.
Updated IUrlHelper to accept null values.
Added functional tests to validate that URLs resolve correctly.
Updated TagHelper tests to ensure that URLs are resolved via an IUrlHelper.
Add URL resolving TagHelper to replace Razor ~/ functionality. #2807

Razor PR to remove existing app-relative URL resolution: aspnet/Razor#466

NTaylorMullen · 2015-07-28T23:21:01Z

ajaybhargavb · 2015-07-28T23:41:11Z

src/Microsoft.AspNet.Mvc.Core/UrlHelper.cs

+            {
+                return null;
+            }
+
            return GenerateClientUrl(_httpContext.Request.PathBase, contentPath);


Not sure if we need to fix this here, but one of the reasons for #2841 is that GenerateClientUrl isn't trimming the contentPath so a leading space will result in ~ not being removed.
Do we want to fix that here or leaving the trimming up to caller?

Hmm, part of me feels like it'd be too protective over users; but then again I can't imagine a user purposefully calling into this method with an untrimmed value.

I'm not sure change this method directly would be the right approach; however, it brings up some gray area in the UrlResolutionTagHelper where it is smarter about parsing URLs with whitespace where our ScriptTagHelper, LinkTagHelper and ImageTagHelper take a users value verbatim and hand it over to the IUrlHelper.

dougbu · 2015-07-29T06:27:45Z

⌚

NTaylorMullen · 2015-07-30T19:55:49Z

Updated.

dougbu · 2015-07-30T21:28:23Z

src/Microsoft.AspNet.Mvc.Razor/TagHelpers/UrlResolutionTagHelper.cs

+                        if (TryResolveUrl(
+                                (string)attribute.Value,
+                                tryEncodeApplicationPath: false,
+                                resolvedUrl: out resolvedUrl))


Fixed one bit of "bit off" formatting but not the other...

NTaylorMullen · 2015-07-30T22:01:18Z

Looks like some tests broke after rebasing, fixed them and the formatting issue.

dougbu · 2015-08-02T04:25:08Z

src/Microsoft.AspNet.Mvc.Razor/TagHelpers/UrlResolutionTagHelper.cs

+    /// <see cref="ITagHelper"/> implementation targeting elements containing attributes with URL expected values.
+    /// </summary>
+    /// <remarks>Resolves application relative URLs that are not targeted by other <see cref="ITagHelper"/>s. Runs
+    /// prior to other <see cref="ITagHelper"/>s to ensure application-relative URLs are resolved.</remarks>


As discussed elsewhere, "application relative" is not consistent with how that term is used elsewhere. But also have no existing doc comments hyphenating "application-relative".

This doesn't matter.

Here "Resolves application relative URLs" -> "Resolves URLs relative to the application's 'webroot' setting" or (perhaps better) "Resolves URLs starting with '~/' (relative to the application's 'webroot' setting)".

dougbu · 2015-08-02T06:16:47Z

⌚

Eilon · 2015-08-02T18:36:06Z

src/Microsoft.AspNet.Mvc.Razor/Resources.resx

@@ -195,4 +195,9 @@
  <data name="RazorPage_InvalidTagHelperIndexerAssignment" xml:space="preserve">
    <value>Unable to perform '{0}' assignment. Tag helper property '{1}.{2}' must not be null.</value>
  </data>
+  <data name="CouldNotResolveApplicationRelativeUrl_TagHelper" xml:space="preserve">
+    <value>Unexpected return value from '{1}.{2}' for URL '{0}'. If you have overridden the '{1}' service, change '{2}' to replace only the '~/' prefix. Otherwise, add the following directive to your Razor page to disable application-relative URL resolution:


Error messages shouldn't assign blame. Remove all the "you" stuff and make it objective: "If the blah service has been overridden... blah blah. Add the Foo directive to the Razor page...."

NTaylorMullen · 2015-08-03T06:17:32Z

Updated.

dougbu · 2015-08-03T16:46:18Z

src/Microsoft.AspNet.Mvc.Razor/Resources.resx

@@ -195,4 +195,9 @@
  <data name="RazorPage_InvalidTagHelperIndexerAssignment" xml:space="preserve">
    <value>Unable to perform '{0}' assignment. Tag helper property '{1}.{2}' must not be null.</value>
  </data>
+  <data name="CouldNotResolveApplicationRelativeUrl_TagHelper" xml:space="preserve">
+    <value>Unexpected return value from '{1}.{2}' for URL '{0}'. If the '{1}' service has been overridden, change '{2}' to replace only the '~/' prefix. Otherwise, add the following directive to the Razor page to disable application-relative URL resolution:


Discussed the now-hidden thread with @Eilon offline. Decided to avoid using "application-relative" in a new way.

Here "application-relative URL resolution" -> "URL resolution relative to the application's 'webroot' setting". Might want to pass "webroot" into the resource.

dougbu · 2015-08-03T17:24:16Z

⌚

NTaylorMullen · 2015-08-03T21:58:22Z

Updated.

dougbu · 2015-08-03T22:12:42Z

src/Microsoft.AspNet.Mvc.Razor/TagHelpers/UrlResolutionTagHelper.cs

+        /// <summary>
+        /// Resolves and updates URL values relative to the application's 'webroot' setting (values starting with '~/')
+        /// for <paramref name="output"/>'s <see cref="TagHelperOutput.Attributes"/> whose
+        /// <see cref="TagHelperAttribute.Name"/> is <paramref name="attributeName"/>.


Wording in class' <remarks> are clearer e.g. Resolves and updates URL values starting with '~/' (relative to the application's 'webroot' setting) for ....

dougbu · 2015-08-03T22:38:05Z

Fix #2903 in this PR. As discussed in a hidden thread, currently doing extra work to preserve a bug.

dougbu · 2015-08-03T22:38:11Z

⌚

NTaylorMullen · 2015-08-03T22:44:07Z

Updated.

Eilon · 2015-08-03T22:46:23Z

Let's not add more to this PR at this point. It's fine to treat other items as bugs.

dougbu · 2015-08-03T22:54:10Z

src/Microsoft.AspNet.Mvc.Razor/TagHelpers/UrlResolutionTagHelper.cs

+        /// <param name="encodeWebRoot">If <c>true</c>, will encode the path to the application's root defined by the
+        /// 'webroot' setting.</param>
+        /// <param name="resolvedUrl">The URL value relative to the application's 'webroot' setting.</param>
+        /// <returns><c>true</c> if the <paramref name="url"/> could be resolved; <c>false</c> otherwise.</returns>


Sorry for commenting on a commit (amateur move). My comments were on this set of doc comments.

dougbu · 2015-08-03T22:56:40Z

w/ last couple of doc comment fixes

- Razor removed the ability to automatically resolve URLs prefixed with `~/`; therefore `ScriptTagHelper`, `LinkTagHelper` and `ImageTagHelper` have changed to take in `IUrlHelper`s and auto-resolve their URL based properties if they start with `~/`. - Added a catch-all `~/` resolver for non `TagHelper` based HTML elements. Razor used to resolve any attribute value that started with `~/` now the behavior is restricted to attributes that can contain URLs. - Updated `IUrlHelper` to accept `null` values. - Added functional tests to validate that URLs resolve correctly. - Updated `TagHelper` tests to ensure that URLs are resolved via an `IUrlHelper`. #2807

dnfclas added the cla-already-signed label Jul 28, 2015

NTaylorMullen mentioned this pull request Jul 28, 2015

Remove ~/ handling from core Razor. aspnet/Razor#466

Merged

ajaybhargavb reviewed Jul 28, 2015
View reviewed changes

NTaylorMullen force-pushed the nimullen/urlresolutionth.2807 branch from ee91fee to bd29dd9 Compare July 30, 2015 19:55

dougbu reviewed Jul 30, 2015
View reviewed changes

dougbu reviewed Aug 2, 2015
View reviewed changes

Eilon reviewed Aug 2, 2015
View reviewed changes

NTaylorMullen force-pushed the nimullen/urlresolutionth.2807 branch from 3401afe to 61a0884 Compare August 3, 2015 06:16

dougbu reviewed Aug 3, 2015
View reviewed changes

NTaylorMullen force-pushed the nimullen/urlresolutionth.2807 branch from 61a0884 to c2d6bc7 Compare August 3, 2015 21:57

dougbu reviewed Aug 3, 2015
View reviewed changes

NTaylorMullen force-pushed the nimullen/urlresolutionth.2807 branch from c2d6bc7 to dd048d2 Compare August 3, 2015 22:41

dougbu reviewed Aug 3, 2015
View reviewed changes

NTaylorMullen force-pushed the nimullen/urlresolutionth.2807 branch from dd048d2 to 0ef68ee Compare August 3, 2015 23:18

NTaylorMullen merged commit 0ef68ee into dev Aug 3, 2015

NTaylorMullen deleted the nimullen/urlresolutionth.2807 branch August 3, 2015 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added default `UrlResolutionTagHelper` to resolve app relative URLs. #2875

Added default `UrlResolutionTagHelper` to resolve app relative URLs. #2875

NTaylorMullen commented Jul 28, 2015

NTaylorMullen commented Jul 28, 2015

ajaybhargavb Jul 28, 2015

NTaylorMullen Jul 28, 2015

dougbu commented Jul 29, 2015

NTaylorMullen commented Jul 30, 2015

dougbu Jul 30, 2015

NTaylorMullen commented Jul 30, 2015

dougbu Aug 2, 2015

Eilon Aug 2, 2015

dougbu Aug 3, 2015

dougbu commented Aug 2, 2015

Eilon Aug 2, 2015

NTaylorMullen commented Aug 3, 2015

dougbu Aug 3, 2015

dougbu commented Aug 3, 2015

NTaylorMullen commented Aug 3, 2015

dougbu Aug 3, 2015

dougbu commented Aug 3, 2015

dougbu commented Aug 3, 2015

NTaylorMullen commented Aug 3, 2015

Eilon commented Aug 3, 2015

dougbu Aug 3, 2015

dougbu commented Aug 3, 2015

Added default UrlResolutionTagHelper to resolve app relative URLs. #2875

Added default UrlResolutionTagHelper to resolve app relative URLs. #2875

Conversation

NTaylorMullen commented Jul 28, 2015

NTaylorMullen commented Jul 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Jul 29, 2015

NTaylorMullen commented Jul 30, 2015

Choose a reason for hiding this comment

NTaylorMullen commented Jul 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Aug 2, 2015

Choose a reason for hiding this comment

NTaylorMullen commented Aug 3, 2015

Choose a reason for hiding this comment

dougbu commented Aug 3, 2015

NTaylorMullen commented Aug 3, 2015

Choose a reason for hiding this comment

dougbu commented Aug 3, 2015

dougbu commented Aug 3, 2015

NTaylorMullen commented Aug 3, 2015

Eilon commented Aug 3, 2015

Choose a reason for hiding this comment

dougbu commented Aug 3, 2015

Added default `UrlResolutionTagHelper` to resolve app relative URLs. #2875

Added default `UrlResolutionTagHelper` to resolve app relative URLs. #2875