Include trailing ) in make_clickable if it is part of the URL

[josephscott]

Parentheses are valid characters in URLs, for instance:

http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)

But it also common to include a URL reference inside of parentheses that aren't part of a URL, like:

(http://www.example.com/)

The make_clickable() function in -trunk works for the second case, but not the first. The link it creates doesn't include in the trailing paren.

The pattern that seems to work is if the first and last characters (second example) are parens then don't include the trailing paren in the URL. If the first character isn't a paren (the first example) then include the trailing paren in the URL.

I'm including a simple patch to make make_clickable() work correctly for these URLs.

I think WP used to work this way, I suspect rev 11844 changed this behavior.

[westi]

I've added test cases for this to wordpress-tests.
Inputs:

'http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)',
'(http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software))',

Outputs:

'<a href="http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)" rel="nofollow">http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)</a>',
'(<a href="http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)" rel="nofollow">http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)</a>)',

The first passes with this patch applied. The second doesn't you get the following output:

(<a href="http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software" rel="nofollow">http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software</a>))

It also causes the following test to fail with the trailing period incorrectly in the url and link.

There was a spoon named http://wordpress.org.

[westi]

FYI running the same tests against 2.8.5 both the tests that fail with your patch applied also fail.

The difference being that the brackets one puts both the closing brackets in the url rather than none of them.

[filosofo]

My patch addresses these issues and works correctly for all of the following examples, some of which were taken from #8300, the ticket originally addressing this.

I'm sorry I didn't catch [11844] sooner, but it seems to have snuck in without a ticket.

blah blah blah http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software) blah blah
blah blah (http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)) blah blah
blah blah http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software).) blah blah
blah blah There was a spoon named http://wordpress.org. [EOL character after .]
blah blah There was a spoon named http://wordpress.org. blah blah
blah http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software) blah blah
blah http://msdn.microsoft.com/en-us/library/aa752574(VS.85).aspx blah blah
blah My site (http://josephscott.org) blah blah
blah My (http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)) article blah blah

[filosofo]

I got to thinking, and make_clickable_end_chars.10990.2.diff might be better, depending on what you want to do. For

blah blah http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software).) blah blah

my second patch makes the url to be

blah blah http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)

but for

blah blah http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software).)moreurl blah blah

it makes the url to be

blah blah http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software).)moreurl

The issue is not so much what are legal characters in URLs as much as what is a likely use.

[filosofo]

Whoops, the URL examples in my previous comment don't have "blah blah" prefixing them, but I think you know what I meant.

[westi]

Patch doesn't work for all the test cases we have in wordpress-tests:

peter-westwoods-computer:wordpress-tests peterwestwood$ php wp-test.php -t TestMakeClickable
Test Suite
 TestMakeClickable
 ....F...F

Time: 1 second

There were 2 failures:

1) test_strip_trailing_with_protocol_nothing_afterwards(TestMakeClickable)
Failed asserting that two strings are equal.
expected string <There was a spoon named (<a href="http://wordpress.org" rel="nofollow">http://wordpress.org</a>)>
difference      <                                                      xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx?>
got string      <There was a spoon named (<a href="http://wordpress.org)" rel="nofollow">http://wordpress.org)</a>>
/Users/peterwestwood/Documents/workspace/wordpress-tests/wp-testcase/test_includes_formatting.php:86
/Users/peterwestwood/Documents/workspace/wordpress-tests/wp-testlib/base.php:526
/Users/peterwestwood/Documents/workspace/wordpress-tests/wp-test.php:107

2) test_brackets_in_urls(TestMakeClickable)
Failed asserting that two strings are equal.
expected string <(<a href="http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)" rel="nofollow">http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software)</a>)>
difference      <                                                                        xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx?>
got string      <(<a href="http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software))" rel="nofollow">http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software))</a>>
/Users/peterwestwood/Documents/workspace/wordpress-tests/wp-testcase/test_includes_formatting.php:167
/Users/peterwestwood/Documents/workspace/wordpress-tests/wp-testlib/base.php:526
/Users/peterwestwood/Documents/workspace/wordpress-tests/wp-test.php:107

FAILURES!
Tests: 9, Assertions: 41, Failures: 2.

Tests:
​http://svn.automattic.com/wordpress-tests/wp-testcase/test_includes_formatting.php

[josephscott]

@filosofo

Thanks for improving this again. Using your patch and the test cases mentioned above (including the two from westi) I think there's only one case left where things don't work as expected:

(http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software))

on a line by itself. The opening paren before the URL is correctly left out, but the trailing paren is included in the URL, when it shouldn't be. This only triggers if that is on a line by itself, since this works fine:

blah (http://en.wikipedia.org/wiki/PC_Tools_(Central_Point_Software))

[josephscott]

@westi -

What WP version are you running those tests against? I applied the last patch provided by filosofo on a -trunk install then submitted a comment with these lines:

http://wordpress.org/hello.html
There was a spoon named http://wordpress.org. Alice!
There was a spoon named http://wordpress.org, said Alice.
There was a spoon named http://wordpress.org; said Alice.
There was a spoon named http://wordpress.org: said Alice.
There was a spoon named (http://wordpress.org) said Alice.

They all linked correctly.

[westi]

Replying to josephscott:

@westi -

What WP version are you running those tests against? I applied the last patch provided by filosofo on a -trunk install then submitted a comment with these lines:

http://wordpress.org/hello.html
There was a spoon named http://wordpress.org. Alice!
There was a spoon named http://wordpress.org, said Alice.
There was a spoon named http://wordpress.org; said Alice.
There was a spoon named http://wordpress.org: said Alice.
There was a spoon named (http://wordpress.org) said Alice.

They all linked correctly.

The test was run against trunk + patch.

All those examples work (they are from test_strip_trailing_with_protocol) the ones that fail are in test_strip_trailing_with_protocol_nothing_afterwards.

[josephscott]

Hmm, I still get different results. When I post all of the test_strip_trailing_with_protocol_nothing_afterwards examples in a comment on -trunk + make_clickable_end_chars.10990.2.diff they all link correctly.

[filosofo]

make_clickable_end_chars.10990.3.diff passes the unit tests and the above examples.

[josephscott]

I tried the make_clickable_end_chars.10990.3.diff patch and so far it works correctly for all of the URL tests that I've seen.

[westi]

Verified here as well with the unit tests.

[westi]

(In [12088]) Ensure that trailing ) in urls are included in the link when it looks appropriate. Fixes #10990 props filosofo.