make_clickable shouldn't include trailing punctuation

[matt]

Here's an example:

​http://photomatt.net/2007/09/21/foundread-public-square/#comment-427539

see http://lists.drupal.org/archives/development/2006-08/msg00965.html.

The trailing period is linked, and it shouldn't be.

[Nazgul]

There are instances where that trailing period "should" be linked.

If I look at the RFC, the link www.wordpress.org. (with the trailing dot) is a valid FQDN and in theorie the period should therefore also be linked.

I see the point of unlinking the trailing period, but wanted to make this side-effect clear.

[JeremyVisser]

Well, "www.wordpress.org." is not a URL. If it were a real URL, like the following, it would be have a trailing slash: http://www.wordpress.org./

But, I guess, if for some reason the URL required a full stop at the end of it, you'd get problems. You could simply override make_clickable() by using a manual <a href="http://example.com/wacko.">http://example.com/wacko.</a>. Having a full stop at the end of the URL seems like a really niche case.

[markjaquith]

Definitely a niche case (valid URL ending in a "dot"). A lot of "make clickable" implementations screw this up and put things like ending parethesis and ending "dots" into the href, so it would be a refreshing change if we could make our implementation handle those cases better.

[Viper007Bond]

Replying to Nazgul:

There are instances where that trailing period "should" be linked.

If I look at the RFC, the link www.wordpress.org. (with the trailing dot) is a valid FQDN and in theorie the period should therefore also be linked.

I see the point of unlinking the trailing period, but wanted to make this side-effect clear.

I think in 99.9% of cases, a period at the end of the URL is just that -- a period and not part of the URL. It shouldn't be included.

[Nazgul]

Replying to Viper007Bond:

I think in 99.9% of cases, a period at the end of the URL is just that -- a period and not part of the URL. It shouldn't be included.

I completely agree.

It's just that I had a similar issue in a program at work and got quite some negative feedback when I made that change (picky customer), so I thought I'd at least throw it out there. Now it's a design decision instead of an unknown side-effect. :)

[neodude]

removes [.,;:] from urls with a protocol, [,;:] from urls without a protocol

[neodude]

Added patch. It removes a trailing period, comma, colon or semicolon in URLs with a protocol (i.e. ​http://wordpress.org), but only the trailing comma, colon or semicolon in URLs without a protocol (i.e. www.wordpress.org). The consensus seemed indicate that www.wordpress.org. is a FQDN, so should stay as is.

[neodude]

btw - this is my first attempt at fixing a bug; advance apologies for making any newbie mistakes!

[westi]

I think we can strip the valid but only informative . off the end of domain names being made clickable as having it/not having it shouldn't make any difference to how the link is interpreted.

The patch looks good - I think it would be good to add some tests to the unit test suite for this as well to prove it works well for all cases.

[neodude]

Test cases; diff against revision 170 at ​http://svn.automattic.com/wordpress-tests/

[neodude]

Just attached the (2) unit tests, as against the wordpress-tests svn. Just running revision 170 of wp-tests against [7343] gives 31 failures and 13 errors (though some are because of safe mode restrictions or bugs in the unit tests themselves). After applying the formatting.php.diff to wordpress and the just-attached patch to wordpress-tests, it gives the same 31/13 failures, but two more successes, so I'd assume my tests passed :)

And hence, I suppose we can call this bug fixed?

[markjaquith]

Bugs are marked as fixed once the fix has been checked in. I'm heading to bed now, but will take a look at it tomorrow.

[neodude]

I see! Good to know.
Will someone commit the test case into the tests svn as well, once the fix has been checked in?

[nbachiyski]

neodude, you can run only your tests by executing:
php wp-test.php -t <test-classname-here>

Also, we have a system, which can connect your unit test with WordPress trac tickets. If you add:

$this->knownWPBug(5081);

call in your the beginning of a test method, it will:

• be skipped if the corresponding ticket isn't closed and checked otherwise
• be skipped if -s command line option is supplied for wp-test.php
• be checked if -f command line option is supplied for wp-test.php

Your tests were committed in revision 172 of wordpress-tests.

[neodude]

This is extremely useful information - probably would've saved me hours of fiddling with wordpress-tests. Is this testing framework documented anywhere?

[nbachiyski]

Replying to neodude:

This is extremely useful information - probably would've saved me hours of fiddling with wordpress-tests. Is this testing framework documented anywhere?

You want better documentation than the code itself? ;) No, it isn't. Have a look at {{{wp-test.php}}, it is pretty straightforward.

[markjaquith]

(In [7452]) make_clickable() trailing punctuation fixes from neodude. fixes #5081

[markjaquith]

FYI, I committed it with it omitting the trailing "dot" -- if you want a trailing dot in a hyperlink, you can code it up properly. In the majority of cases, we don't want it.