Make WordPress Core

Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#23756 closed defect (bug) (fixed)

make_clickable should not convert the URLs in pre,code tags

Reported by: alex-ye's profile alex-ye Owned by: wonderboymusic's profile wonderboymusic
Milestone: 3.8 Priority: normal
Severity: normal Version: 3.5.1
Component: Formatting Keywords: has-patch needs-testing has-unit-tests commit
Focuses: Cc:

Description

Hi , Maybe it's a duplicate ticket but I tried to search in history.

Anyway , The purpose of this ticket is to have a clean snippet code posts .. for now it's hard to do it by plug-in .. and many people remove make_clickable filter at all !

So let's make it possible in 3.6 :)

Attachments (8)

23756.patch (701 bytes) - added by johnjamesjacoby 11 years ago.
Skip links in code and pre blocks.
23756.diff (802 bytes) - added by helen 11 years ago.
23756.test.diff (2.3 KB) - added by helen 11 years ago.
23756.2.diff (1.0 KB) - added by adamsilverstein 11 years ago.
add nest level count, corrects issue with nested pre & code tags
23756.3.diff (3.3 KB) - added by nacin 11 years ago.
23756.4.diff (3.9 KB) - added by sirbrillig 11 years ago.
Updated combined patch and test that covers code and pre tags with attributes.
23756.5.diff (3.9 KB) - added by sirbrillig 11 years ago.
Re-added @ to ticket number in test.
ticket-23756-ignore-case.patch (3.0 KB) - added by bpetty 11 years ago.

Download all attachments as: .zip

Change History (34)

#1 @alex-ye
12 years ago

  • Cc nashwan.doaqan@… added

and who will say that I must escape the URLs before posting them so the make_clickable won't work , It's really hard and dirty solution !!

Many users don't do that and many users facing problems !! , I think we should not convert the URLs in pre,code tags at all !! , or at least make it easy by some hooks :)

#2 @alex-ye
12 years ago

  • Version set to 3.5.1

#3 follow-up: @alexvorn2
12 years ago

does this function is used when displaying the post? -> the_content()

Last edited 12 years ago by alexvorn2 (previous) (diff)

#4 in reply to: ↑ 3 ; follow-up: @alex-ye
12 years ago

Replying to alexvorn2:

does this function is used when displaying the post? -> the_content()

Yes

#5 in reply to: ↑ 4 ; follow-up: @SergeyBiryukov
12 years ago

Replying to alex-ye:

Replying to alexvorn2:

does this function is used when displaying the post? -> the_content()

Yes

No, it isn't: #18214.

The purpose of this ticket is to have a clean snippet code posts

make_clickable() is only applied to comment_text() by default:
http://core.trac.wordpress.org/browser/tags/3.5.1/wp-includes/default-filters.php#L151

#6 follow-up: @SergeyBiryukov
12 years ago

Making it skip <pre> and <code> tags might still be a good idea though, if it doesn't have a noticeable effect on performance.

#7 in reply to: ↑ 5 @alex-ye
12 years ago

Replying to SergeyBiryukov:

No, it isn't: #18214.

Thank you for correction , I make sure that a plugin who adds the make_clickable to the_content filter ..

#8 in reply to: ↑ 6 @alex-ye
12 years ago

Replying to SergeyBiryukov:

Making it skip <pre> and <code> tags might still be a good idea though, if it doesn't have a noticeable effect on performance.

Yes , I face problems in comments or topics/replies also in bbPress , I made my own hack to fix this but I think it is something the core should do .

The bug be bigger when you use a Syntax Highlighter plugins .

@johnjamesjacoby
11 years ago

Skip links in code and pre blocks.

#9 @mordauk
11 years ago

  • Cc pippin@… added
  • Keywords has-patch added; needs-patch removed

#10 @rachelbaker
11 years ago

  • Cc rachel@… added

#11 @rachelbaker
11 years ago

  • Keywords needs-testing added

#12 @nacin
11 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.7

Nice. This was more difficult to implement until we started splitting on HTML elements.

Needs unit tests!

@helen
11 years ago

@helen
11 years ago

#13 follow-up: @helen
11 years ago

I WROTE A UNIT TEST YOU GUYS. 23756.test.diff

Looking at jjj's patch, it seemed to me that nesting them would break things, as in it would end the $in_code flag early, which does seem to be the case. The last test fails; however, I'm not sure that you're supposed to have a code element inside pre unless it wraps everything as well, e.g. <pre><code>blah</code></pre>. See http://www.w3.org/TR/html5/grouping-content.html#the-pre-element

@adamsilverstein
11 years ago

add nest level count, corrects issue with nested pre & code tags

#14 in reply to: ↑ 13 @adamsilverstein
11 years ago

Replying to helen:

I WROTE A UNIT TEST YOU GUYS. 23756.test.diff

Looking at jjj's patch, it seemed to me that nesting them would break things, as in it would end the $in_code flag early, which does seem to be the case. The last test fails; however, I'm not sure that you're supposed to have a code element inside pre unless it wraps everything as well, e.g. <pre><code>blah</code></pre>. See http://www.w3.org/TR/html5/grouping-content.html#the-pre-element

your unit test looks good to me. i added a nested level count in 23756.2.diff​ and your test now passes! not sure pre tag surrounding code is right, but it is going to happen, so makes sense to account for the possibility. even in the w3.org page you linked they have nested that way in their sample code. http://cl.ly/Qp6K

@nacin
11 years ago

#15 @nacin
11 years ago

23756.3.diff merges the tests with the code and tidies things up a bit.

It looks good to me, but I am asking that duck_ or mdawaffe review this as well given their experience with the function and potential security implications (XSS) with make_clickable().

#16 @kpdesign
11 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from 3.7 to 3.8

Moving to 3.8 for review and further discussion.

#17 @nacin
11 years ago

What about attributes assigned to <code> or <pre>? We should account for that possibility as well.

@sirbrillig
11 years ago

Updated combined patch and test that covers code and pre tags with attributes.

#18 @sirbrillig
11 years ago

I updated the test to include code and pre tags with attributes as well as custom tags that start with the words 'code' or 'pre'. This caused the test to fail. I then updated the function part of the patch to pass the test.

23756.5.diff

Last edited 11 years ago by sirbrillig (previous) (diff)

@sirbrillig
11 years ago

Re-added @ to ticket number in test.

#19 @kpdesign
11 years ago

I've been watching this ticket, as this is an issue that affects the handbooks. The handbooks use the SyntaxHighlighter Evolved plugin, and all of the code samples containing URLs are broken (they display full markup for a URL instead of just the URL). See Automated Testing > Installation for an example.

I applied 23756.5.diff to my local install (latest trunk), tested, and got the expected results listed in the unit test.

This patch also fixed my local test code samples using the shortcodes in the plugin (no markup added to URLs).

Code:

[sourcecode lang="bash"]
$ URL https://core.trac.wordpress.org/attachment/ticket/00000/00000.patch
$ Email name@domain.tld
$ FTP ftp://user:password@domain.tld/path
[/sourcecode]

Generated markup:

<pre class="brush: bash; title: ; notranslate" title="">
$ URL https://core.trac.wordpress.org/attachment/ticket/00000/00000.patch
$ Email name@domain.tld
$ FTP ftp://user:password@domain.tld/path
</pre>

#20 @netweb
11 years ago

  • Cc stephen@… added

#21 @rmccue
11 years ago

  • Keywords commit added

I'd prefer $nested_code_pre > 0 rather than the implicit check, but that's just me. +1 otherwise.

#22 @wonderboymusic
11 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 26052:

Don't convert URLs inside <pre> and <code> tags when parsing string using make_clickable().
Adds Unit Tests.

Props johnjamesjacoby, helen, nacin, adamsilverstein, sirbrillig.
Fixes #23756.

#23 @bpetty
11 years ago

  • Cc bpetty added
  • Resolution fixed deleted
  • Status changed from closed to reopened

For good measure, added in case insensitive modifier for <pre> and <code> tag searches in ticket-23756-ignore-case.patch

#24 @wonderboymusic
11 years ago

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

In 26094:

Make HTML tag searches for <code> and <pre> case-insensitive in make_clickable().

Props bpetty.
Fixes #23756.

#25 @nbachiyski
11 years ago

There is some prior art of not mangling certain parts of the HTML in wptexturize(). Look at the bottom of the function. It handles more cases – some shortcodes (code), more tags (kbd, style, script, tt), handles some invalid HTML cases, supports filtering the disabled tags/shortcodes.

It’d be great if the logic for the two can be consolidated, because for example now make_clickable will convert URLs in script tags and in shortcodes.

if anybody else is excited, I’d be happy to help.

P.S. I wrote the older code, but I’m pretty embarrassed of functions like _wptexturize_pushpop_element :-)

#26 @beaulebens
11 years ago

  • Cc beau@… added
Note: See TracTickets for help on using tickets.