From http://stackoverflow.com/questions/1732348/regex-match-open-tags-except-... :

You can't parse [X]HTML with regex. Because HTML can't be parsed by regex. Regex is not a tool that can be used to correctly parse HTML. As I have answered in HTML-and-regex questions here so many times before, the use of regex will not allow you to consume HTML. Regular expressions are a tool that is insufficiently sophisticated to understand the constructs employed by HTML. HTML is not a regular language and hence cannot be parsed by regular expressions. Regex queries are not equipped to break down HTML into its meaningful parts. so many times but it is not getting to me. Even enhanced irregular regular expressions as used by Perl are not up to the task of parsing HTML. You will never make me crack. HTML is a language of sufficient complexity that it cannot be parsed by regular expressions. Even Jon Skeet cannot parse HTML using regular expressions. Every time you attempt to parse HTML with regular expressions, the unholy child weeps the blood of virgins, and Russian hackers pwn your webapp. Parsing HTML with regex summons tainted souls into the realm of the living. HTML and regex go together like love, marriage, and ritual infanticide. The cannot hold it is too late. The force of regex and HTML together in the same conceptual space will destroy your mind like so much watery putty. If you parse HTML with regex you are giving in to Them and their blasphemous ways which doom us all to inhuman toil for the One whose Name cannot be expressed in the Basic Multilingual Plane, he comes. HTML-plus-regexp will liquify the n​erves of the sentient whilst you observe, your psyche withering in the onslaught of horror. Rege̿̔̉x-based HTML parsers are the cancer that is killing StackOverflow it is too late it is too late we cannot be saved the trangession of a chi͡ld ensures regex will consume all living tissue (except for HTML which it cannot, as previously prophesied) dear lord help us how can anyone survive this scourge using regex to parse HTML has doomed humanity to an eternity of dread torture and security holes using regex as a tool to process HTML establishes a breach between this world and the dread realm of c͒ͪo͛ͫrrupt entities (like SGML entities, but more corrupt) a mere glimpse of the world of reg​ex parsers for HTML will ins​tantly transport a programmer's consciousness into a world of ceaseless screaming, he comes, the pestilent slithy regex-infection wil​l devour your HT​ML parser, application and existence for all time like Visual Basic only worse he comes he comes do not fi​ght he com̡e̶s, ̕h̵i​s un̨ho͞ly radiańcé destro҉ying all enli̍̈́̂̈́ghtenment, HTML tags lea͠ki̧n͘g fr̶ǫm ̡yo​͟ur eye͢s̸ ̛l̕ik͏e liq​uid pain, the song of re̸gular exp​ression parsing will exti​nguish the voices of mor​tal man from the sp​here I can see it can you see ̲͚̖͔̙î̩́t̲͎̩̱͔́̋̀ it is beautiful t​he final snuffing of the lie​s of Man ALL IS LOŚ͖̩͇̗̪̏̈́T ALL I​S LOST the pon̷y he comes he c̶̮omes he comes the ich​or permeates all MY FACE MY FACE ᵒh god no NO NOO̼O​O NΘ stop the an​*̶͑̾̾​̅ͫ͏̙̤g͇̫͛͆̾ͫ̑͆l͖͉̗̩̳̟̍ͫͥͨe̠̅s ͎a̧͈͖r̽̾̈́͒͑e n​ot rè̑ͧ̌aͨl̘̝̙̃ͤ͂̾̆ ZA̡͊͠͝LGΌ ISͮ̂҉̯͈͕̹̘̱ TO͇̹̺ͅƝ̴ȳ̳ TH̘Ë͖́̉ ͠P̯͍̭O̚​N̐Y̡ H̸̡̪̯ͨ͊̽̅̾̎Ȩ̬̩̾͛ͪ̈́̀́͘ ̶̧̨̱̹̭̯ͧ̾ͬC̷̙̲̝͖ͭ̏ͥͮ͟Oͮ͏̮̪̝͍M̲̖͊̒ͪͩͬ̚̚͜Ȇ̴̟̟͙̞ͩ͌͝S̨̥̫͎̭ͯ̿̔̀ͅ

Have you tried using an XML parser instead?

CommentFileSizeAuthor
#2 1276042_2.patch461 byteschx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented

We have a meticulously crafted, painstakingly ported piece of an HTML parser in filter_xss. Then, you do return preg_replace('|<([^> ]*)/>|i', '<$1 />', $body_content);. What were you thinking?

<html>
<body>
<img title="This is a very tricky title/>" src="misc/druplicon.png" />
</body>
</html>
chx’s picture

chx CreditAttribution: chx commented
Status: Active » Needs review
FileSize
1276042_2.patch461 bytes

Digging the issue queue finds this comment "The XHTML guidelines recommend to include a space before the trailing / and > of empty elements for better rendering on HTML user agents.. Recommend. OK. We disregard that happily. 'Cos, you know, you just can't do that. Not easily at least.

chx’s picture

chx CreditAttribution: chx commented
Issue tags: +Needs backport to D7
chx’s picture

chx CreditAttribution: chx commented

To clarify further this preg changes user facing data like in my example. While it's totally cool to mangle HTML as much as we like but changing the contents is not so cool.

chx’s picture

chx CreditAttribution: chx commented

And to further clarify the reason this is major because I want attention. Yes, adding a space there is a minor bug but the mindset itself is a bug.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Priority: Major » Normal
Status: Needs review » Postponed (maintainer needs more info)

Well, this regexp was not in my initial design, because the goal was precisely to remove the text parsing we do on "HTML". This said, this is a regexp on a *valid* XML document, so what do you think the problem is exactly?

<html>
<body>
<img title="This is a very tricky title/>" src="misc/druplicon.png" />
</body>
</html>

^ This is *not* a valid XML document.

chx’s picture

chx CreditAttribution: chx commented
Status: Postponed (maintainer needs more info) » Needs review

http://www.w3.org/TR/xml/#sec-attribute-types

. The string type may take any literal string as a value

the attribute value can be anything. Edit:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
        "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" lang="en" xml:lang="en">
<head>
<title></title>
</head>
<body><p>
<img title="This is a very tricky title/>" src="misc/druplicon.png" alt="** PLEASE DESCRIBE THIS IMAGE **" />
</p></body>
</html>

Passes http://validator.w3.org/check as XHTML 1.0 Strict.

pillarsdotnet’s picture

pillarsdotnet CreditAttribution: pillarsdotnet commented
Status: Needs review » Reviewed & tested by the community

Totally agreed, although the issue summary seems a bit ... repetitive.

chx’s picture

chx CreditAttribution: chx commented

@pillardotsnet, the issue summar looks like because even "some of the most skilled core developers" per http://drupal.org/node/1274838#comment-4974502 don't get it. So I try....

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Active
Issue tags: -Needs backport to D7

this one was committed to D8: http://drupalcode.org/project/drupal.git/commit/bb01b39
assigning to 7.x queue

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
Issue tags: +Needs backport to D7

As far as I can see, the only intention of this line was to prettify the HTML tags. But that's just a wild guess. As usual, there's no code comment explaining why it's done... did anyone care to look up why that line was added?

Because of that, I think the RTBC and commit here was a bit too fast.

Anyway, restoring tag and status.

pillarsdotnet’s picture

pillarsdotnet CreditAttribution: pillarsdotnet commented
Status: Needs review » Reviewed & tested by the community

As usual, there's no code comment explaining why it's done... did anyone care to look up why that line was added?

Yes, chx did, and reported in #2:

Digging the issue queue finds this comment "The XHTML guidelines recommend to include a space before the trailing / and > of empty elements for better rendering on HTML user agents..

I don't know whether you failed to read his comment or simply doubted his conclusion, but here is an exhaustive history of that bit of code:

  • scor added the comment and code in #374441-35: Refactor Drupal HTML corrector (PHP5)
    He said:

    Drupal strives to produce XHTML code, let's not break the XHTML compliant tests just for this patch to pass the tests. Instead, this patch adds the missing XHTML whitespace for empty elements in the HTML corrector filter and fix the non XHTML tests.

    Here is the relevant chunk of the interdiff between 374441-refactor-html-corrector_11.patch from #33 and 374441-refactor-html-corrector_12.patch from #35:

    diff -u modules/filter/filter.module modules/filter/filter.module
--- modules/filter/filter.module
+++ modules/filter/filter.module        6 Jul 2009 14:47:56 -0000
@@ -765,7 +765,10 @@
   // The "s" modifier makes "." match newlines too.
   $bodyNode = $htmlDom->getElementsByTagName('body')->item(0);
   if (preg_match("|^<body[^>]*>(.*)</body>$|s", $htmlDom->saveXML($bodyNode), $matches)) {
-    return $matches[1];
+    $body_content = $matches[1];
+    // The XHTML guidelines recommend to include a space before the trailing /
+    // and > of empty elements for better rendering on HTML user agents.
+    return preg_replace('|<([^>]*)/>|i', '<$1 />', $body_content);
   }
   else {
     return "";
  • scor updated tests in #37 to match the added code.
  • He further commented in #40:

    The whitespace added via preg_replace('|<([^>]*)/>|i', '<$1 />', $body_content) is to support the compatibility with the old HTML 4 browsers, see http://www.w3.org/TR/xhtml1/#C_2. The code would still be valid XHTML without it but I'm unsure how the older browsers would react. Since Drupal and the old HTML Corrector have been supporting it, we would need to make sure it does not break anywhere before removing it.

  • He added another test requiring this behavior in #41, commenting:

    The (old) HTML Corrector converts the valid XHTML to non valid . The new implementation in this patch fixes that. Adding a test for it.

  • tic2000 marked the issue RTBC in #45.
  • Dries committed the patch in #48. Here is the commitdiff.
  • A follow-up issue moved the comment and code in this patch attached to #542742: Filter: Create wrapper functions to load/serialize a DOM..
  • It was maintained in the patch revisions posted to comments #1, #2, #4, #5, #6, #7, and #9, without debate.
  • Damien Tournoud marked the issue RTBC in #10.
  • Dries committed it in #11. Here is the commit diff.
  • There were no follow-up issues nor patches.
pillarsdotnet’s picture

pillarsdotnet CreditAttribution: pillarsdotnet commented

As an aside, there are a lot of other places in the same file where preg is used on html:

1429
$text = preg_replace_callback('`<!--(.*?)-->`s', '_filter_url_escape_comments', $text);
1432
$chunks = preg_split('/(<.+?>)/is', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
1448
$chunks[$i] = preg_replace_callback($pattern, $task, $chunks[$i]);
1475
$text = preg_replace_callback('`<!--(.*?)-->`', '_filter_url_escape_comments', $text);
1601
$chunks = preg_split('@(<!--.*?-->|</?(?:pre|script|style|object|iframe|!--)[^>]*>)@i', $text, -1, PREG_SPLIT_DELIM_CAPTURE);
1617
list($tag) = preg_split('/[ >]/', substr($chunk, 2 - $open), 2);
1631 to 1647
$chunk = preg_replace('|\n*$|', '', $chunk) . "\n\n"; // just to make things a little easier, pad the end
$chunk = preg_replace('|<br />\s*<br />|', "\n\n", $chunk);
$chunk = preg_replace('!(<' . $block . '[^>]*>)!', "\n$1", $chunk); // Space things out a little
$chunk = preg_replace('!(</' . $block . '>)!', "$1\n\n", $chunk); // Space things out a little
$chunk = preg_replace("/\n\n+/", "\n\n", $chunk); // take care of duplicates
$chunk = preg_replace('/^\n|\n\s*\n$/', '', $chunk);
$chunk = '<p>' . preg_replace('/\n\s*\n\n?(.)/', "</p>\n<p>$1", $chunk) . "</p>\n"; // make paragraphs, including one at the end
$chunk = preg_replace("|<p>(<li.+?)</p>|", "$1", $chunk); // problem with nested lists
$chunk = preg_replace('|<p><blockquote([^>]*)>|i', "<blockquote$1><p>", $chunk);
$chunk = str_replace('</blockquote></p>', '</p></blockquote>', $chunk);
$chunk = preg_replace('|<p>\s*</p>\n?|', '', $chunk); // under certain strange conditions it could create a P of entirely whitespace
$chunk = preg_replace('!<p>\s*(</?' . $block . '[^>]*>)!', "$1", $chunk);
$chunk = preg_replace('!(</?' . $block . '[^>]*>)\s*</p>!', "$1", $chunk);
$chunk = preg_replace('|(?<!<br />)\s*\n|', "<br />\n", $chunk); // make line breaks
$chunk = preg_replace('!(</?' . $block . '[^>]*>)\s*<br />!', "$1", $chunk);
$chunk = preg_replace('!<br />(\s*</?(?:p|li|div|th|pre|td|ul|ol)>)!', '$1', $chunk);
$chunk = preg_replace('/&([^#])(?![A-Za-z0-9]{1,8};)/', '&amp;$1', $chunk);

I'm sure that each of them have their own history.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review

I'm not inclined to commit this to D7, unless we can prove that it actually fixes something (e.g. there's a test that passes with the patch and fails without). While I'm all about clean-ups (esp. in D8), the comments pillarsdotnet dug up seem to imply this was done very deliberately with an eye towards compatibility with older browsers which, unfortunately, D7 still has to deal with (IE6--).

Marking needs review for now to give sun/chx a chance to respond, but I'd be perfectly happy with D8 + fixed and leave it at that.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

FYI: @webchick is explicitly referring to http://www.w3.org/TR/xhtml1/#C_2 -- that's essentially the kind of input I was asking for in #11

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes

added a blockquote.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented
Status: Needs review » Closed (won't fix)

Let's close this then for D7. It's been 3 years with no feedback to push this along.

Please reopen this if, as @webchick says "we can prove that it actually fixes something".

chx’s picture

chx CreditAttribution: chx commented
Assigned: chx » Unassigned
Status: Closed (won't fix) » Active

So if I don't push an issue forward then it's going to rot based on out of context code examples -- if anyone would've checked the code examples in #13 then it's either filter_url commented to death or _filter_autop which stretches back before the dawn of time and is actually quite battle tested and solid. I even provided the example where the preg fails and yet it's questioned what it fixes. And people ask on twitter why my avatar is crying. I am so out of this issue.

  • Dries committed bb01b39 on 8.3.x 
    - Patch #1276042 by chx: preg is used on html.

  • Dries committed bb01b39 on 8.3.x 
    - Patch #1276042 by chx: preg is used on html.

  • Dries committed bb01b39 on 8.4.x 
    - Patch #1276042 by chx: preg is used on html.

  • Dries committed bb01b39 on 8.4.x 
    - Patch #1276042 by chx: preg is used on html.

  • Dries committed bb01b39 on 9.1.x 
    - Patch #1276042 by chx: preg is used on html.