Drupal\Component\Utility\Html::normalize() leaves messy </body></html> in certain situations

Confirming the problem, when selecting the "Field can contain HTML" as on or off, the resulting field always displays clipped HTML tags. Glad someone saw this, I thought it was my bad CSS on the theme layer. Here is a sample of an unclosed a and p tag that clued me in to the problem:

<div class="field-content"><p>This Week in NeighborMedia: a recap of the week's captivating Cambridge stories produced by <a href="http://www.cctvcambridge.org/...</div>

The ellipsis comes from the field setting in views.

I'll try the patch and report back.

This patch corrects the same problem I was having, however it doesn't correct the problem if the "Trim only on a word boundary" checkbox is checked in Views. Since I don't know what code is executed to trim the text, I can't tell precisely where the code is deciding where the "word boundary" is, but it clearly thinks it's somewhere inside a tag. The result is the trimmed tag is incorrectly displayed as text to the end user.

Here is the input and the incorrect output with a trim length of 500, only on word boundary:

input:

<div class="snap_preview"><br />
<p>Listen to an interview with Gustavo Vilchis on Worldview on Chicago Public Radio!</p>
</div>
<div class="snap_preview"><br />
<p>Listen to an interview with Gustavo Vilchis on Worldview on Chicago Public Radio!</p>
<ul>
    <li><a href="http://www.chicagopublicradio.org/Program_WV.aspx?episode=30344">http://www.chicagopublicradio.org/Program_WV.aspx?episode=30344</a></li>
</ul>
<a rel="nofollow"><img alt="" style="border: 0px;" src="http://feeds.wordpress.com/1.0/comments/teachingrebellion.wordpress.com/353/" /></a> <a rel="nofollow"><img alt="" style="border: 0px;" src="http://feeds.wordpress.com/1.0/delicious/teachingrebellion.wordpress.com/353/" /></a> <a rel="nofollow"><img alt="" style="border: 0px;" src="http://feeds.wordpress.com/1.0/stumble/teachingrebellion.wordpress.com/353/" /></a> <a rel="nofollow"><img alt="" style="border: 0px;" src="http://feeds.wordpress.com/1.0/digg/teachingrebellion.wordpress.com/353/" /></a> <a rel="nofollow"><img alt="" style="border: 0px;" src="http://feeds.wordpress.com/1.0/reddit/teachingrebellion.wordpress.com/353/" /></a> <img alt="" style="border: 0px;" src="http://stats.wordpress.com/b.gif?host=teachingrebellion.wordpress.com&amp;blog=5191692&amp;post=353&amp;subd=teachingrebellion&amp;ref=&amp;feed=1" /></div>

output:

<div class="snap_preview"><br/>
<p>Listen to an interview with Gustavo Vilchis on Worldview on Chicago Public Radio!</p>
<ul>
    <li><a href="http://www.chicagopublicradio.org/Program_WV.aspx?episode=30344" class="views-processed">http://www.chicagopublicradio.org/Program_WV.aspx?episode=30344</a></li>
</ul>
<a rel="nofollow" class="views-processed"><img style="border: 0px none ;" src="http://feeds.wordpress.com/1.0/comments/teachingrebellion.wordpress.com/353/" alt=""/></a> <a rel="nofollow" class="views-processed">img alt="" src="http://feeds.wordpress.com/1.0/</a></div>

Closed. The reason: expired.

Update issues summary because this problem still exists in Drupal 7 (might be present in Drupal 8 too)

Here's a Drupal 7 patch that works in my testing (based on Views code)

Drupal 8 theoretically has the same issue. In D8, the function is Drupal\Component\Utility\Html::normalize(), which uses pretty much the exact same code as _filter_htmlcorrector(). If I run this code via Devel or drush, it produces the same messy HTML as in Drupal 7:

use Drupal\Component\Utility\Html;
print Html::normalize('<p>Here <img alt="ao');

However, I can't seem to be able to reproduce on Drupal 8 using similar steps as I can on Drupal 7! I'm not sure what's different, but maybe we can backport whatever that is?

Aha! The difference is that the "Correct faulty and chopped off HTML" filter isn't enabled on the "Basic HTML" text format by default on Drupal 8. So, I can reproduce on D8 now too!

I've updated the issue summary to reflect this.

Here's an updated Drupal 7 patch which should fix the failed test. It doesn't add new test coverage for this specific bug - which should really be added - for now, it just fixes the existing tests.

Wondering if this is still an issue in D9,5?

Normalize() seems to be a different from when this ticket was opened.

I followed the instructions in the issue summary to verify, and yes, unfortunately this is still an issue in D9.5 and D10.1

Not really sold on this solution but the regex using for D7 didn't work on D9.5

But added some tests

Wonder if ckeditor5 has any bearing on this.

ckeditor5 won't make any difference here. There is #2441811: Upgrade filter system to HTML5 but I don't think that really affects this either.

Added test coverage looks good, I'm also not entirely sure about the string replace but don't really have a better idea either. However if we go that way we should add a comment.

Added comment

+++ b/core/lib/Drupal/Component/Utility/Html.php
@@ -252,7 +252,9 @@ public static function resetSeenIds() {
+    // Remove any leftover <body> or <html> tags.
+    return str_replace(['&lt;/body&gt;', '&lt;/html&gt;'], '', $serialize);

Super nit, but they're technically 'closing' tags.

What I don't understand is how the </body> and </html> get there - \Drupal\Component\Utility\Html::serialize works by finding the body element and then looping over its child elements, so the wrapping html and body elements shouldn't even be in the picture.

I think we need to do some more forensics here.

As far as I understand, the HTML-encoded closing tags come from the load() method, where they are parsed as part of the cut-off attribute, not closing tags, and they get "repaired" (i.e. HTML-encoded) to make them valid as part of the attribute.

What I don't understand is how the and get there - \Drupal\Component\Utility\Html::serialize works by finding the body element and then looping over its child elements, so the wrapping html and body elements shouldn't even be in the picture.

They are added because of the reasons mentioned by @mfb in #28.
https://3v4l.org/r6nNq#v8.1.18 is a 3v4l with the minimum of load & serialize. I think this could also be a bug in php core's dom handling, but I think we should just fix it like in #26.
Setting this back to rtbc.

#26 seems to remove any closing body and html elements, also intentional ones inside code blocks.

Should we instead try to fix this somehow in Html::load() that adds them? https://3v4l.org/vm89s

Yes, it actually seems quite reasonable to just leave out the potentially-misinterpreted </body></html> and rely on DOMDocument to deal with the HTML soup. Let's see if tests pass.

Well, the one failing test honestly seems like an improvement, so I'm fixing the test.

Seems odd the closing bracket was the failing test but agreed.

Clarified proposed resolution and remaining tasks. Btw, it looks like these closing tags should still be removed even with #2441811: Upgrade filter system to HTML5, which is not surprising since DOMDocument is still in use under the hood.

Known fail

We recently committed #2441811: Upgrade filter system to HTML5 which makes this one not apply.

Rerolled.

I think I agree with #33 and

Yes, it actually seems quite reasonable to just leave out the potentially-misinterpreted </body></html> and rely on DOMDocument to deal with the HTML soup. Let's see if tests pass.

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/RssResponseRelativeUrlFilterTest.php
@@ -49,8 +49,7 @@ public function providerTestOnResponse() {
   <item>
      <title>Drupal 8 turns one!</title>
      <link>https://www.drupal.org/blog/drupal-8-turns-one</link>
-     <description>&lt;a href="localhost/node/1"&gt;Hello&lt;/a&gt;
-    </description>
+     <description>&lt;a href="localhost/node/1"&gt;Hello&lt;/a&gt;</description>
   </item>
   </channel>
 </rss>

This change is not longer needed.

Discussed with @alexpott. It seems that most of the HTML boilerplate in Html::load() is no longer required and we can get away with just parsing <body> . $html.

• Doctype is not needed, if I reserialize the DOMDocument then the doctype is added.
• Encoding meta tag is not needed, UTF-8 is the default but we can also specify this to the parser.
• <html> tag is not needed, again if I reserialize the document then the tag is added around <body>.

Opened an MR with the new test and my suggested changes.

Changes look good to me (especially getting rid of the ancient Content-Type meta tag :)

Added the comment, reworded slightly.

Looks good! Big win getting #2441811: Upgrade filter system to HTML5 to land!

Committed f101ff7 and pushed to 11.x. Thanks!

This bugfix will need a little tweaking if folks want to backport to Drupal 7 or Drupal 10.1, but should be easy..