wpautop breaks HTML comments

[Thomas Hicks]

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 --> " ) );

[Thomas Hicks]

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

[johnbillion]

Thanks for the report, Thomas!

[miqrogroove]

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]

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

[miqrogroove]

Fixed and tested both bugs in attachment.

[miqrogroove]

Is the Milestone supposed to be 4.3.1 now?

[miqrogroove]

Optimized regexp.

[miqrogroove]

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.

[wonderboymusic]

Ozz will approve when ready

[azaozz]

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.

[miqrogroove]

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

[miqrogroove]

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

[azaozz]

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.

[azaozz]

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

[azaozz]

In 34024:

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

Props miqrogroove.
Fixes #33645 for 4.3.

[johnbillion]

Needs unit tests

[lkraav]

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

@@ -524 +524 @@
                 $this->assertEquals( $expected, trim( wpautop( $content ) ) );
         }
 
+        /**
+         * wpautop() should skip HTML comments
+         *
+         * @ticket 33645
+         */
+        function test_that_wpautop_skips_html_comments() {
+                $content = '
+        <!-- .column .something -->
+';
+
+                $expected = '
+<!-- .column .something -->
+';
+
+                $this->assertEquals( $expected, trim( wpautop( $content ) ) );
+        }
+
 }