Problem/Motivation

This seems to be a regression compared to D7

http://example.org/?search=test
http://example.org/?search=Test

The second URL is not correctly identified, it stops at the =

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 2557021-url-14.patch2.21 KBtim.plunkett
#8 url_filter_does_not-2557021-8-interdiff.txt1.93 KBsasanikolic
#8 url_filter_does_not-2557021-8.patch2.67 KBsasanikolic
#5 2557021-filter-5.patch2.43 KBtim.plunkett
#5 interdiff.txt651 bytestim.plunkett
#3 url_filter_does_not-2557021-3.patch1.79 KBsasanikolic

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +Needs tests

The only issue that I can find that could have caused this is #1657886: Filter "Convert URLs into links" doesn't support multilingual web addresses.

sasanikolic’s picture

sasanikolic commented
Status: Active » Needs review
StatusFileSize
new url_filter_does_not-2557021-3.patch1.79 KB

This should make the test fail.

Status: Needs review » Needs work

The last submitted patch, 3: url_filter_does_not-2557021-3.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt651 bytes
new 2557021-filter-5.patch2.43 KB

Yep, @Berdir found the right issue.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/modules/filter/filter.module
@@ -515,7 +515,7 @@ function _filter_url($text, $filter) {
 
-  $valid_url_query_chars = '[a-z0-9!?\*\'@\(\);:&=\+\$\/%#\[\]\-_\.,~|]';
+  $valid_url_query_chars = '[a-zA-Z0-9!?\*\'@\(\);:&=\+\$\/%#\[\]\-_\.,~|]';
   $valid_url_query_ending_chars = '[a-z0-9_&=#\/]';

Obvious fix is obvious but I think we need the same change for the ending chars too? Just add two more lines with tesT and TesT and it will not work as expected.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work
Issue tags: -Needs tests
sasanikolic’s picture

sasanikolic commented
Status: Needs work » Needs review
StatusFileSize
new url_filter_does_not-2557021-8.patch2.67 KB
new url_filter_does_not-2557021-8-interdiff.txt1.93 KB

Extended the test and added the condition for urls ending with capital letters.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Great, RTBC if green.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +Needs backport to D7

Wow, how was this problem not discovered sooner!? This definitely needs a backport to D7 — this code is virtually unchanged from that in D7.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: -Needs backport to D7

As written, this is a regression, it works fine in 7.x, as visible in the issue summary :)

There was exactly one change in that function since 7.x and that broke it, see #2.

However, this fix needs to be included in the backport of that issue.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Oh, sorry :(

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/filter/tests/filter.url-input.txt
@@ -33,4 +36,4 @@ The old URL filter has problems with <a title="kind of link www.example.com with
-This is the end!
\ No newline at end of file
+This is the end!

+++ b/core/modules/filter/tests/filter.url-output.txt
@@ -33,4 +36,4 @@ The old URL filter has problems with <a title="kind of link www.example.com with
-This is the end!
\ No newline at end of file
+This is the end!

These are out of scope changes. Since we are testing filters I'm loathed to change this without considering whether or not we are losing text coverage - since you are changing both the input and output.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
StatusFileSize
new 2557021-url-14.patch2.21 KB

Fair point.

LKS90’s picture

LKS90 commented
Status: Needs review » Reviewed & tested by the community

Unrelated changes have been reverted, the patch still applies and passes the tests. RTBC!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Thanks for addressing #13. Committed a3ccfba and pushed to 8.0.x. Thanks!

  • alexpott committed a3ccfba on 8.0.x 
    Issue #2557021 by sasanikolic, tim.plunkett: Url Filter does not...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.