WordPress.org

Make WordPress Core

Opened 13 months ago

Closed 5 months ago

Last modified 4 months ago

#23756 closed defect (bug) (fixed)

make_clickable should not convert the URLs in pre,code tags

Reported by: alex-ye Owned by: 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 months ago.
Skip links in code and pre blocks.
23756.diff (802 bytes) - added by helen 8 months ago.
23756.test.diff (2.3 KB) - added by helen 8 months ago.
23756.2.diff (1.0 KB) - added by adamsilverstein 8 months ago.
add nest level count, corrects issue with nested pre & code tags
23756.3.diff (3.3 KB) - added by nacin 8 months ago.
23756.4.diff (3.9 KB) - added by sirbrillig 6 months ago.
Updated combined patch and test that covers code and pre tags with attributes.
23756.5.diff (3.9 KB) - added by sirbrillig 6 months ago.
Re-added @ to ticket number in test.
ticket-23756-ignore-case.patch (3.0 KB) - added by bpetty 5 months ago.

Download all attachments as: .zip

Change History (34)

comment:1 alex-ye13 months 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 :)

comment:2 alex-ye13 months ago

  • Version set to 3.5.1

comment:3 follow-up: alexvorn213 months ago

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

Last edited 13 months ago by alexvorn2 (previous) (diff)

comment:4 in reply to: ↑ 3 ; follow-up: alex-ye13 months ago

Replying to alexvorn2:

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

Yes

comment:5 in reply to: ↑ 4 ; follow-up: SergeyBiryukov13 months 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

comment:6 follow-up: SergeyBiryukov13 months 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.

comment:7 in reply to: ↑ 5 alex-ye13 months 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 ..

comment:8 in reply to: ↑ 6 alex-ye13 months 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 .

johnjamesjacoby11 months ago

Skip links in code and pre blocks.

comment:9 mordauk9 months ago

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

comment:10 rachelbaker8 months ago

  • Cc rachel@… added

comment:11 rachelbaker8 months ago

  • Keywords needs-testing added

comment:12 nacin8 months 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!

helen8 months ago

helen8 months ago

comment:13 follow-up: helen8 months 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

adamsilverstein8 months ago

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

comment:14 in reply to: ↑ 13 adamsilverstein8 months 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

nacin8 months ago

comment:15 nacin8 months 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().

comment:16 kpdesign7 months 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.

comment:17 nacin7 months ago

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

sirbrillig6 months ago

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

comment:18 sirbrillig6 months 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 6 months ago by sirbrillig (previous) (diff)

sirbrillig6 months ago

Re-added @ to ticket number in test.

comment:19 kpdesign6 months 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>

comment:20 netweb6 months ago

  • Cc stephen@… added

comment:21 rmccue6 months ago

  • Keywords commit added

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

comment:22 wonderboymusic5 months 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.

comment:23 bpetty5 months 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

comment:24 wonderboymusic5 months 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.

comment:25 nbachiyski5 months 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 :-)

comment:26 beaulebens4 months ago

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