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

Fenced code block with list fails #239

Closed
moorereason opened this issue Feb 12, 2016 · 19 comments
Closed

Fenced code block with list fails #239

moorereason opened this issue Feb 12, 2016 · 19 comments
Labels

Comments

@moorereason
Copy link
Contributor

Given the following fenced code block,

1. Foo

    ```
    + bar
    ```

blackfriday renders:

<ol>\n<li><p>Foo</p>\n\n<p>```</p>\n\n<ul>\n<li>bar\n```</li>\n</ul></li>\n</ol>\n

I expect it to render:

<ol>\n<li><p>Foo</p>\n\n<pre><code>+ bar\n</code></pre></li>\n</ol>\n

It appears to occur when the first line of the code block matches a list item pattern (ie. -, +, *, or 1.). If there's a blank line or any non-list item patterned content on the first line, the code fencing works properly.

Here's a test case for TestFenceCodeBlockWithinList:

        "1. Foo\n\n    ```\n    + bar\n    ```\n",
        `<ol>
<li><p>Foo</p>

<pre><code>+ bar
</code></pre></li>
</ol>
`,

I haven't been able to figure out where it's breaking down.

@dynamind
Copy link

I encountered this issue with Jenkins console output, which contains xtrace lines (i.e. shell commands are logged to stdout prefixed by + followed by a space).

Notably, the + character was not on the first line. I've taken the liberty to include the whole thing below.

Started by user anonymous
[Pipeline] Allocate node : Start
Running on master in /var/jenkins_home/jobs/simple-pipeline/workspace
[Pipeline] node {
[Pipeline] sh
[workspace] Running shell script
+ echo 
            echo "Hello from script"
[Pipeline] load: Loaded script: script.groovy
[Pipeline] load {
[Pipeline] echo
Hello from script
[Pipeline] } //load
[Pipeline] } //node
[Pipeline] Allocate node : End
[Pipeline] End of Pipeline
Finished: SUCCESS

@dmitshur
Copy link
Collaborator

Confirmed, this is a bug.

@slohse
Copy link

slohse commented May 25, 2016

I encountered this problem today due to a line in a code section that starts with ": ".

Disclaimer: this morning I knew nothing about the go programming language.
Nonetheless I looked into it a little. I built a test case in TestFenceCodeBlockWithinList very similar to the one @moorereason posted.

--- FAIL: TestFencedCodeBlockWithinList (0.00s)
        block_test.go:65: 
                Input   ["* Foo\n\n    ```\n    : bar\n    ```\n"]
                Expected["<ul>\n<li><p>Foo</p>\n\n<pre><code>: bar\n</code></pre></li>\n</ul>\n"]
                Actual  ["<ul>\n<li><p>Foo</p>\n\n<p>```</p>\n\n<p>: bar\n```</p></li>\n</ul>\n"]

It seems to happen during secondPass, when parsing out lists. I think due to the "list-like" start of the line, it considers the line starting with the offending symbol as another list item. What then happens is that data.len is set in such a way, that

if beg == 0 || beg >= len(data)

in func (p *parser) fencedCode(out *bytes.Buffer, data []byte, doRender bool) int becomes true due to both beg and len(data) being 4.

@kaushalmodi
Copy link

I faced this issue today. The issue occurs specifically when there is Markdown list syntax in a fenced code block within a Markdown list. The parser tries to interpret the hyphens in the code fence to list in HTML.

Looking forward to the fix.

@dmitshur dmitshur added the bug label Aug 2, 2017
kaushalmodi added a commit to kaushalmodi/ox-hugo that referenced this issue Aug 4, 2017
1) If a code block is in a list, and
2) If a line in that code block begins with the regular
   dash (HYPHEN-MINUS) character or a PLUS SIGN (0x2b), replace that
   character with the HYPHEN character (which looks like HYPHEN-MINUS,
   but has a different binary code.. 0x2010 instead of 0x2d). That is
   enough to fool Blackfriday to get around this bug.
kaushalmodi added a commit to kaushalmodi/ox-hugo that referenced this issue Aug 4, 2017
1) If a code block is in a list, and
2) If a line in that code block begins with the regular
   dash (HYPHEN-MINUS) character or a PLUS SIGN (0x2b), replace that
   character with the HYPHEN character (which looks like HYPHEN-MINUS,
   but has a different binary code.. 0x2010 instead of 0x2d). That is
   enough to fool Blackfriday to get around this bug.
@bryfry
Copy link

bryfry commented Jan 27, 2018

This is very frustrating when trying to display YAML files in code blocks inside of numbered lists. Our use case is Lab steps which require sequential numbers, code blocks, and to display YAML files.

Workaround I found in other bugs which reference this issue is to use 8 spaces instead of code fences.

This issue was reported almost 2 years ago; Is there a plan to fix this?

kaushalmodi added a commit to kaushalmodi/ox-hugo that referenced this issue Mar 20, 2018
@ahmetb
Copy link

ahmetb commented Mar 21, 2018

I hit this too, here's a minimal repro:

1. First item

    ```yaml
    ports:
      foo: bar
    ```

2. Second item

    ```yaml
    ports:
    - port: 80
    ```

3. Third item

Both code fences are indented by 4 spaces. The first YAML block under "First item" renders just fine, however the one under "Second item" doesn't, and just prints the raw code fence syntax (picture below). I was able to determine that it's happening when the code block contains an array like above.

image

tfogo added a commit to tfogo/website that referenced this issue May 21, 2018
There is an issue with ordered lists in the Creating HA clusters with
kubeadm setup tutorial. All list items are labeled 1. This is because
the underlying markdown processor for Hugo, blackfriday, requires
list items with multiple paragraphs to have four space indentation.
See (russross/blackfriday#244)

This commit adds an extra space before paragraphs in lists so they are
correctly interpreted as multi-paragraph lists.

However, this exposes a bug in blackfriday where lines beginning with
`-` in codefenced code blocks inside lists are parsed as sub-lists.
This means code fenced code blocks inside lists that contain yaml are
malformed. (See russross/blackfriday#239)

So this commit mitigates this bug in two situations. It uses a 4-space
indented code block in one situation. In another situation where a list
only contained one element, it is no longer a list.
tfogo added a commit to tfogo/website that referenced this issue May 21, 2018
There is an issue with ordered lists in the Creating HA clusters with
kubeadm setup tutorial. All list items are labeled 1. This is because
the underlying markdown processor for Hugo, blackfriday, requires
list items with multiple paragraphs to have four space indentation.
See (russross/blackfriday#244)

This commit adds an extra space before paragraphs in lists so they are
correctly interpreted as multi-paragraph lists.

However, this exposes a bug in blackfriday where lines beginning with
`-` in codefenced code blocks inside lists are parsed as sub-lists.
This means code fenced code blocks inside lists that contain yaml are
malformed. (See russross/blackfriday#239)

So this commit mitigates this bug in two situations. It uses a 4-space
indented code block in one situation. In another situation where a list
only contained one element, it is no longer a list.
@client9
Copy link

client9 commented May 22, 2018

if someone wants to write the test cases mentioned in #372 I can finish the fix. I don't use this package anymore so help appreciated. CC: @tfogo

@tfogo
Copy link
Contributor

tfogo commented May 22, 2018

@client9 thanks for the update. I'm happy to take a stab at it.

tfogo added a commit to tfogo/website that referenced this issue May 23, 2018
There is an issue with ordered lists in the Creating HA clusters with
kubeadm setup tutorial. All list items are labeled 1. This is because
the underlying markdown processor for Hugo, blackfriday, requires
list items with multiple paragraphs to have four space indentation.
See (russross/blackfriday#244)

This commit adds an extra space before paragraphs in lists so they are
correctly interpreted as multi-paragraph lists.

However, this exposes a bug in blackfriday where lines beginning with
`-` in codefenced code blocks inside lists are parsed as sub-lists.
This means code fenced code blocks inside lists that contain yaml are
malformed. (See russross/blackfriday#239)

So this commit mitigates this bug in two situations. It uses a 4-space
indented code block in one situation. In another situation where a list
only contained one element, it is no longer a list.
rtfb pushed a commit that referenced this issue May 24, 2018
Issue #239 codeblock inside list.
@client9
Copy link

client9 commented May 25, 2018

this should be fixed... can @ahmetb @tfogo @moorereason @bryfry @kaushalmodi confirm?

regards,

n

@kaushalmodi
Copy link

@client9 Thanks for the work. I won't be able to test it though, till Hugo moves to Blackfriday v2.

@client9
Copy link

client9 commented May 25, 2018

Oh good point this could be backported to v1

@tfogo
Copy link
Contributor

tfogo commented May 25, 2018

Yeah a backport to v1 would be great for Hugo. I'm happy to put together a PR for it today unless you would like to do it @client9

@client9
Copy link

client9 commented May 25, 2018

@tfogo made a start of it on https://github.com/client9/blackfriday

I get some failures in the tests, but this might mean the test just need adjusting. V1 might work differently.

existing test still pass -- only the new tests have some failures.

--- FAIL: TestFencedCodeBlockWithinList (0.00s)
	block_test.go:64: 
		Input   ["* Foo\n\n    ```\n    bar\n\n    qux\n    ```\n"]
		Expected["<ul>\n<li><p>Foo</p>\n\n<pre><code>bar\n\nqux\n</code></pre></li>\n</ul>\n"]
		Actual  ["<ul>\n<li>Foo\n\n<code>\nbar\n\nqux\n</code></li>\n</ul>\n"]
--- FAIL: TestListWithMalformedFencedCodeBlock (0.00s)
	block_test.go:64: 
		Input   ["1. one\n\n    ```\n    code\n\n2. two\n"]
		Expected["<ol>\n<li>one\n```\ncode\n2. two</li>\n</ol>\n"]
		Actual  ["<ol>\n<li>one\n\n```\ncode\n\n2. two</li>\n</ol>\n"]
	block_test.go:64: 
		Input   ["1. one\n\n    ```\n    - code\n\n2. two\n"]
		Expected["<ol>\n<li>one\n```\n- code\n2. two</li>\n</ol>\n"]
		Actual  ["<ol>\n<li>one\n\n```\n- code\n\n2. two</li>\n</ol>\n"]
FAIL
FAIL	github.com/client9/blackfriday	16.318s

@tfogo
Copy link
Contributor

tfogo commented Jun 11, 2018

Hi @client9 - sorry for the late reply.

I looked into this. Looks like TestFencedCodeBlockWithinList is a test which is in v1 but not v2. It's testing a fenced code block if there is one item in a list. Looks like this has caught an edge case with your fix where the output is different from what is expected.

I believe this input:

* Foo

    ```
    Hello
    ```

should produce this output:

<ul>
<li><p>Foo</p>

<pre><code>Hello
</code></pre></li>
</ul>

However it is producing this:

<ul>
<li>Foo

<code>
Hello
</code></li>
</ul>

It looks like this is because the LIST_ITEM_CONTAINS_BLOCK flag is never set so the list item is run through the inline parser instead of the block parser. See here: https://github.com/client9/blackfriday/blob/d68ad860fb6ced29d8a9448e3da5aa8ad7efa145/block.go#L1279

I've opened a PR to your repo to set the LIST_ITEM_CONTAINS_BLOCK flag if there is a fenced code block in a list item.

As for the TestListWithMalformedFencedCodeBlock test, as @rtfb mentioned in #372 this is undefined behavior and it should be okay as long as it doesn't cut off content. Looks like it's just outputting more newlines so in my PR I just change the expected output of the test.

@tfogo
Copy link
Contributor

tfogo commented Jun 11, 2018

PR here: https://github.com/client9/blackfriday/pull/2

@client9 if it looks good to you, I think we should submit it to @rtfb for approval. There may be some edge case I'm not thinking of here where this isn't the solution we want.

Also, we probably want to add this test case and fix to v2. This test fails on v2. But I'll wait to hear from you and @rtfb if you think this is the right fix.

@tfogo
Copy link
Contributor

tfogo commented Aug 31, 2018

This should now be fixed in both v1 and v2.

@rtfb
Copy link
Collaborator

rtfb commented Aug 31, 2018

This should now be fixed in both v1 and v2.

Just a quick check on what you meant by "should": are we good to close it, or should we still doublecheck whether it helped?

@moorereason
Copy link
Contributor Author

I've confirmed that it's fixed in master and v2. 🎉

@rtfb
Copy link
Collaborator

rtfb commented Aug 31, 2018

Awesome! Thanks for everyone involved!

miekg added a commit to mmarkdown/markdown that referenced this issue Sep 1, 2018
This fixes the same issue, but in this code base. Patching was done
manual, but with minimal effort.

Signed-off-by: Miek Gieben <miek@miek.nl>

full details:

~~~
% wget https://patch-diff.githubusercontent.com/raw/russross/blackfriday/pull/372.patch
...
Saving to: ‘372.patch’
~~~

Edit `372.patch` to fix the path nams -> a/block.go -> a/parser/block.go etc.

~~~
% patch -p1 --dry-run < 372.patch
checking file block_test.go
Hunk #1 succeeded at 1513 with fuzz 2 (offset 55 lines).
checking file parser/block.go
Hunk #1 succeeded at 1571 (offset 325 lines).
Hunk gomarkdown#2 succeeded at 1605 (offset 325 lines).
checking file parser/block.go
Hunk #1 FAILED at 1293.
1 out of 1 hunk FAILED
checking file parser/block.go
Hunk #1 FAILED at 1280.
1 out of 1 hunk FAILED
checking file block_test.go
Hunk #1 succeeded at 1513 with fuzz 2 (offset 44 lines).
~~~

looks promising. Let's do this for real and manual fix.

~~~
% patch -p1 --merge < 372.patch
patching file block_test.go
Hunk #1 merged at 1516-1526.
patching file parser/block.go
patching file parser/block.go
patching file parser/block.go
patching file block_test.go
Hunk #1 merged at 1527-1553.
~~~
miekg added a commit to gomarkdown/markdown that referenced this issue Sep 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants