Convert line breaks into HTML (i.e.<br> and <p>) stops working after a comment.

To reproduce:

  1. Install drupal 7 dev, do not touch filter module.
  2. Create a new node using full html format
  3. Paste this 
1- This is the first paragraph and the summary. It contains a break comment at the end.<!--break-->

2- This is the second paragraph.

3- This is the third paragraph. It should be an opening p tag at the beginning and a closing p tag at the end, in the generated source code.

This issue was found by federico in #881006: Regression: 'break' tag doesn't work with Filtered HTML #16. As this is a another issue, I created this new issue.

This issue results from #559584: filter_xss() and Line break filter break HTML comments and is a bug of the line/paragraph filter.
$open = ($chunk[1] != '/' || $chunk[1] != '!'); is always true in the function _filter_autop().
Better is $open = ($chunk[1] != '/' && $chunk[1] != '!');

Changes in the patch: I've modified the filter.module and improved the tests. Note: I commented out (marked with TODO) the iframe test since it seems not useful for the line/paragraph filter. Should we treat the <iframe> tag as the other tags such as <pre> in the line break filter? Maybe this was the intention.
$chunks = preg_split('@(<!--.*?-->|</?(?:pre|script|style|object|!--)[^>]*>)@i', $text, -1, PREG_SPLIT_DELIM_CAPTURE);

I've replaced in the tests the "found" message text with "expected" as this seems clearer.

CommentFileSizeAuthor
#4 filter_comment_1029108_4.diff2.93 KBscito
filter_comment_issue.diff2.99 KBscito
filter_comment_issue_tests_only.diff2.44 KBscito

Comments

Status: Needs review » Needs work

The last submitted patch, filter_comment_issue_tests_only.diff, failed testing.

scito’s picture

scito
Primary language German
commented
Status: Needs work » Needs review

The correct example for reproduction is

1- This is the first paragraph and the summary. It contains a break comment at the end.<!--break-->

2- This is the second paragraph.

3- This is the third paragraph. It should be an opening p tag at the beginning and a closing p tag at the end, in the generated source code.
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
+++ modules/filter/filter.test
@@ -797,8 +797,20 @@ class FilterUnitTestCase extends DrupalUnitTestCase {
-      "aaa\nbbb\n\nccc" => array(
-        "<p>aaa<br />\nbbb</p>\n<p>ccc</p>" => TRUE,
+      "aaa\nbbb\n\nccc\n\nddd" => array(
+        "<p>aaa<br />\nbbb</p>\n<p>ccc</p>\n<p>ddd</p>" => TRUE,
+      ),

Please revert.

+++ modules/filter/filter.test
@@ -797,8 +797,20 @@ class FilterUnitTestCase extends DrupalUnitTestCase {
+      // Comments remain unchanged;
+      // single line breaks should be changed to <br /> tags, while paragraphs
+      // separated with double line breaks should be enclosed with <p></p> tags.
...
+      // Preformatted tags remain unchanged;
+      // single line breaks should be changed to <br /> tags, while paragraphs
+      // separated with double line breaks should be enclosed with <p></p> tags.

No need to repeat the comment from the first test; this one is about comments, so either provide details on what is expected for comments, or remove the 2nd and 3rd comment line.

+++ modules/filter/filter.test
@@ -797,8 +797,20 @@ class FilterUnitTestCase extends DrupalUnitTestCase {
+      "aaa<!--comment-->\n\nbbb\n\nccc\n\nddd" => array(
+        "<p>aaa</p>\n<!--comment--><p>\nbbb</p>\n<p>ccc</p>\n<p>ddd</p>" => TRUE,

I'd like to see second comment in the source input that contains line breaks.

Also, you don't need to repeat the entire line in the expected string; you can do:

"<!--comment-->" => TRUE,
"<!-- A comment\n with linebreak -->" => TRUE,

+++ modules/filter/filter.test
@@ -811,7 +823,7 @@ class FilterUnitTestCase extends DrupalUnitTestCase {
-        "<iframe>aaa\nbbb\n\nccc</iframe>" => TRUE,
+        //TODO: "<iframe>aaa\nbbb\n\nccc</iframe>" => TRUE,

So needs work.

+++ modules/filter/filter.test
@@ -1457,7 +1469,7 @@ www.example.com with a newline in comments -->
-            . '<hr />' . ($is_expected ? 'Found:' : 'Not found:')
+            . '<hr />' . ($is_expected ? 'Is expected:' : 'Is not expected:')

Please remove the leading "Is" from both.

Powered by Dreditor.

scito’s picture

scito
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new filter_comment_1029108_4.diff2.93 KB

I improved the patch according to your comments.

Remarks:

  • Testing the expected paragraphs after the comments is necessary since the tests would otherwise pass with the unpatched code.
  • I added iframe to the $chunks. Either we add it there or we remove it from the tests since the iframe test is failing with the patched code.
scito’s picture

scito
Primary language German
commented

#1054632: Lines and paragraphs stop converting to individual p and br tags after pre tag. is a duplicate.

scito’s picture

scito
Primary language German
commented

#1063178: Line break filter will ignore everything following a <pre>xxx</pre> is related or a duplicate.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Closed (duplicate)

Marking as a duplicate of #1063178: Line break filter will ignore everything following a <pre>xxx</pre>, which has a proper fix (the logic around comments is totally bogus right now).