This repository has been archived by the owner. It is now read-only.

ImageTagHelper #2516

Merged
merged 1 commit into from May 11, 2015

Conversation

Projects
None yet
5 participants
@dpaquette
Copy link
Contributor

dpaquette commented May 7, 2015

An ImageTagHelper that supports globbed src paths and cache busting file
version hash
[Resolves #2249]

@dnfclas

This comment has been minimized.

Copy link

dnfclas commented May 7, 2015

Hi @dpaquette, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

@dpaquette

This comment has been minimized.

Copy link
Contributor

dpaquette commented May 8, 2015

What's the deal with @dnfclas ? I signed my cla before submitting this PR

@dougbu

This comment has been minimized.

Copy link
Member

dougbu commented May 8, 2015

@dpaquette We just changed the repos from MS Open Tech to .NET Foundation so there's a different CLA. The new CLA is simpler than the old one and so should only take a few moments to re-sign. Apologies for the extra bit of work!

@dnfclas

This comment has been minimized.

Copy link

dnfclas commented May 8, 2015

@dpaquette, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

@dnfclas dnfclas added cla-signed and removed cla-required labels May 8, 2015

/// If <c>true</c> then a query string "v" with the encoded content of the file is added.
/// </remarks>
[HtmlAttributeName(FileVersionAttributeName)]
public bool? FileVersion { get; set; }

This comment has been minimized.

@Eilon

Eilon May 8, 2015

Member

IncludeFileVersion? Or maybe AppendFileVersion? The current name makes it sound like it is a file version (e.g. you'd set asp-file-version="1.23.4" or something).

This comment has been minimized.

@dpaquette

dpaquette May 8, 2015

Contributor

I agree that AppendFileVersion or AddFileVersion would make more sense. This is modeled off the Link and Script tag helpers. If we change the name here then it would need to be changed on both those tag helpers too.

This comment has been minimized.

@NTaylorMullen

NTaylorMullen May 10, 2015

Member

Why nullable? It's in the TargetElement so it'll always be provided.

ModeAttributes.Create(Mode.FileVersion, new[] { FileVersionAttributeName }),
};

private enum Mode

This comment has been minimized.

@Eilon

Eilon May 8, 2015

Member

A single-value enum is... weird. Is it supposed to be more like None=0, FileVersion=1 or something? Not sure I quite get how this works.

This comment has been minimized.

@dpaquette

dpaquette May 8, 2015

Contributor

This could be a boolean now. It previously had multiple modes back when this supported src globbing.

@dpaquette

This comment has been minimized.

Copy link
Contributor

dpaquette commented May 9, 2015

@Eilon I simplified the ImageTagHelper class by removing the Mode and Mode Detection code since this was no longer needed. Not sure what we should do about the naming of the asp-file-version attribute.

private FileVersionProvider _fileVersionProvider;

/// <summary>
/// Src of the image.

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Source

BuildImageTag(attributes, builder);
} else
{
//No attributes matched so we have nothing to do

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

else on its own line and always include a space after //

// "src" values come from bound attributes and globbing. So anything but a non-null string is
// unexpected but could happen if another helper targeting the same element does something odd.
// Pass through existing value in that case.
var attributeStringValue = attributeValue as string;

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

This isn't necessary since Src is bound.

builder
.Append(attribute.Name)
.Append("=\"")
.Append(HtmlEncoder, ViewContext.Writer.Encoding, attributeValue)

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

For every attribute except Src, this usually double-encode the attribute value.


[Activate]
[HtmlAttributeNotBound]
protected internal ILogger<ImageTagHelper> Logger { get; set; }

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

unused


[Activate]
[HtmlAttributeNotBound]
public IJavaScriptStringEncoder JavaScriptEncoder { get; set; }

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

unused

/// <inheritdoc />
public override void Process(TagHelperContext context, TagHelperOutput output)
{
// Pass through attribute that is also a well-known HTML attribute.

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Entire method could be significantly simplified since <link> and <script> tag helpers only need SetContent() workaround due to globbing. For example:

public override void Process(TagHelperContext context, TagHelperOutput output)
{
    if (Src != null)
    {
        if (FileVersion == true)
        {
            EnsureFileVersionProvider();
            output.Attributes[SrcAttributeName] = _fileVersionProvider.AddFileVersionToPath(Src);
        }
        else
        {
            // Pass through attribute that is also a well-known HTML attribute.
            output.CopyHtmlAttribute(SrcAttributeName, context);
        }
    }
}
@@ -162,6 +162,11 @@ public IActionResult Link()
return View();
}

public IActionResult Image()

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Need another test in MvcTagHelpersTest. This addition isn't confirmed otherwise.

This comment has been minimized.

@dougbu
@dougbu

This comment has been minimized.

Copy link
Member

dougbu commented May 10, 2015

⌚️

@dpaquette

This comment has been minimized.

Copy link
Contributor

dpaquette commented May 10, 2015

@dougbu Simplified implementation and updated tests as per your comments

using Microsoft.Framework.Runtime;

using Moq;
using Xunit;

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Please remove and sort usings. That should also clean up the extra blank line.

/// Passed through to the generated HTML in all cases.
/// </remarks>
[HtmlAttributeName(SrcAttributeName)]
public string Src { get; set; }

This comment has been minimized.

@NTaylorMullen

NTaylorMullen May 10, 2015

Member

Don't bind this and just add it to the [TargetElement], Aka [TargetElement("img", Attributes = FileVersionAttributeName + ",src")]. That way you don't have to restore it or check for null.

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

@NTaylorMullen your advice is incorrect. This is the way to ensure the tag helper gets the non-HTML encoded value of the src attribute.

This comment has been minimized.

@NTaylorMullen

NTaylorMullen May 10, 2015

Member

Ah, fair point since we're modifying it. 👍

This comment has been minimized.

@NTaylorMullen

NTaylorMullen May 10, 2015

Member

Probably still worth adding it to TargetElement since it no-ops otherwise. Also, since it's a string a null value wont be possible (for the TagHelper) if its in the TargetElement.

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

since it's a string a null value wont be possible if its in TargetElement.

Side note: Put that way, it sounds like a bug. Razor doesn't do what author likely meant if author writes src="@null" for a bound string attribute.

But I agree having both SrcAttributeName and FileVersionAttributeName in the TargetElement.Attributes value makes sense. Avoids some useless executions of the helper.

@@ -47,6 +47,8 @@ public class MvcTagHelpersTest
[InlineData("Link", null)]
// Testing the ScriptTagHelper
[InlineData("Script", null)]
//Testing the ImageTagHelper

This comment has been minimized.

@NTaylorMullen

NTaylorMullen May 10, 2015

Member

// Testing the ImageTagHelper

{ "data-extra", new HtmlString("something") },
{ "title", new HtmlString("Image title") },
{ "src", "testimage.png" },
{ "asp-file-version", "true" }

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Bit unrealistic. src and asp-file-version are not in the TagHelperOutput.Attributes collection by default. Bound attributes appear only in the TagHelperExecutionContext.AllAttributes collection when tag helper execution begins. That's why the CopyHtmlAttribute() call is necessary in the <img> tag helper.

This comment has been minimized.

@dougbu

dougbu May 11, 2015

Member

Thanks. output looks good now.

HostingEnvironment = hostingEnvironment,
ViewContext = viewContext,
Src = "/bar/images/image.jpg",
FileVersion = true,

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Need a test with FileVersion == false to confirm CopyHtmlAttribute() is called correctly.

This comment has been minimized.

@dougbu
@NTaylorMullen

This comment has been minimized.

Copy link
Member

NTaylorMullen commented May 10, 2015

⌚️

{ "data-extra", new HtmlString("something") },
{ "title", new HtmlString("Image title") },
{ "src", "testimage.png?v=f4OxZX_x_FO5LcGBSKHWXfwtSx-j1ncoSt3SABJtkGk" },
{ "asp-file-version", "true" }

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

in realistic scenario asp-file-version would not appear in the Attributes collection after processing

Assert.True(output.Content.IsEmpty);
Assert.Equal("img", output.TagName);
Assert.Equal(2, output.Attributes.Count);
TagHelperAttribute srcAttribute = Assert.Single(output.Attributes, attr => attr.Name.Equals("src"));

This comment has been minimized.

@dougbu
Assert.True(output.Content.IsEmpty);
Assert.Equal("img", output.TagName);
Assert.Equal(2, output.Attributes.Count);
TagHelperAttribute srcAttribute = Assert.Single(output.Attributes, attr => attr.Name.Equals("src"));

This comment has been minimized.

@dougbu

This comment has been minimized.

@dpaquette

dpaquette May 10, 2015

Contributor

just to confirm...we can use var in tests but shouldn't in other code?

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

No. Our Guidelines say "The var keyword is to be used as much as the compiler will allow." (Everywhere.)

This comment has been minimized.

@dpaquette

dpaquette May 10, 2015

Contributor

My bad. For some reason I read that part of the guideline as 'is not to be used'.


private static TagHelperContext MakeTagHelperContext(
TagHelperAttributeList attributes = null,
string content = null)

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Simplify method signature: No need for attributes to be optional. No need for content at all.

});
}

private static TagHelperOutput MakeImageTagHelperOutput(TagHelperAttributeList attributes = null)

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

No need for attributes to be optional.

return hostingEnvironment.Object;
}

private static IApplicationEnvironment MakeApplicationEnvironment(string applicationName = "testApplication")

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Unused; remove.

This comment has been minimized.

@dpaquette

dpaquette May 10, 2015

Contributor

I'm wondering what this. It is also unused in the test for the Link tag helper

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Might have been needed at some point there. But you're not using it here.

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Please remove this method.

return applicationEnvironment.Object;
}

private static IMemoryCache MakeCache(object result = null)

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Remove result parameter. No caller passes an argument.

@dougbu

This comment has been minimized.

Copy link
Member

dougbu commented May 10, 2015

⌚️ very close

@dpaquette

This comment has been minimized.

Copy link
Contributor

dpaquette commented May 10, 2015

@dougbu @NTaylorMullen Thanks for the review. I have updated based on your feedback. In retrospect, basing this implementation on the Link and Script Tag Helpers was probably a bad idea 😄

using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

This comment has been minimized.

@dougbu

dougbu May 10, 2015

Member

Sorry, should have mentioned that we use the old VS default here: System namespaces first.

This comment has been minimized.

@dougbu

dougbu May 11, 2015

Member

Thanks

@dougbu

This comment has been minimized.

Copy link
Member

dougbu commented May 10, 2015

⌚️

@@ -47,6 +47,8 @@ public class MvcTagHelpersTest
[InlineData("Link", null)]
// Testing the ScriptTagHelper
[InlineData("Script", null)]
// Testing the ImageTagHelper
[InlineData("Image", null)]

This comment has been minimized.

@dougbu

dougbu May 11, 2015

Member

Odd that MvcTagHelpersWebSite.MvcTagHelper_Home.Image.html shows up here as a binary file. Anything unusual about it on your end? (Content looks fine when I click on View.)

This comment has been minimized.

@dpaquette

dpaquette May 11, 2015

Contributor

Looks line an encoding problem. I will try saving as UTF-8

@dougbu

This comment has been minimized.

Copy link
Member

dougbu commented May 11, 2015

:shipit:

Please rebase your branch to latest dev then squash your commits down to one (git rebase -i head~8 and change pick to s or squash for all commits except the first. We won't lose attribution when merging if you do that.

/// <inheritdoc />
public override void Process(TagHelperContext context, TagHelperOutput output)
{
if (FileVersion == true)

This comment has been minimized.

@NTaylorMullen

NTaylorMullen May 11, 2015

Member

if (FileVersion)

@NTaylorMullen

This comment has been minimized.

Copy link
Member

NTaylorMullen commented May 11, 2015

:shipit:

ImageTagHelper
An ImageTagHelper that supports cache busting by appending a file
version hash to the image src attribute
[Resolves #2249]

Code cleanup
@dpaquette

This comment has been minimized.

Copy link
Contributor

dpaquette commented May 11, 2015

@dougbu @NTaylorMullen Thanks guys. I did a rebase to latest dev and squashed my commits.

@Eilon

This comment has been minimized.

Copy link
Member

Eilon commented May 11, 2015

BTW I logged #2540 to track changing the name of the FileVersion property.

@dougbu dougbu merged commit ab4d2ee into aspnet:dev May 11, 2015

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor Waiting for AppVeyor build to complete
Details
@dougbu

This comment has been minimized.

Copy link
Member

dougbu commented May 11, 2015

ab4d2ee Thanks for your contribution!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.