Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tpl/tplimpl: Fix double-escaping in opengraph template #12425

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

jmooring
Copy link
Member

  • Do not escape HTML entities
  • Use consistent formatting for title and description with opengraph, schema, and twitter_cards
  • Improve readability of twitter_cards

Closes #12418

@@ -1,14 +1,14 @@
<meta property="og:url" content="{{ .Permalink }}">

{{- with or site.Title site.Params.title | plainify }}
{{- with or site.Title site.Params.title | .RenderString | plainify | htmlUnescape }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we focus on fixing #12418 and not add other not obviously related.

I assume the HTML gets escaped the second time in

content="{{ . }}"

Where is the first escape?

Also, a test for #12418 would help.

@jmooring
Copy link
Member Author

Where the escapes coming from?

Front matter:

description = "This is <em>emphasized</em> and this is **bold**, but it's been stripped."

Passing description through .RenderString renders the single quote to &rsquo;.
Passing that through plainify renders &rsquo; to &amp;rsquo;.

I can split this into three PR's each with their own issue:

  1. Fix Twitter og:description doubly escaped by Hugo 0.125.x #12418
  2. Fix the same problem with Page.Title, Site.Title, and Site.Params.title in the opengraph template (Add plainify to Title - in internal template opengraph.html #8698)
  3. Fix the same problems in schema.html and twitter_cards.html, and improve readability of twitter_cards.html at the same time

I will add a test for #12418, and add additional conditions to that test with each subsequent PR.

Believe me, I am as fatigued by this as you are.

@bep
Copy link
Member

bep commented Apr 24, 2024

Passing description through .RenderString renders the single quote to ’.

.RenderString wasn't in the template before this PR, was it? Where is the double escape in the below?

{{- with or .Description .Summary site.Params.description | plainify }}
  <meta property="og:description" content="{{ . }}">
{{- end }}

Using Markdown syntax in title: e.g. sounds foreign to me and adding .RenderString all over the place is something we need to discuss. I have been known to overthink things in the performance department, but .RenderString isn't particulary light weight.

I suggest that we fix the bug with as little change as possible. People can create their own templates if they need special things.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this @jmooring!
See inline for one nit.

tpl/tplimpl/embedded/templates/opengraph.html Show resolved Hide resolved
@jmooring
Copy link
Member Author

jmooring commented Apr 24, 2024

@bep

Where is the double escape in the below?

When og:description is derived from .Summary, either automatic or with summary divider, e.g.

---
title: p1
---

Test line with a single quote: can't.
<!--more-->

There are three test cases that I need to include, with examples here:
#12418 (comment)

We've got way too many ways to define Summary, each with their own idiosyncrasies:
#8910 (comment)

@bep
Copy link
Member

bep commented Apr 24, 2024

@jmooring OK,

So,

{{- with or site.Title site.Params.title | plainify | htmlUnescape }}

Etc. should work (??) and is perfectly fine.

Adding .RenderString to the mix needs a discussion (it will certainly slow down asciidoc builds ...)

@jmooring
Copy link
Member Author

OK, we pass markdown through un-rendered. To handle the manual summary divider in one of the (now) 5 test cases, we need to chomp the trailing newline:

{{- with or site.Title site.Params.title | plainify | htmlUnescape | chomp }}

Hopefully that's OK.

@jmooring jmooring force-pushed the improve-embedded-templates-12418 branch from 71b7282 to e918e70 Compare April 24, 2024 20:52
@jmooring jmooring changed the title tpl/tplimpl: Improve embedded templates tpl/tplimpl: Fix double-escaping in opengraph template Apr 24, 2024
@bep bep merged commit fb51b69 into gohugoio:master Apr 25, 2024
8 checks passed
@jmooring jmooring deleted the improve-embedded-templates-12418 branch April 25, 2024 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Twitter og:description doubly escaped by Hugo 0.125.x
3 participants