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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

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

Status: Active » Needs review
FileSize
1.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

Status: Needs work » Needs review
FileSize
651 bytes
2.43 KB

Yep, @Berdir found the right issue.

Berdir’s picture

+++ 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

Status: Needs review » Needs work
Issue tags: -Needs tests
sasanikolic’s picture

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

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Great, RTBC if green.

Wim Leers’s picture

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

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

Oh, sorry :(

alexpott’s picture

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

Status: Needs work » Needs review
FileSize
2.21 KB

Fair point.

LKS90’s picture

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

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.