Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#14769 closed Bug (fixed)

URL with '-' in host not detected by autolink plugin

Reported by: mpeuss Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.7.0
Component: General Version: 4.5.0
Keywords: Cc:

Description

Steps to reproduce

  1. enable the autolink plugin
  2. paste a url with a '-' in the hostname (e.g. http://example-site.org/) into the editor text field

Expected result

url string is converted to a clickable link

Actual result

url is not detected

Change History (14)

comment:1 Changed 8 years ago by Jakub Ś

Status: newconfirmed
Version: 4.5.10 (GitHub - master)4.5.0

Problem can be reproduced from CKEditor 4.5.0.

comment:2 Changed 8 years ago by Jakub Ś

#13364 we had similar but it has turned out there was rather a problem in user implementation.

Are you by any chance using any non-standard package with third-party plugins not created by CKSource?

comment:3 Changed 7 years ago by Jakub Ś

Other issues for autolink plugin: #13569, #14769, #16685.

Version 1, edited 7 years ago by Jakub Ś (previous) (next) (diff)

comment:4 Changed 7 years ago by Jakub Ś

#14903 was marked as duplicate.

In search of the perfect URL validation regex might help with finding a good regexp.

comment:5 Changed 7 years ago by Jakub Ś

#16842 was marked as duplicate.

comment:6 Changed 7 years ago by Marek Lewandowski

Milestone: CKEditor 4.7.0

It's a super trivial bug, let's have it fixed.

comment:7 Changed 7 years ago by Tade0

Done.

Changes pushed to branch:t/14769.

comment:8 Changed 7 years ago by Tade0

Added manual test.

Changes pushed to ​branch:t/14769.

comment:9 Changed 7 years ago by Tade0

Owner: set to Tade0
Status: confirmedreview

comment:10 Changed 7 years ago by Tomasz Jakut

Status: reviewpending

The fix fixes the issue with not recognizing links with hyphen in them, but it also links invalid links, e.g. http://-invalid-

I'm not sure if it's easily fixable, but we could also consider replacing our current regex with another one. The questions are:

  • Do we want to be 100% compliant with the spec?
  • Do we want to include big regex instead of our current small one?

@m.lewandowski @Tade0 WDYT?

comment:11 Changed 7 years ago by Tade0

I think most instances of pasting such an invalid link will be people testing if this particular URL is transformed.

In other words: There are significantly more valid urls with hyphens pasted each day into CKE than weird, invalid links.

Originally I had chosen this particular regex because it strikes a good balance between shortness and correctness.

Last edited 7 years ago by Tade0 (previous) (diff)

comment:12 in reply to:  11 Changed 7 years ago by Marek Lewandowski

Replying to Tade0:

I think most instances of pasting such an invalid link will be people testing if this particular URL is transformed.

In other words: There are significantly more valid urls with hyphens pasted each day into CKE than weird, invalid links.

Originally I had chosen this particular regex because it strikes a good balance between shortness and correctness.

+1 it make sense. I can imagine being unable to paste links with hypens to be far more irritating, than letting some edge cases through.

comment:13 Changed 7 years ago by Tomasz Jakut

Resolution: fixed
Status: pendingclosed

In that case, fix is good enough. Merged with git:f8e830b.

comment:14 Changed 7 years ago by Marek Lewandowski

Summary: [autolink] URL with '-' in host not detectedURL with '-' in host not detected by autolink plugin
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy