WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#5081 closed defect (bug) (fixed)

make_clickable shouldn't include trailing punctuation

Reported by: matt Owned by: neodude
Milestone: 2.5 Priority: normal
Severity: minor Version: 2.3
Component: General Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (2)

formatting.php.diff (1.3 KB) - added by neodude 6 years ago.
removes [.,;:] from urls with a protocol, [,;:] from urls without a protocol
wptests-[170]-for-[5081].diff (2.7 KB) - added by neodude 6 years ago.
Test cases; diff against revision 170 at http://svn.automattic.com/wordpress-tests/

Download all attachments as: .zip

Change History (19)

comment:1 follow-up: Nazgul7 years ago

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.

comment:2 JeremyVisser7 years ago

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.

comment:3 markjaquith7 years ago

  • Keywords needs-patch added

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.

comment:4 in reply to: ↑ 1 ; follow-up: Viper007Bond7 years ago

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.

comment:5 in reply to: ↑ 4 Nazgul7 years ago

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. :)

neodude6 years ago

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

comment:6 neodude6 years ago

  • Cc neodude added
  • Keywords has-patch added; needs-patch removed
  • Owner changed from anonymous to neodude
  • Status changed from new to assigned

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.

comment:7 neodude6 years ago

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

comment:8 westi6 years ago

  • Keywords needs-unit-tests added

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.

neodude6 years ago

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

comment:9 neodude6 years ago

  • Keywords needs-unit-tests removed
  • Resolution set to fixed
  • Status changed from assigned to closed

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?

comment:10 markjaquith6 years ago

  • Keywords needs-testing added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:11 neodude6 years ago

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

comment:12 follow-up: nbachiyski6 years ago

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.

comment:13 in reply to: ↑ 12 ; follow-up: neodude6 years ago

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

comment:14 in reply to: ↑ 13 nbachiyski6 years ago

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.

comment:15 nbachiyski6 years ago

  • Keywords needs-testing removed

comment:16 markjaquith6 years ago

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

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

comment:17 markjaquith6 years ago

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.

Note: See TracTickets for help on using tickets.