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

Markdown parser fails in content file when using a content variable as the url #992

Open
mjauvin opened this issue Oct 13, 2023 · 10 comments
Assignees

Comments

@mjauvin
Copy link
Member

mjauvin commented Oct 13, 2023

Winter CMS Build

dev-develop

PHP Version

8.1

Database engine

MySQL/MariaDB

Plugins installed

No response

Issue description

The new markdown parser fails in a contend file when using a content variable in place of a static url.

Steps to replicate

Using this markup in a CMS page:

{% content "test-content.md" url="https://wintercms.com" %}

And this content file:

[click here]({url})

Workaround

Using <a href="{url}">click here</a> tag in the content file works.

@mjauvin mjauvin added Type: Unconfirmed Bug needs review Issues/PRs that require a review from a maintainer labels Oct 13, 2023
@LukeTowers
Copy link
Member

LukeTowers commented Oct 14, 2023

Do the variables need to be parsed before the markdown gets parsed?

@mjauvin
Copy link
Member Author

mjauvin commented Oct 14, 2023

It used to work, so I would say yes.

@LukeTowers
Copy link
Member

@bennothommo any thoughts?

@bennothommo
Copy link
Member

I assume the new Markdown parser is probably applying some escaping on the URLs for links, whereas the old Parsedown library didn't. I do feel that those variables should be parsed before Markdown however, so if that's not the case, I'll fix that.

@mjauvin
Copy link
Member Author

mjauvin commented Oct 16, 2023

@bennothommo You must have had really good reasons to change the parser, but it has created quite a bit of BC... and now you're saying it can't even be extended easily because of final class/methods ?

@bennothommo
Copy link
Member

@mjauvin I'd argue it's only been edge cases so far. Given how prevalent it's used in the Blog and Docs plugins, it's been surprisingly smooth if I'm being honest, especially since Parsedown was very loose with its following of standards whilst CommonMark is more strict.

@mjauvin
Copy link
Member Author

mjauvin commented Oct 16, 2023

Seems I've been hitting all those "edge cases" myself... ;)

@bennothommo
Copy link
Member

True - which means either you have very bad luck, or people are silent on the matter. But either way, I still feel it was a net positive. 😛

@LukeTowers LukeTowers added Status: In Progress and removed Type: Unconfirmed Bug needs review Issues/PRs that require a review from a maintainer labels Oct 19, 2023
@mjauvin mjauvin added this to the 1.2.6 milestone Mar 5, 2024
@LukeTowers LukeTowers modified the milestones: 1.2.6, 1.2.7 Apr 25, 2024
@LukeTowers
Copy link
Member

@mjauvin is this still an issue for you?

@LukeTowers LukeTowers removed this from the 1.2.7 milestone Jul 22, 2024
@mjauvin
Copy link
Member Author

mjauvin commented Jul 22, 2024

@LukeTowers it still is, but I'm now using Winter.Parsedown plugin to revert to the old parser when needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants