Opened 11 months ago

Closed 4 months ago

Last modified 2 months 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 11 months ago by j.swiderski

  • Status changed from new to confirmed
  • Version changed from 4.5.10 (GitHub - master) to 4.5.0

Problem can be reproduced from CKEditor 4.5.0.

comment:2 Changed 11 months ago by j.swiderski

#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 months ago by j.swiderski

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

Last edited 5 months ago by j.swiderski (previous) (diff)

comment:4 Changed 5 months ago by j.swiderski

#14903 was marked as duplicate.

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

comment:5 Changed 5 months ago by j.swiderski

#16842 was marked as duplicate.

comment:6 Changed 4 months ago by m.lewandowski

  • Milestone set to CKEditor 4.7.0

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

comment:7 Changed 4 months ago by Tade0

Done.

Changes pushed to branch:t/14769.

comment:8 Changed 4 months ago by Tade0

Added manual test.

Changes pushed to ​branch:t/14769.

comment:9 Changed 4 months ago by Tade0

  • Owner set to Tade0
  • Status changed from confirmed to review

comment:10 Changed 4 months ago by t.jakut

  • Status changed from review to pending

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 follow-up: Changed 4 months 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 4 months ago by Tade0 (previous) (diff)

comment:12 in reply to: ↑ 11 Changed 4 months ago by m.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 4 months ago by t.jakut

  • Resolution set to fixed
  • Status changed from pending to closed

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

comment:14 Changed 2 months ago by m.lewandowski

  • Summary changed from [autolink] URL with '-' in host not detected to URL with '-' in host not detected by autolink plugin
Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy