Line break filter corrupts existing XHTML

This is actually bigger than the original title and I'm kinda shocked it has survived so many cycles of Drupal Core.

The linebreak filter is actually from Wordpress (http://ma.tt/scripts/autop/), where it is also broken and people hate it.

Here's an experiment which works on both D6 and D7:

<strong> Line 1 \n Line2\n\n Line3</strong>

becomes

<p><strong>Line1<br/>Line2</p><p>Line 3</strong></p>

I'm all for semantically correct HTML, but would prefer using nl2br in this case if we are going to generate invalid XHTML.

What would be the desired output for that, though? GIGO. Are we going to fix the user's input error of stretching a span-level tag across blocks? That'd be a lot of work and might cause trouble with regards to computers trying to be smarter than humans. Perhaps escape the <strong> tags to make it more apparent something's broken?

It's not the user's input error. The user provided proper HTML. The linebreak filter produces invalid XHTML by trying to wrap the content in

tags and doing it badly.

But it is, though, because conceptually, the user is still spreading a span level tag (<strong>) across two blocks (the <p>s implied by the double line break). I'm not saying this isn't an honest, common mistake; I'm just not sure it's the job of the line-breaks-to-HTML function to fix it. If anything, it's a job for the HTML corrector filter.

EDIT: Or are you saying that we simply should not HTML-ize line breaks inside span-level tags?

However, the behavior as demonstrated in the OP's example is definitely wrong.

Wouldn't the correct behavior be to break up the span level tag?

<strong>Line 1 \n Line2\n\n Line3</strong>

becomes
<p><strong>Line1<br/>Line2</strong></p><p><strong>Line 3</strong></p>

But I agree, that might not fit in the scope of the line break filter, and might be something to try to catch in HTML corrector. Then again, the actual structural error was introduced by autop, so I'm not sure.

Looking at this chunk of code further… It looks like we're setting ourselves up for failure from the outset by trying to define all block-level tags. I notice this patch adds a couple, but there's still a bunch still missing, particularly new HTML 5 ones - <canvas>, <article>, <header> and <footer>, etc. Is there some way to determine if a tag is block-level without a list like this…? Or am I in fantasy-land?

Does anyone know how other text filters handle this? I'm going to take a peek at what PHP Markdown is doing…

That's fine for D7 and beyond, but D6 still supports PHP4, so perhaps our best bet for fixing D6 is to test for whatever PHP5 features we need and use a DOM parser if possible (since most D6 sites should be on PHP5), and revert to the old parser for PHP4.

I don't like that idea. It would leave things open too much for inconsistent behavior depending on which version of PHP is running.

Thinking over this, I thought the situation might be better if we stripped out whitespace between HTML tags, so that _filter_autop() wouldn't have any line breaks to get tripped up on. So I ran this through:

<form action="http://example.com/"><fieldset class="collapsible collapsed"><legend>This is the legend</legend><div class="collapse-text"><p>Paragraph text</p></div></fieldset></form>

Despite any wrapping your browser may be doing, there's no actual line breaks in there. But here was the output:

<form action="http://example.com/"><fieldset class="collapsible collapsed"><legend>This is the legend</legend><br />
<div class="collapse-text">
<p>Paragraph text</p>
</div>
<p></fieldset></form>

What the snot?! How is this function finding line breaks that don't exist?! Why is it inserting its own?! This thing has got to go!

I agree, and I'm in favor of HTMLPurifier for the HTML fixing and for the XSS security.

-J

I noticed that the previous patches added <legend> as block-level, and it is in fact inline and shouldn't be added.

I also found a couple regexp's that use the $block snippet without including "[^>]*" at the end, so any whitespace or attributes at the end of a block-level tag cause unexpected results. That's why there's a useless <br /> after <legend> in #13, since the next tag is a <div> with a class.

There are obviously more problems than just with non-block elements. I did a quick test with HEAD:

$test = '<div>Paragraph text

some more text
</div>';

$test = _filter_autop($test);

The output:

<div>Paragraph text
</p>
<p>some more text
</div>

... so wrong!

Actually, <legend> should probably be added, since structurally it is never used in the flow of text, even if it is technically non block-level. Adding <legend> to $block does fix the stray <p> after the </div>. However I do agree that it could get ridiculous adding tags to these hard-coded lists as the bugs crop up. Looking at WordPress's version, they have added quite a few tags to the list since we borrowed the code.

WordPress's current code has the same bugs...but they probably run a balance tags function to correct the output, which would explain a lot :) In Drupal at least, the line break filter should probably be self-contained and not depend on the HTML corrector filter. People expect "faulty and chopped off HTML" to be user-entered, not resulting from a series of crappy regexp's :P

I'll experiment with some document parsing too, although I realize D8 has an initiative for that in #659580: Specify what the line break converter should do and rewrite it in DOM.

Here's a new version of the filter that should avoid some of the egregious tag balancing errors. There are undoubtedly bugs with this code though, so testing is welcome.

.

#17: filter-autop.2.patch queued for re-testing.

This version keeps track of a tag stack, which is used to determine what type of tag we're inside of. </p><p> is avoided inside inline tags and <br /><br /> is substituted. Also did some bug fixes and tests. My version of the filter is more complex than the old one, but adds the ability to insert logic into the process.

It's kind of a work in progress right now, and needs more testing, but so far I think it produces the expected results from the test snippets posted. Example:

Input:

<strong> Line 1 \n Line2\n\n Line3</strong>

Output:

<p><strong> Line 1 <br />
 Line2<br /><br />

 Line3</strong></p>

More bug fixes and unit tests. The tests include running the filter through some complex HTML examples, and the new filter is holding up pretty well.

The new tests use a new method, FilterUnitTestCase->assertTagOrder(), which is useful for making sure new tags are added in the right places and nesting is correct. It allows the filter to have some freedom to change the whitespace in the HTML a bit, as long as the tag order is good.

Here's a comparison of the new vs. old autop filter when the HTML contains extra spacing:

Input:

<form action="http://example.com/"><fieldset class="collapsible collapsed">

<legend>This is the legend</legend>    <div class="collapse-text">

      Paragraph text  </div>  </fieldset>


</form>

HEAD autop:

<form action="http://example.com/"><fieldset class="collapsible collapsed"></p>
<p><legend>This is the legend</legend><br />
<div class="collapse-text">
<p>      Paragraph text  </div>
<p>  </fieldset></p>
</form>

patched autop:

<form action="http://example.com/"><fieldset class="collapsible collapsed">

<legend>This is the legend</legend>    <div class="collapse-text">

      <p>Paragraph text</p>  </div>  </fieldset>


</form>

:)

is using <br /><br /> correct?

I think we can presume that the user intended a paragraph break. More, the default drupal texareas do not add (AFAIK) <p></p> tags to the code and double line breaks are used unless the person has configured a wysiwyg textarea along with the necessary changes to the input format.

I don't think it is a good idea to have <br /><br /> as the output of what is the default situation.

Fair enough, instead of <br /><br /> we now close the inline tags in the current tag stack and start a new paragraph:

Input:

First <em class="nice">Line

Two lines</em> later

Output:

<p>First <em class="nice">Line</em></p>

<p><em class="nice">Two lines</em> later</p>

Re: the second statement, let's be clear that the changes I'm making to this filter don't result in <p> being added to any textareas, since Drupal doesn't run filtered output back through forms. Further, the filter is also built to respect and not mess with <p></p> that a user or wysiwyg editor may enter into a textarea. So the changes here don't affect anything other than fixing the completely broken markup that the current filter produces.

Made a correction to the tag stack logic and tweaked the test to check if we close the inline tags before closing <p>.

Can you fill in the doc blocks please. (add @param and @return blocks)

Also to clarify: we were only using <br /><br /> in the limited situation when an inline tag was open and we didn't want to create invalid XHTML by ending the paragraph in the middle of that. This behavior was suggested in comment #9. The new patch's behavior is equivalent to #8, which as you say is perhaps more what the user intended.

I think using <br /><br /> in span-level tags instead of closing and re-opening it around a paragraph break is simpler and more predictable behavior, and less prone to causing problems. It may not technically be "correct," but it will still result in output that the user, who is most likely a novice if they're making this mistake, is expecting. +1 for going back to that approach.

-1 to going back to that approach unless we find specific problems - drupal does not ship with a wysiwyg. If someone enters tags manually or using a tag editor like bueditor/markitup etc, by default there will be no paragraph markup. A person can easily highlight a section of content to bold or make italic (strong/em) and expect the markup that comes out at the end to be good.

If we can fix any problems that arise, I think doing it properly is worth it.

Saying that, here are my findings:

If the line break filter is before the htmlcorrector: (default)

- strong, em and span apply to the first two paragraphs only and not to subsequent ones.
- del only applies to one paragraph.

If the line break filter is after the htmlcorrector:

- strong, em and del work properly (tested with 4 paragraphs only though) and each paragraph has the correct breaking and opening.

- span however only applies to the first two.

@nbz:

-1 to going back to that approach unless we find specific problems - drupal does not ship with a wysiwyg. If someone enters tags manually or using a tag editor like bueditor/markitup etc, by default there will be no paragraph markup. A person can easily highlight a section of content to bold or make italic (strong/em) and expect the markup that comes out at the end to be good.

There are specific problems. See my earlier comments. It's absolutely reproducible and very easy to do on a stock install. Just add a double linebreak inside of a <strong> or <code> tag. Produces bad XHTML.

If we can fix any problems that arise, I think doing it properly is worth it.

We got the autop script from wordpress. autop is broken there too.

http://7and1.net/2009/08/17/how-can-i-disable-wps-fucking-autop-filter/
http://urbangiraffe.com/plugins/disable-wpautop/

On my own WP site I did a test:
http://pajamadesign.com/2010/05/26/test-silly/

See the source there, it produces invalid XHTML.

We should certainly forgo semantic "correctness" for syntax correctness until the problem is properly solved (I vote for HTMLPurifier).

There is a real bug here, and it has nothing to do with the HTML corrector.

-J

I do not see any critical functionality being broken here.

+++ modules/filter/filter.module	25 May 2010 10:10:17 -0000
@@ -1359,13 +1359,10 @@ function _filter_htmlcorrector($text) {
+  global $_p_open, $_tag_stack;

These global variables need to go.

+++ modules/filter/filter.module	25 May 2010 10:10:17 -0000
@@ -1394,30 +1392,115 @@ function _filter_autop($text) {
+  $blocklevel = _filter_autop_blocklevel_tags() . '|legend';

Why is legend not in the block level tags list, but added here?

+++ modules/filter/filter.module	25 May 2010 10:10:17 -0000
@@ -1394,30 +1392,115 @@ function _filter_autop($text) {
+function _filter_autop_blocklevel_tags() {
...
+function _filter_autop_inline_tags() {

I think these should be variables.

78 critical left. Go review some!

- Converted globals to drupal_static().
- Got rid of block/inline taglist functions, side-effect being that $blocklevel is now hardcoded in two different functions. Added <legend> to $blocklevel since we don't want it wrapped in <p></p>.
- Went back to <br /><br /> when inside inline tags. With the ...</strong></p><p><strong>... approach, it gets difficult to maintain that across multiple paragraphs, and ensure that nothing breaks between those paragraphs. Also the problem when you might have <strong id="some-id"> and then if you re-create the strong you would have to strip out the id attribute. <br /><br /> is not as pretty but should avoid all those types of complications.
- Added doxygen.

Wow! Really impressive effort here. The tests are much better, and the behavior is a lot more sane! Ditto to Sun's comment about legend kinda hanging out there by itself. Is there a reason for that?

Best,
Jacob

"del" is not listed in inline tags and any attributes for the span tag seem to be stripped by the line break filter (I tried <span style="color:blue"> but the parsed results gave <span>)

Setting to CNW

More, the correctness of the output using <br /><br /> seems to so far be the same as using <p></p> so I am not totally convinced by that argument.

Saying that, any improvement is better than none.

Added del to inline tag list, and could not reproduce the attribute stripping bug. Added a quick test to assert that attributes are preserved. nbz, could you post the raw text input you used for which attributes were stripped?

What I am using to test:

<strong>Line one of strong Text

Line two of strong text

Line three of strong text

Line four of strong text</strong>


<em>Line one of emphasised text

Line two of emphasised text

Line three of emphasised text

Line four of emphasised text</em>


<del>Line one of deleted text

Line two of deleted text

Line three of deleted text

Line four of deleted text</del>


<span style="color:blue;">Line one of blue text

Line two of blue text

Line three of blue text

Line four of blue text</span>

The last one there strips the style statement from the tag. I am also seeing different results in node teaser (in this case the front page for that text), but I am not sure if that is a cache issue or something else (I ahve been clearing the cache after making any changes.)

I ran your example through _filter_autop() and the output looked good. The attribute stripping you're seeing must be from a different filter, probably "Limit allowed HTML tags". That filter strips all style attributes, I assume for XXS-prevention. Try using Full HTML text format, or run the text directly through _filter_autop() to make sure that no other factors are interfering. This issue strictly addresses the _filter_autop() function and all we're trying to do is make it output correct XHTML. I use a test.php in the Drupal root to do testing:

define('DRUPAL_ROOT', getcwd());
require_once DRUPAL_ROOT . '/includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
header('Content-Type: text/plain; charset=utf-8');

$test = <<<EOT
The <em>lazy dog</em>
jumps over

the quick brown fox.
EOT;

print _filter_autop($test);

Indeed, style attributes get stripped.

This is a big problem for WYSIWYG editors. The DrupalGardens team is working on porting HTMLPurifier to DRUPAL-7 to solve this problem.

-J

#39: filter-autop.8.patch queued for re-testing.

Quick re-roll without the new tests. The autop tests in HEAD have changed, so let's see how the patch fairs.

This is a totally nice effort. However, we absolutely have to start with writing tests. Leveraging the new FilterUnitTestCase::assertFilteredString() method. That is, because as of now, we have close to zero expectations for the line break filter.

So what I'd like to see in the next patch here is no filter.module changes (for now) and only new test assertions in filter.test's testLineBreakFilter(), which prove that the our expectations (whatever they are) are not met. The new assertions should follow the coding style of the existing in testLineBreakFilter() as well as testUrlFilter(). Extra points for finding a way to categorize assertions in some way, like testUrlFilter() does.

I already attempted to do this myself, but I had no clue at all what our expectations for this filter actually are. Therefore, this is the main, primary, and most important challenge to solve, before doing anything else.

After adding expectations, we can add back the filter.module hunks, if necessary, or fix the bugs in another way, whatever makes sense. But please understand that I'm not able to review or even RTBC a patch that changes the behavior of one of Drupal core's default input filters without adding unit tests.

Also, please don't take that "or fix the bugs in another way" as an insult... you hard work is highly appreciated! I merely want to point out that our copy/fork of WP's autop is outdated: #900956: Update autop filter to include missing block level tags -- so that may be another possibility to try to fix the bugs.

The expectation is pretty basic: post valid XHTML in your node content, and the line break filter should not produce invalid XHTML out of that. I wrote tests for this (see the last few hunks of #39) which check the tag order of the output, if there are stray </p> etc, the tests will fail. The tests are very specific in the tag order though, so there's some improvement to be done (maybe simply checking that no invalid nesting is going on, AND that we have at least some paragraph tags that weren't already there, rather than a specific tag order). I didn't include the tests in #44 because the HEAD tests have changed, so I want to test against those and I need to re-roll my tests.

By the way, the current version of the WP filter is just as broken, so I don't think upgrading will fix anything. The problem is that in WP, they use an HTML corrector every time to clean up the preg_replace() mess (there are lots of stray tags left over), while we leave HTML corrector as a separate filter and therefore the line break filter is not sufficient in itself.

I'll work on the tests and post that separately so we can see the problem clearer.

Here's a manual test, which needs to be converted into a SimpleTest:

1. Create a new text format using only the line break filter.
2. Create a node using the new text format with any sort of XHTML. Example:
   

   <div>
   Paragraph text
   
   some more text
   </div>
   

3. Run the page through an XHTML validator.

The result is usually broken XHTML -- failures attached.

Here are some tests written around the assumption that we can use _filter_htmlcorrector() to act as our XHTML validator, implemented as assertCorrectHTML(). If _filter_htmlcorrector() returns different content, we have reason to believe we gave it invalid XHTML.

Note that test 2 fails, but test 3 passes. The only difference is an added "pre" tag, which is curious. Somehow the "pre" tag screws up the processing of content below it.

Apply patch #44 and the assertCorrectHTML()'s do pass, but other tests fail because they are overly specific to WP autop. WP autop totally changes the whitespace of the original content, adding newlines as it pleases, while my autop tries to keep the original whitespace. WP autop also strips out empty paragraph tag pairs, which IMO is changing the input unnecessarily so I also avoid that. The current tests need to be rewritten to be more flexible and not check for exact output. Having a whitespace-insensitive equality check would be a start, and from there we can decide on the behavior, i.e. should the filter strip out empty p tag pairs, and should the filter add paragraphs inside block level tags.

I found that the HTML corrector actually does change the whitespace in some cases, which could cause assertCorrectHTML() to fail. I now added assertIdenticalWithoutWhitespace(), which should adjust for that. Also now asserting that our source is valid HTML, before we run it through a filter.

humm... why didn't you go with the existing assertFilteredString() method? PHP's DOM implementation can have bugs, too. I'd highly prefer to see regular input/output assertions, just like all the other revised LineBreakFilter/UrlFilter tests.

At first I tried assertFilteredString() but it didn't seem to suit the situation since I'm not checking for exact output, just that the output is valid HTML. The code would be like:

$test1 = "<div>\nParagraph text\n\nMore text\n</div>";
$filtered = _filter_autop($test1);
$corrected = _filter_htmlcorrector($filtered);
$tests[$test1] = array($corrected => TRUE);
$this->assertFilteredString($filter, $tests);

Even then, the assertion doesn't work because the comparison needs to ignore whitespace, since (I discovered) _filter_htmlcorrector() may change the whitespace of already valid HTML. assertCorrectHTML() is the alternative I came up with.

That's exactly what I mean. These are unit tests, so HTML corrector should not be involved at all. It adds entirely different expectations to the Line break filter's expectations, which invalidates the entire test. We only want to test _filter_autop(), and nothing else. Therefore, we want and have to use simple input/output assertions.

My idea was to make the tests flexible in terms of whitespace, since the current autop filter adds gratuitous whitespace as a side-effect of its sloppy preg_replace() mechanism. I'd like to transition to my implementation, which preserves whitespace. To transition though we would need to rewrite the tests to expect different whitespace, if we use simple input/output assertions. We can certainly do that, I just think (ideally) the assertions shouldn't be so specific about whitespace, which in XHTML doesn't matter a whole lot, but tag nesting/order certainly matters.

Returning to #45, I think we just need to document all the gory details of what this filter currently does, and what it ideally should do (with respect to tags added/removed and whitespace added/removed). Only then can we write "pure" input/output tests. I was hoping to get by by just adding the expectation "output should be valid XHTML, given valid XHTML as input" and fixing that aspect. I guess that's not really going to cut it :)

Trying to make sense of the major queue currently. This might be an annoying bug, but doesn't affect the majority of users, and can also be circumvented in many ways. Demoting to normal.

subscribe

but there's still a bunch still missing, particularly new HTML 5 ones - , , and , etc. Is there some way to determine if a tag is block-level without a list like this…? Or am I in fantasy-land?

Well while this issue got totally stuck, the front-end team managed to fix most HTML5 tags in #1280522: _filter_autop (line break converter) does not recognize html5 block level elements. Figures that's what you get when you don't over-think things. :P

I've rerolled a patch similar to deviantintegral's original patches that adds remaining missing tags over in #900956: Update autop filter to include missing block level tags.

The only working solution we found is using HTML Purifier module (to fix broken HTML) afterwards

Possibly related: #1938746: _filter_autop inserts </p> without <p>