WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 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 7 years ago.
removes [.,;:] from urls with a protocol, [,;:] from urls without a protocol
wptests-[170]-for-[5081].diff (2.7 KB) - added by neodude 7 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. :)

@neodude7 years ago

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

comment:6 @neodude7 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 @neodude7 years ago

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

comment:8 @westi7 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.

@neodude7 years ago

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

comment:9 @neodude7 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 @markjaquith7 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 @neodude7 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: @nbachiyski7 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: @neodude7 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 @nbachiyski7 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 @nbachiyski7 years ago

  • Keywords needs-testing removed

comment:16 @markjaquith7 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 @markjaquith7 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.