Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 8 years ago

#33645 closed defect (bug) (fixed)

wpautop breaks HTML comments

Reported by: thomas-hicks's profile Thomas Hicks Owned by: azaozz's profile azaozz
Milestone: 4.3.1 Priority: normal
Severity: normal Version: 4.2.4
Component: Formatting Keywords: has-patch wpautop fixed-major
Focuses: Cc:

Description

Our site had a commented out section of code (within <!-- -->) within a post on a page.

previously ( < 4.2.3), newlines in the code were replaced with a space. Now this gets replaced with <!-- wpnl -->, which closes the user made commented out code prematurely, leading to elements that should not be displayed being displayed on the live site.

code in question:
Was (< 4.2.3):

// Strip newlines from all elements.	 	     
       $pee = wp_replace_in_html_tags( $pee, array( "\n" => " " ) );

Now (4.2.4):

// Find newlines in all elements and add placeholders.
$pee = wp_replace_in_html_tags( $pee, array( "\n" => " <!-- wpnl --> " ) );


Attachments (5)

33645.patch (1.9 KB) - added by miqrogroove 9 years ago.
Fixes newlines around objects. Fixes confusion of P and PARAM. Adds unit test.
33645.2.patch (1.9 KB) - added by miqrogroove 9 years ago.
Optimized regexp.
33645-4.3.1.patch (565 bytes) - added by azaozz 9 years ago.
33645-4.4.patch (1.8 KB) - added by azaozz 9 years ago.
33645-4.4-expanded.patch (3.2 KB) - added by miqrogroove 9 years ago.

Download all attachments as: .zip

Change History (25)

#1 @Thomas Hicks
9 years ago

  • Summary changed from Change in formatting.php [ Version Wordpress 4.2.4] to add wpnl comment breaks sites. to Change in wp-includes/class-wp-embed.php ? [ Version Wordpress 4.2.4] to add wpnl comment breaks sites

#2 @Thomas Hicks
9 years ago

For information: Affected post on site (for reference)

code in post:

<!--<object width="528" height="580">
    <param name="movie" value="http://www.example.com/wp-content/uploads/2012/03/old1.swf">
    <param name="wmode" value="transparent" />
    <embed src="file.swf" width="528" height="580">
    </embed>
</object>

<object width="520" height="520">
    <param name="movie" value="http://www.example.com/wp-content/uploads/2012/03/old2.swf">
    <param name="wmode" value="transparent" />
    <embed src="file.swf" width="520" height="520">
    </embed>
</object>


<object width="520" height="385">
    <param name="movie" value="http://www.example.com/wp-content/uploads/2012/03/old3.swf">
    <param name="wmode" value="transparent" />
    <embed src="file.swf" width="520" height="385">
    </embed>
</object>
-->
...SNIP...

code produced on page by Wordpress:

 <div class="section-content"><p><!--<object width="528" height="580"><!-- wpnl -->      <!-- wpnl --><param name="movie" value="http://www.example.com/wp-content/uploads/2012/03/old1.swf"><!-- wpnl -->      <!-- wpnl --><param name="wmode" value="transparent" /><!-- wpnl --><embed src="file.swf" width="528" height="580"><!-- wpnl --></embed><!-- wpnl --></object>

<object width="520" height="520"><!-- wpnl -->      <!-- wpnl --><param name="movie" value="http://www.example.com/wp-content/uploads/2012/03/old2.swf"><!-- wpnl -->      <!-- wpnl --><param name="wmode" value="transparent" /><!-- wpnl --><embed src="file.swf" width="520" height="520"><!-- wpnl --></embed><!-- wpnl --></object>


<object width="520" height="385"><!-- wpnl -->      <!-- wpnl --><param name="movie" value="http://www.example.com/wp-content/uploads/2012/03/old3.swf"><!-- wpnl -->      <!-- wpnl --><param name="wmode" value="transparent" /><!-- wpnl --><embed src="file.swf" width="520" height="385"><!-- wpnl --></embed><!-- wpnl --></object>
--></p>
...SNIP...

#3 @swissspidy
9 years ago

  • Summary changed from Change in wp-includes/class-wp-embed.php ? [ Version Wordpress 4.2.4] to add wpnl comment breaks sites to wpautop breaks HTML comments

#4 @johnbillion
9 years ago

  • Keywords needs-patch needs-unit-tests needs-testing added
  • Milestone changed from Awaiting Review to 4.2.5

Thanks for the report, Thomas!

#5 @miqrogroove
9 years ago

I wrote a new patch for this but it's still not working. The block element regex is also doing weird things to PARAM elements. :(

@miqrogroove
9 years ago

Fixes newlines around objects. Fixes confusion of P and PARAM. Adds unit test.

#6 @miqrogroove
9 years ago

  • Keywords has-patch added; needs-patch needs-unit-tests needs-testing removed
  • Owner set to miqrogroove
  • Status changed from new to accepted

Fixed and tested both bugs in attachment.

#7 @miqrogroove
9 years ago

Is the Milestone supposed to be 4.3.1 now?

#8 @SergeyBiryukov
9 years ago

  • Milestone changed from 4.2.5 to 4.3.1

@miqrogroove
9 years ago

Optimized regexp.

#9 @miqrogroove
9 years ago

Explanation of optimizations in the second patch:

In this particular regexp, we are not actually looking for the end of an element, but the end of an element's name. It is therefore not always necessary to find a > character because the name can also be delimited by whitespace. For improved speed and readability, I have reduced this term to simply [\s>] The same tests still pass.

#10 @wonderboymusic
9 years ago

  • Keywords commit added

#11 @wonderboymusic
9 years ago

  • Keywords commit removed
  • Owner changed from miqrogroove to azaozz
  • Status changed from accepted to assigned

Ozz will approve when ready

#12 follow-up: @azaozz
9 years ago

This is caused by not replacing the line break placeholders <!-- wpnl --> at the end of wpautop(). So the first line of the patch is not needed in order to fix it.

I'm also a bit unsure about '!(<' . $allblocks . '[\s>])!' in a dot release. Before we used to match the whole tag, now we match just the beginning. That looks fine but might introduce other edge cases and we don't have adequate tests for it. Also, it has to be '!(<' . $allblocks . '[\s/>])!' to match things like <hr/>.

How about we add 33645.2.patch without the first line to 4.3.1, and add the revised patch to 4.4?

Also, ideally we should exclude HTML comments from wpautop completely, same as the content of CDATA, <script>, and <style> tags, perhaps similarly to how <pre> tags are handled.

Last edited 9 years ago by azaozz (previous) (diff)

@azaozz
9 years ago

@azaozz
9 years ago

#13 @miqrogroove
9 years ago

That all looks good. I will add one more unit test for the extra slash.

#14 @miqrogroove
9 years ago

  • Keywords wpautop added

In 33645-4.4-expanded.patch one of the tests will fail if wpautop doesn't insert whitespace between self-closing blocks.

#15 @azaozz
9 years ago

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

In 33955:

Formatting: maintain the content of HTML comments when they contain <object> tags. Add more tests for wpaitop().

Props miqrogroove.
Fixes #33645 for trunk.

#16 @azaozz
9 years ago

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

Reopen for 4.3.1 consideration. The prefered patch for there is 33645-4.3.1.patch

Version 0, edited 9 years ago by azaozz (next)

#17 @azaozz
9 years ago

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

In 34024:

Formatting: fix removing line break placeholders from HTML comments at the end of wpautop().

Props miqrogroove.
Fixes #33645 for 4.3.

#18 @johnbillion
9 years ago

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

Needs unit tests

#19 @johnbillion
9 years ago

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

#20 in reply to: ↑ 12 @lkraav
8 years ago

Replying to azaozz:

Also, ideally we should exclude HTML comments from wpautop completely, same as the content of CDATA, <script>, and <style> tags, perhaps similarly to how <pre> tags are handled.

Do I read this correctly: this part was never handled, leading to #33834?

Here's a dead simple test case to use:

  • tests/phpunit/tests/formatting/Autop.php

     
    524524                $this->assertEquals( $expected, trim( wpautop( $content ) ) );
    525525        }
    526526
     527        /**
     528         * wpautop() should skip HTML comments
     529         *
     530         * @ticket 33645
     531         */
     532        function test_that_wpautop_skips_html_comments() {
     533                $content = '
     534        <!-- .column .something -->
     535';
     536
     537                $expected = '
     538<!-- .column .something -->
     539';
     540
     541                $this->assertEquals( $expected, trim( wpautop( $content ) ) );
     542        }
     543
    527544}
Last edited 8 years ago by lkraav (previous) (diff)
Note: See TracTickets for help on using tickets.