[3.1.x] Markdown rendering regression

[GwendolenLynch]

Noticed today by @PhillippOhlandt that bolt.cm wasn't rendering Markdown as HTML, rather just plain text MD source.

Reverting to 3.0.x-dev or earlier fixes problem.

Details

• Relevant Bolt Version: 3.1.0+
• Install type: any

[bobdenotter]

This is prolly related to my changes in how filtering and sanitization is enforced since 3.1.0. Will look into it.

[bobdenotter]

I can't really reproduce this.. If I make a simple content type, like this:

dummy:
    name: Dummies
    singular_name: Dummy
    fields:
        title:
            type: text
            class: large
            group: "Block"
        slug:
            type: slug
            uses: [ title ]
        content:
            type: markdown
            height: 150px

Both of these will produce proper Markdown output:

    {{ record.content|markdown }}

    {{ fields() }}

What happens on bolt.cm, is because we're doing {{ record|excerpt(600) }}. As far as I know, 'excerpt' has never parsed markdown, right?

[PhillippOhlandt]

@bobdenotter do we really use the excerpt() filter on the newsitem page? that doesn't make much sense.

[PhillippOhlandt]

And, it worked before, so something broke in the codebase.

[bobdenotter]

@PhillippOhlandt Yes. we use Excerpt. And I'm trying to thing how "excerpt" could've used correct HTML before. Are you 100% positive it did before? I don't see recent changes in that page:

https://github.com/bolt/site-v30/commits/master/public/theme/bolt-v300/news.twig

[PhillippOhlandt]

Thats the newsitem listing page. But also all newitems were broken and there we shouldn't use excerpt().

[PhillippOhlandt]

https://github.com/bolt/site-v30/blob/master/public/theme/bolt-v300/record.twig#L29-L32

[bobdenotter]

@PhillippOhlandt Euhm. What? I don't see what's broken?

[PhillippOhlandt]

When Gawain updated the site to use the latest 3.1.x, all newsitems showed the raw markdown text.

[bobdenotter]

Oooooooh, right. I think i get it now.. currently in 3.1.0, you need to do {{ record.foo|markdown }}, but before it used to output markdown when just doing {{ record.foo }} if "foo" happened to be a markdown field.

[PhillippOhlandt]

Ah, that explains it. So it's more our fault because we went the lazy route and didn't use the proper filter?

[bobdenotter]

@rossriley Can you help out, here? I can't find the place where this magic happens anymore.

[bobdenotter]

One way to fix this, would be to re-introduce this code to \Legacy\Content.php:

                case 'markdown':
                    // Parse the field as Markdown, return HTML
                    $value = $this->app['markdown']->text($this->values[$name]);
                    $value = $this->preParse($value, $allowtwig);
                    $value = new \Twig_Markup($value, 'UTF-8');

                    break;

                case 'html':

(see #5611)

But, this is probably not the best way going forward.

[bobdenotter]

Note: What happened on bolt.cm was not related to this issue. That was #5601 instead.

[SvanteRichter]

Quick question: Wouldn't this lead to any theme that "adapted to the new way" would double-parse the markdown?

[bobdenotter]

@SahAssar Yes, but if they've changed their template this quickly without problems, I'm sure they'll be able to revert that. (I'd think)

[CarsonF]

Yeah I'm wondering if this would double parse too...

[bobdenotter]

It will, but only if somebody themselves added {{ record.markdownfield|markdown() }} to the template.

There's really no good way to check for that, so they'll just have to revert that. I think the impact will be low though, because it's not like any actual data is lost.
So far, we've bumped into it on bolt.cm, and had no reports from other people about it.

[CarsonF]

Is there a way that the markdown filter could check for markdown source and otherwise pass through?

[bobdenotter]

Not in a foolproof way. We could "guess" if something is markdown, by checking for HTML tags, but even valid markdown might have embedded tags. So, no.

[CarsonF]

Maybe we could wrap the rendered markdown content in a class with a __toString method?

[bobdenotter]

That's not unlike what Ross suggested we do in the (near) future. For right now we should just fix the bug we introduced in 3.1.0, and be done with it, imho.

[CarsonF]

Bokay. I'm not looped in enough to know if would be BC break. 🙊 😶