make_clickable incorrectly formats anchors with URL's and spaces in them in comments

[johnjamesjacoby]

When posting a comment, if an anchor tag contains both a URL and some words, make_clickable formats the output incorrectly.

To duplicate, post the following content in a comment:

Hey! <a href="http://wordpress.org">http://wordpress.org is awesome</a> in case you didn't know!

When viewing the comment, you'll get:

Hey!
<a href="#" rel="nofollow"></a>
<a href="http://wordpress.org" rel="nofollow">http://wordpress.org</a> is awesome in case you didn't know.

[miqrogroove]

Confirmed in 3.5 as well as in 3.4.2.

[mikejolley]

Fix for the code which removes links within links

[mikejolley]

Attached ​http://core.trac.wordpress.org/attachment/ticket/23050/fix.make.clickable.23050.diff which fixed the link within link code. Before it would only work if the closing link tags were side by side. This checks for content between them.

[netweb]

Awesome, works for me.

Related: bbPress#1932

[netweb]

WITHOUT rel="nofollow"

[netweb]

Spoke to soon, adding some unit tests (I am pretty sure links should include rel="nofollow")

• 23050-unit-tests.diff - Resulting links WITHOUT rel="nofollow"
• 23050-unit-tests-nofollow.diff - Resulting links WITH rel="nofollow"

Current patch attachment:fix.make.clickable.23050.diff​​ breaks PHPUnit formatting test.

$ phpunit --group formatting
There was 1 failure:
1) Tests_Formatting_MakeClickable::test_no_links_within_links
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Some text with a link <a href="http://example.com">http://example.com</a>'
+'Some text with a link <a href="http://example.com"><a href="http://example.com" rel="nofollow">http://example.com</a></a>'

$ phpunit --group 23050
There was 1 failure:

1) Tests_Formatting_MakeClickable::test_anchors_with_url_and_words
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Hey! <a href="http://wordpress.org" rel="nofollow">http://wordpress.org is awesome</a> if you have clicks to give!'
+'Hey! <a href="http://wordpress.org">http://wordpress.org is awesome</a> if you have clicks to give!'

[netweb]

WITH rel="nofollow"

[mikejolley]

@netweb Personally I would expect existing links to be left alone. The code patched was to fix links within links, the links within links being the invalid ones (which would have nofollow).

Entire string could, after fixing, be ran though something like wp_rel_nofollow which is also in the formatting.php file, but I'm not sure if this is the expected behaviour or not - the function is make_clickable, and makes no mention of formatting links with nofollow.

In fact, in the case of comments where nofollow is important, the content would have already been nofollowed during the pre_comment_content hook.

[beaulebens]

Using a similar approach to #23756 might make sense here. Example patch attached (which includes compacting the initial regex into a single one, vs introducing a third) and moving the $nested_code_pre check in the elseif to the start to avoid doing string operations if unnecessary.

I have not run the unit tests because I don't currently have them set up :)

[beaulebens]

Ignore links within link a la code/pre blocks

[netweb]

PHPUnit tests using ​23050-ala-23757.diff

phpunit --group "formatting,23050" - 23050-unit-tests-nofollow.diff - Resulting links WITH rel="nofollow"

Time: 11 seconds, Memory: 43.75Mb

There were 2 failures:

1) Tests_Formatting_MakeClickable::test_anchors_with_url_and_words
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Hey! <a href="http://wordpress.org" rel="nofollow">http://wordpress.org is awesome</a> if you have clicks to give!'
+'Hey! <a href="http://wordpress.org">http://wordpress.org is awesome</a> if you have clicks to give!'

C:\xampp\htdocs\develop.wp.nw\tests\phpunit\tests\formatting\MakeClickable.php:390

2) Tests_Formatting_MakeClickable::test_square_brackets
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<a href="http://example.com/?foo%5Bbar%5D=baz" rel="nofollow">http://example.com/?foo%5Bbar%5D=baz</a>'
+'<a href="http://example.com/?foobar=baz" rel="nofollow">http://example.com/?foobar=baz</a>'

C:\xampp\htdocs\develop.wp.nw\tests\phpunit\tests\formatting\MakeClickable.php:434

FAILURES!
Tests: 557, Assertions: 1430, Failures: 2, Incomplete: 1, Skipped: 27.

phpunit --group "formatting,23050" - 23050-unit-tests.diff - Resulting links WITHOUT rel="nofollow"

Time: 6 seconds, Memory: 43.00Mb

OK, but incomplete or skipped tests!
Tests: 557, Assertions: 1433, Incomplete: 1, Skipped: 28.

phpunit --group "formatting" i.e. without any of the 23050 unit tests

Time: 7 seconds, Memory: 43.25Mb

OK, but incomplete or skipped tests!
Tests: 556, Assertions: 1428, Incomplete: 1, Skipped: 28.

[nacin]

So, looks like there is still some breakage. I don't think we should allow for someone to circumvent nofollow (at least it's not a security issue). Let's see what we can do here. Probably needs more tests and obviously some tweaks to the patch.

I definitely like beauleben's approach, but I'm up for whatever pans out.

[chriscct7]

Related #23922