WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#10990 closed defect (bug) (fixed)

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

Reported by: josephscott Owned by: westi
Milestone: 2.9 Priority: normal
Severity: normal Version: 2.8.4
Component: Formatting Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (4)

wp-includes--formatting.php.diff (582 bytes) - added by josephscott 4 years ago.
make_clickable_end_chars.10990.diff (1.5 KB) - added by filosofo 4 years ago.
make_clickable_end_chars.10990.2.diff (1.5 KB) - added by filosofo 4 years ago.
make_clickable_end_chars.10990.3.diff (1.5 KB) - added by filosofo 4 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 westi4 years ago

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.

comment:2 westi4 years ago

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.

comment:3 filosofo4 years ago

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

comment:4 filosofo4 years ago

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.

comment:5 filosofo4 years ago

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

comment:6 westi4 years ago

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

comment:7 josephscott4 years ago

@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))

comment:8 follow-up: josephscott4 years ago

@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.

comment:9 in reply to: ↑ 8 westi4 years ago

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.

comment:10 josephscott4 years ago

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.

comment:11 westi4 years ago

  • Owner changed from azaozz to westi
  • Status changed from new to accepted

comment:12 filosofo4 years ago

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

comment:13 josephscott4 years ago

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.

comment:14 westi4 years ago

Verified here as well with the unit tests.

comment:15 westi4 years ago

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

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

Note: See TracTickets for help on using tickets.