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

Last closing round bracket is stripped out from the href #1459

Closed
teimurjan opened this issue Apr 1, 2019 · 9 comments · Fixed by #1509
Closed

Last closing round bracket is stripped out from the href #1459

teimurjan opened this issue Apr 1, 2019 · 9 comments · Fixed by #1509
Labels
category: links L2 - annoying Similar to L1 - broken but there is a known workaround available for the issue

Comments

@teimurjan
Copy link

Describe the bug

The last ) is stripped out from the href when rendering HTML.

It works fine on CommonMark.

To Reproduce
Steps to reproduce the behavior:

marked receives the following content: testText https://test.test?_g=(refreshInterval:(display:Off,pause:!f,value:0),time:(from:'2015-10-11T11:08:59.548Z',mode:absolute,to:'2015-10-11T11:23:59.548Z'))&_a=(columns:!(URL),index:logs_web_error,interval:auto,,sort:!(TimeStamp,desc)). Instead of having the anchor element with this whole link I get:

<a href="https://test.test?_g=(refreshInterval:(display:Off,pause:!f,value:0),time:(from:'2015-10-11T11:08:59.548Z',mode:absolute,to:'2015-10-11T11:23:59.548Z'))&_a=(columns:!(URL),index:logs_web_error,interval:auto,,sort:!(TimeStamp,desc)">https://test.test?_g=(refreshInterval:(display:Off,pause:!f,value:0),time:(from:'2015-10-11T11:08:59.548Z',mode:absolute,to:'2015-10-11T11:23:59.548Z'))&_a=(columns:!(URL),index:logs_web_error,interval:auto,,sort:!(TimeStamp,desc)</a>)</p>

The last ) is lost from the href somehow.

  1. Marked Demo
  2. CommonMark Demo

Expected behavior
Expected not to have the last ) stripped from href.

@teimurjan teimurjan changed the title Last ) is stripped out from the href Last closing round bracket is stripped out from the href Apr 1, 2019
@teimurjan
Copy link
Author

The problem caused by the usage of this regex.

Testing the Regex.

@teimurjan
Copy link
Author

If someone could explain me what is the purpose of this regexp, I'd fix the issue by myself

@UziTech
Copy link
Member

UziTech commented Apr 1, 2019

The purpose of that is to prevent urls from including punctuation.

e.g.

(https://example.com)
My website is https://example.com.
https://example1.com, https://example2.com, https://example3.com

)., in those examples should not be part of the url

It is very difficult to determine what should be and what should not be part of a url.

My website is https://example.com?web=site!

Should ! be included in the url? Maybe it is part of the query? There is no way of knowing which is why common mark only includes urls surrounded by <> as links.

@UziTech UziTech added category: links L2 - annoying Similar to L1 - broken but there is a known workaround available for the issue labels Apr 1, 2019
@UziTech
Copy link
Member

UziTech commented Apr 1, 2019

The fix would be to wrap your urls with <> demo

@teimurjan
Copy link
Author

@UziTech Thanks, got it. But to be absolutely calm, why is it working at CommonMark?

@UziTech
Copy link
Member

UziTech commented Apr 2, 2019

It's not working on your CommonMark Demo. If you view the html you will see that it doesn't set it as a link because autolinking is in the gfm spec not the commonmark spec.

@UziTech
Copy link
Member

UziTech commented Apr 2, 2019

To be clear, this is actually a bug in marked because matched parenthesis should be part of the link but just changing the _backpedal regex isn't going to fix that. We should be using findClosingBracket similar to links

@tmorehouse
Copy link

tmorehouse commented Jun 30, 2019

I'm seeing similar issues when we convert our changelog to HTML (we are using standard-version to genreate the changelog):

(fixes [#3025](https://github.com/bah/blah/issues/3025)) ([#3029](https://github.com/bah/blah/pull/3029)) ([ad57e8c](https://github.com/bah/blah/commit/ad57e8c))

becomes

(fixes <a href="/bah/blah/issues/3025">#3025</a> (<a href="/bah/blah/pull/3029">#3029</a> (<a href="/bah/blah/commit/ad57e8c">ad57e8c</a>)

which renders visually as

(fixes #3025 (#3029 (ad57e8c)

which is missing the expected closing brackets, and should be rendered as:

(fixes #3025) (#3029) (ad57e8c)

It used to work several versions ago, but I think the issue was introduced in #1435 or #1414

@UziTech UziTech mentioned this issue Jul 1, 2019
4 tasks
@UziTech
Copy link
Member

UziTech commented Jul 1, 2019

@tmorehouse Your case is a little bit different since it is missing a parenthesis around the link not inside the link.

I created PR #1509 to fix this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: links L2 - annoying Similar to L1 - broken but there is a known workaround available for the issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants