Fix html encoding in LiquidViewFilters

[hishamco]

Fixed #7497

@jagbarcelo could please try the PR

[jptissot]

You should use the AmbientValues of the context to get a service. Not the ShellScope.

Look at the ShortCodeFilter for an example.

[hishamco]

You should use the AmbientValues of the context to get a service. Not the ShellScope.

Is there any difference?

[jptissot]

I am not aware what the difference is, but it removes the need to depend on the ShellScope. I can only assume it allows for use in different contexts and also better testing (without having to implement a shell scope).

[jagbarcelo]

I've applied the same changes (including the use AmbientValues) to my dev machine and it still shows the same output:

I think it is being encoded twice, thus the & instead of a simple &. Might it be re-encoded elsewhere, up in the pipeline?

[jtkech]

Before the regression, shape_render and shape_stringify were both returning a StringValue, not encoded for shape_render, and encoded for shape_stringify, that's why i still encoded it in my PR, Notice that StringValue is encoded by default so no need to encode it again. After the regression, shape_render and shape_stringify were both returning an IHtmlContent and in fact were doing exactly the same thing.

So currently, when shape_stringify is used for direct rendering there are 2 workarounds

In place of using shape_stringify we can just use shape_render that is doing the same thing as the previous stringify

<title>{{ "PageTitle" | shape_new | shape_render }}</title>

Or add the raw filter to prevent double encoding

<title>{{ "PageTitle" | shape_new | shape_stringify | raw }}</title>

Then we would have to update the layout in our themes.

---

But stringify is intended to be used as a string not for direct rendering, e.g. to be passed as a parameter to another shape, so maybe before the regression there was another issue by encoding it. So, finally here i suggest to just replace

                value = new StringValue(writer.ToString());

With

                value = new StringValue(writer.ToString(), false);

I tried it, it works for the related issue.

[sebastienros]

I agree with the fact that stringify, if it's returning html, should not be encoded. But not sure that the result string should know about it, or actually its usage in the title and then use | raw. Need to find other examples of stringify, and what the doc states. Or watch the meeting where it was introduced. I assume I wrote this one, or you maybe JT?

[Skrypt]

@sebastienros It was written because we had issues with the Lucene/SQL Query templates (liquid). We needed something to get a raw value from a form posted value.

[sebastienros]

Take a look at this sample:

OrchardCore/src/OrchardCore.Themes/TheBlogTheme/Views/Content-BlogPost.Summary.liquid

Lines 12 to 13 in 95b3301

	{% assign dateTime = "DateTime" | shape_new: utc: Model.ContentItem.CreatedUtc, format: format | shape_stringify %}
	{{ "Posted by" | t }} <a href="#">{{ Model.ContentItem.Owner }}</a> {{ "on {0}" | t: dateTime | raw }}

It's using shape_stringify and then calls | raw of the result. So it's expecting it to be html, but not to be a StringValue(encode: false).

Update:
Well it's a translation argument ... might be even more complex

[jtkech]

Yes i saw this and tried it, here even we change it to StringValue(..., false) we still need the raw filter

Keep in mind that before #7463 that i did to fix an issue comented on gitter, shape_render and shape_stringify were doing exactly the same thing, this after this commit 44754f6#diff-8141fd33486da211684152b5d06ea47dc755a10fd523d5fceeff0f0e8ff2d112, so at least one of them was useless.

Before the above commit there were both reurning a StringValue but shape_stringify was (maybe wrongly) encoded, that's why i kept it encoded in #7463

[hishamco]

If no encoding is required I will close this PR

[jtkech]

@sebastienros about {{ "on {0}" | t: dateTime | raw }}

Well it's a translation argument ... might be even more complex

Yes just tried, in fact here the raw is not applied to the dateTime argument, but to the whole, here this is the t filter that encodes, anyway the t filter uses the inner _value of the passed argument regardless it is marked to be encoded.

shape_stringify should not be marked to be encoded as it is the representation of a shape, so normally already encoded, hmm, so not sure that the result of a shape stringify is a good candidate for a t parameter, as it is already encoded, that's why here we need anyway the raw filter.

shape_render return an HtmlContentValue holding a ViewBuffer whose WriteTo() doesn't encode again, that's why it was working for #7497 with the previous shape_stringify, before i did #7463, because it was doing exactly the same thing, so useless.

But there was another issue on gitter where someone really want to work with the returned value expecting it is a string (not a ViewBuffer), as it was before this commit (LiquidViewFilters.cs line 75)

That's why i did #7463 to really return a string, but unfortunately before the above commit shape_stringify was wrongly returning a string marked to be encoded again, that's why i just did the new #7503 PR to fix it.

I think there are other filters that wrongly return a string marked to be encoded, i'm thinking about those that generate urls, maybe currently it works because the browser is aware of this and html decodes them before url encoding them.

[agriffard]

Fixed by #7503