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

Markdownify and .Summary sometimes produce an unclosed <p> tag #7372

Closed
AlanLynn opened this issue Jun 8, 2020 · 10 comments
Closed

Markdownify and .Summary sometimes produce an unclosed <p> tag #7372

AlanLynn opened this issue Jun 8, 2020 · 10 comments

Comments

@AlanLynn
Copy link

AlanLynn commented Jun 8, 2020

What version of Hugo are you using?

$ hugo version
Hugo Static Site Generator v0.72.0/extended windows/amd64 BuildDate: unknown

To reproduce:

  1. Add the following to the top of a content .md file.
+++
summary = """
## Header

Paragraph of text
"""
+++
  1. View the rendered summary.

Expected result:

<h2 id="header">Header</h2>
<p>Paragraph of text</p>

Actual result:

The closing </p> tag for the paragraph is missing.

<h2 id="header">Header</h2>
<p>Paragraph of text

Additional bug?

When a single line summary is provided, no <p></p> tag is rendered at all. It's possible that this is the expected behavior, but it feels incorrect to me. A user could very easily make the following mistake, thinking that paragraph tags would be added in both examples.

summary = """
Single paragraph
"""
summary = """
Paragraph 1

Paragraph 2
"""

If that is the expected behavior, is there at least a way to ensure that the summary is always processed by markdown?

@AlanLynn
Copy link
Author

AlanLynn commented Jun 9, 2020

I found the problematic function: TrimShortHTML in helpers/content.go

// TrimShortHTML removes the <p>/</p> tags from HTML input in the situation
// where said tags are the only <p> tags in the input and enclose the content
// of the input (whitespace excluded).
func (c *ContentSpec) TrimShortHTML(input []byte) []byte {
	firstOpeningP := bytes.Index(input, paragraphIndicator)
	lastOpeningP := bytes.LastIndex(input, paragraphIndicator)

	lastClosingP := bytes.LastIndex(input, closingPTag)
	lastClosing := bytes.LastIndex(input, closingIndicator)

	if firstOpeningP == lastOpeningP && lastClosingP == lastClosing {
		input = bytes.TrimSpace(input)
		input = bytes.TrimPrefix(input, openingPTag)
		input = bytes.TrimSuffix(input, closingPTag)
		input = bytes.TrimSpace(input)
	}
	return input
}

Commit history:

But before we think about fixing the TrimShortHTML function, can we take a minute to ask: Should the function even be used? To me, it feels like a major gotcha that the summary would only sometimes be HTML (see my thoughts above). What exactly is the purpose of removing <p></p> from a single paragraph? And is that something that can't be solved by having the user use plainify as needed?

@AlanLynn
Copy link
Author

AlanLynn commented Jun 10, 2020

Curiously, I noticed that TrimShortHTML is also used in the Markdownify function. To me, that feels even more presumptive of the user's intentions.

@divinerites
Copy link

divinerites commented Jun 12, 2020

for mardownify I ended up with this systematic code in order to get valid HTML syntax and consistent css behaviour. I guess you can adapt for summary.
But i agree that is not consistent as a default behaviour.

{{ with .description }}
      {{ $markdown := . | markdownify }}
      {{ if not ( strings.Contains $markdown "<p>" ) }}
            <p>{{ $markdown }}</p>
      {{ else }}
            {{ $markdown }}
      {{ end }}
{{ end }}

I do that <p> test with my markdownified variables and also .Content and .Summary even they are already markdown (see comment below)

@divinerites
Copy link

also see #7069

@divinerites
Copy link

divinerites commented Jun 12, 2020

One of the problem is that .Content and .Summary are markdown (see Bep's comment here #2616 (comment)).

But when we have one line in .Content or in .Summary, there is no <p> tag, so it mess often with our css styles.
So we are driven to use markdownify to have a consistent <p> css behaviour.
Then this problem.
Then my systematic hack.

@AlanLynn
Copy link
Author

AlanLynn commented Jun 29, 2020

Clearly I am not the only one frustrated by markdownify and .Summary producing sometimes HTML.

See also:

The average person expects a function named markdownify to do a standard markdown filter. Why? Because the function name is a verbified form of "markdown", suggesting that it applies a markdown filter. Nothing else is in the name, and so nothing else should be in the code. The idea that functions should do what they say (and nothing else) is fundamental to software development. That's because if a function does more than its name says, it gives developers an unpleasant surprise, often discovered long after the fact.

As a parallel, imagine if you ran this code: fmt.println("Hello, world!"), and it outputted Hello, world!. (with a period added at the end). You might think: "What?? Why is it doing that? I didn't tell it to put a period at the end!" That's because the function did more than it said it would. Then you are forced to search documentation and read through the code to figure out what is happening. And you discover a line in the code that says "Add a period to the end of sentences that don't have them, for good grammar". And you think "What? I didn't tell it to do that! Why is it assuming what I want? How do I undo this unwanted behavior?" And that is exactly the process that many people have gone through, and will continue to go through, if markdownify and .Summary are left as is.

Having markdownify and .Summary go through anything but a standard markdown filter is unexpected as a default behavior. The default should be the expected (standard markdown), and anything else should need to be explicitly configured, either through a setting or a function parameter.

@AlanLynn AlanLynn changed the title Rendered front matter summary is sometimes missing a closing </p> tag Markdownify and .Summary sometimes produce an unclosed <p> tag Jun 29, 2020
@hollowaykeanho
Copy link

Replicate this issue consistently with markdownify.

When feeding the following with markdownify $html | safeHTML:

### My Card Title Here
This can be written in Markdown where it will be processed into HTML
automatically.

I got a missing </p> with its generated HTML output:

<h3 id="my-card-title-here">My Card Title Here</h3>
<p>This can be written in Markdown where it will be processed into HTML
automatically.

I'm on:

Hugo Static Site Generator v0.78.2-959724F0 linux/amd64 BuildDate: 2020-11-13T10:08:14Z

@jpinto3488
Copy link

Running

Hugo Static Site Generator v0.80.0-792EF0F4 windows/amd64 BuildDate: 2020-12-31T13:37:57Z

I confirm markdownify on a single line text parameter will not wrap the content into <p></p>

Having following in front-matter:

zone2Content: |-
    Lorem *ipsum* dolor sit amet

On the layout I'm doing the following:

{{ .Params.zone2Content | markdownify }}

The result is:

Lorem <em>ipsum</em> dolor sit amet

Expected result would be:

<p>Lorem <em>ipsum</em> dolor sit amet</p>

note: if there are multiple lines, the

tags are being added correctly. The problem occurs only when there's a single line of content.

@github-actions
Copy link

github-actions bot commented Mar 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. The resources of the Hugo team are limited, and so we are asking for your help.
If this is a bug and you can still reproduce this error on the master branch, please reply with all of the information you have about it in order to keep the issue open.
If this is a feature request, and you feel that it is still relevant and valuable, please tell us why.
This issue will automatically be closed in the near future if no further activity occurs. Thank you for all your contributions.

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants