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

[6.x] Use CommonMark For Mailable #30982

Merged
merged 6 commits into from Dec 30, 2019
Merged

[6.x] Use CommonMark For Mailable #30982

merged 6 commits into from Dec 30, 2019

Conversation

@taylorotwell
Copy link
Member

taylorotwell commented Dec 30, 2019

This switches Markdown Mailable parsing to CommonMark instead of Parsedown. The main reason for this is better granularity of security related features in CommonMark, which allows us to prevent unsafe links in user input (a user input with the content: "a") without needing to prevent all HTML tags in general which would break our current mailable system.

I think it would be good to do this basically security related change on 6.x because it is an LTS.

@GrahamCampbell GrahamCampbell changed the title Use CommonMark For Mailable [6.x] Use CommonMark For Mailable Dec 30, 2019
@taylorotwell taylorotwell merged commit 9308a4e into 6.x Dec 30, 2019
2 of 4 checks passed
2 of 4 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/styleci/push The analysis has passed
Details
@driesvints driesvints deleted the commonmark branch Dec 30, 2019
@shengslogar

This comment has been minimized.

Copy link

shengslogar commented Jan 14, 2020

It's worth noting that (new Parsedown())->text('#Header') (note the missing space after the octothorpe) will "correctly" translate to <h1>Header</h1>, while (new League\CommonMark\CommonMarkConverter())->convertToHtml('#Header') translates to <p>#Header</p>.

The correct syntax, of course, is # Header (with the added space), and will translate properly with both libraries.

There are also other small nuanced differences. Parsedown translates - to <p>-</p>, while CommonMark translates - to <ul>\n<li></li>\n</ul>\n.

@driesvints

This comment has been minimized.

Copy link
Member

driesvints commented Jan 14, 2020

@shengslogar actually, that's not correct. Any hashtag character with no spaces after it is interpreted as a regular hashtag. See the spec here: https://spec.commonmark.org/0.29/#example-34

Otherwise you could never start a sentence with a hashtag. This was something that Parsedown incorrectly rendered.

Edit: just realized you probably meant the same by putting "correctly" between quotes.

@shengslogar

This comment has been minimized.

Copy link

shengslogar commented Jan 14, 2020

@driesvints Right on. I slipped up in a project, which broke when I updated Laravel, but you're absolutely correct regarding the spec. Figured my comment might give pointers to someone with a similar issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.