WordPress.org

Make WordPress Core

Opened 11 years ago

Last modified 11 days ago

#2691 accepted defect (bug)

HTML comments in posts aren't handled properly.

Reported by: gord Owned by: adamsilverstein
Milestone: 5.0 Priority: normal
Severity: normal Version: 2.8.5
Component: Formatting Keywords: wpautop dev-feedback has-patch has-unit-tests
Focuses: Cc:

Description

When an HTML comment is added in a post, autop adds paragraph ( <p> ) tags around the comment and for multi-line comments line breaks ( <br /> ) are added after every line. This should not happen in HTML comments.

This ticket is similar to #712 which was closed with wontfix. I would like to know why this isn't seen as an issue? It prevents the addition of RDF and other metadata, not to mention just plain old HTML comments in posts.

Attachments (5)

2691.diff (1.5 KB) - added by collinsinternet 3 years ago.
Unit tests for HTML comments.
2691.2.diff (3.0 KB) - added by collinsinternet 3 years ago.
Adjustments to formatting.php to accommodate HTML comments.
2691.3.diff (3.4 KB) - added by adamsilverstein 9 months ago.
2691.4.diff (4.2 KB) - added by adamsilverstein 6 months ago.
2691.5.diff (5.1 KB) - added by adamsilverstein 6 months ago.

Download all attachments as: .zip

Change History (25)

#1 @skeltoac
11 years ago

This is what is implied: nobody who has seen this has found it important enough to spend their time rewriting autop to work around comments. The applicable mantra is "edge case." You are welcome to work up a patch and submit it for testing and consideration.

#2 @random
11 years ago

Maybe a bandaid on it? (e.g.)

$pee = preg_replace('!<p><!--(.*?)--></p>!ise', " '<!--' .  stripslashes(clean_pre('$1'))  . '-->' ", $pee);

clean_pre() will ruin any comments which are supposed to have <p> and <br /> in them, but that's an even edgier case.

Gord, you could also try a plugin replacement for wpautop -- I know Markdown handles comments fine.

#3 @Nazgul
10 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

No traction in over a year, so closing as wontfix.

Feel free to reopen if you have patches/suggestions/...

#4 @vikingjs
8 years ago

  • Cc vikingjs added
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Version changed from 2.0 to 2.8.5

I can't believe this got no traction - I use plugins that conscientiously mark where their added content begins and ends using HTML comments, and those comments are adding empty paragraphs in my posts, which in some cases affect layout and undermine the aesthetics of the page.

This issue affects anyone who uses plugins that modify the text of the blog post, e.g., SexyBookmarks.

I'll look at what it would take to patch it, but someone who knows how and where the unwanted behavior is occurring could do it in a fraction of the time.

#5 follow-up: @scribu
8 years ago

  • Keywords wpautop needs-patch added; autop removed
  • Milestone set to Future Release

The function responsible is called wpautop() and can be found in /wp-includes/formatting.php

#6 in reply to: ↑ 5 @vikingjs
8 years ago

Replying to scribu:

The function responsible is called wpautop() and can be found in /wp-includes/formatting.php

I took a look and I'm probably not the guy to be fixing this. As far as I can tell the band-aid advised by random above is a good fix. The reason I think I'm not the guy is because I don't know why stripslashes() and clean_pre() are needed for a comment, so I bow to the superior understanding of others.

random calls his patch a band-aid, but it would be just another in a long list of band-aids in the function. The philosophy seems to be to add paragraph tags and then go back and repair special cases where they broke things. This would be just another special case. Ideally it would be better simply not to apply the wpautop routines to comments (or some of the other cases like embed tags) in the first place.

#7 @mrclay
7 years ago

  • Cc mrclay added

I've created a more resilient autop implementation and a more rigorous testcase cobbled from various WP tickets. It avoids the dangerous regex operations by doing most work in a DOMDocument object.

This is undoubtedly slower than current wpautop (and may choke on extremely invalid HTML), but happily handles markup that the current wpautop breaks. Since there's no real spec for wpautop, it's hard to duplicate the style of markup wpautop outputs when it does work, but a lot of testing will surely be necessary.

#8 @nacin
4 years ago

  • Component changed from General to Formatting
  • Keywords needs-unit-tests added

@collinsinternet
3 years ago

Unit tests for HTML comments.

@collinsinternet
3 years ago

Adjustments to formatting.php to accommodate HTML comments.

#9 @collinsinternet
3 years ago

  • Keywords dev-feedback added

Hey All,

2691.2.diff allows my unit tests for HTML comments to pass, however it caused some others related to video and embed objects to fail. Can I get another set of eyes on this to make sure I'm not missing anything?

Last edited 3 years ago by collinsinternet (previous) (diff)

#10 @miqrogroove
2 years ago

I'm working on a new patch for this and want some fresh comments regarding @collinsinternet patch which attempts to avoid adding BR directly after an HTML comment.

Is the BR after the comment part of this bug? Or is it only the BR that gets added inside a comment?

Would it be a step in the right direction for wpautop to start avoiding HTML comments but still add a BR when the comment is followed by a single newline character?

What is the desired behavior when an HTML comment is surrounded by multiple newline characters?

#11 @adamsilverstein
9 months ago

In 2691.3.diff :

  • refactor unit tests using dataProvider
  • only remove the tailing </p> after an HTML comment when the paragraph starts as a comment

*adjust the regex for adding breaks after comments - exclude cases where a closing </p> tag is already present

all of the formatting group unit tests now pass. appreciate testing & feedback.

#12 @adamsilverstein
6 months ago

2691.4.diff updates unit tests for test_multiline_comment_with_embeds to pass after this change, removing some stray <br /> tags in the expected results.

#13 @adamsilverstein
6 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

@nacin or @azaozz any feedback here? does 2691.4.diff look good?

This is an 11 year old ticket and we should get this fixed! This might affect stored block content for Gutenberg (which uses HTML comments to store blocks).

#14 @adamsilverstein
6 months ago

  • Owner set to adamsilverstein
  • Status changed from reopened to accepted

#15 @adamsilverstein
6 months ago

2691.5.diff adds some additional tests for the examples in #2833.

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


5 months ago

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


5 months ago

This ticket was mentioned in Slack in #core by pento. View the logs.


5 months ago

#19 @adamsilverstein
3 months ago

  • Milestone changed from Future Release to 4.9

#20 @adamsilverstein
11 days ago

  • Milestone changed from 4.9 to 5.0

Punting from 4.9. This needs more testing and feedback.

Note: See TracTickets for help on using tickets.