- 🇺🇸United States smustgrave
Wonder if ckeditor5 has any bearing on this.
- Status changed to Needs work
over 1 year ago 2:57pm 28 March 2023 - 🇬🇧United Kingdom catch
ckeditor5 won't make any difference here. There is 🐛 Upgrade filter system to HTML5 Fixed 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.
- Status changed to Needs review
over 1 year ago 3:27pm 7 April 2023 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
+++ 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(['</body>', '</html>'], '', $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.
- 🇺🇸United States mfb San Francisco
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.
- Status changed to RTBC
over 1 year ago 2:01pm 22 April 2023 - 🇧🇪Belgium borisson_ Mechelen, 🇧🇪
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. - last update
over 1 year ago 29,302 pass 22:50 21:37 Running- last update
over 1 year ago 29,343 pass - 🇫🇮Finland olli
#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 - last update
over 1 year ago 29,098 pass, 83 fail The last submitted patch, 26: 418620-26.patch, failed testing. View results →
- last update
about 1 year ago 30,360 pass - Status changed to Needs review
about 1 year ago 3:42am 2 October 2023 - last update
about 1 year ago 30,358 pass, 1 fail - 🇺🇸United States mfb San Francisco
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. The last submitted patch, 33: 418620-33.patch, failed testing. View results →
- last update
about 1 year ago 30,360 pass - 🇺🇸United States mfb San Francisco
Well, the one failing test honestly seems like an improvement, so I'm fixing the test.
- Status changed to RTBC
about 1 year ago 3:02pm 2 October 2023 - 🇺🇸United States smustgrave
Seems odd the closing bracket was the failing test but agreed.
- 🇺🇸United States mfb San Francisco
Clarified proposed resolution and remaining tasks. Btw, it looks like these closing tags should still be removed even with 🐛 Upgrade filter system to HTML5 Fixed , which is not surprising since DOMDocument is still in use under the hood.
- last update
about 1 year ago 30,367 pass, 2 fail The last submitted patch, 35: 418620-35.patch, failed testing. View results →
- last update
about 1 year ago 30,371 pass - Status changed to Needs work
about 1 year ago 9:16am 4 October 2023 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
We recently committed 🐛 Upgrade filter system to HTML5 Fixed which makes this one not apply.
- Status changed to Needs review
about 1 year ago 9:25am 4 October 2023 - last update
about 1 year ago 30,367 pass, 2 fail - Status changed to Needs work
about 1 year ago 9:28am 4 October 2023 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
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><a href="localhost/node/1">Hello</a> - </description> + <description><a href="localhost/node/1">Hello</a></description> </item> </channel> </rss>
This change is not longer needed.
- First commit to issue fork.
- last update
about 1 year ago 30,379 pass - @longwave opened merge request.
- Status changed to Needs review
about 1 year ago 9:40am 4 October 2023 - 🇬🇧United Kingdom longwave UK
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.
- 🇺🇸United States mfb San Francisco
Changes look good to me (especially getting rid of the ancient Content-Type meta tag :)
The last submitted patch, 41: 418620-40.patch, failed testing. View results →
- last update
about 1 year ago 30,379 pass - Status changed to RTBC
about 1 year ago 2:57pm 4 October 2023 - 🇺🇸United States smustgrave
Looks good! Big win getting 🐛 Upgrade filter system to HTML5 Fixed to land!
- Status changed to Fixed
about 1 year ago 4:23pm 4 October 2023 -
alexpott →
committed f101ff7f on 11.x
Issue #418620 by mfb, smustgrave, longwave, dsnopek, greenbeans,...
-
alexpott →
committed f101ff7f on 11.x
- 🇺🇸United States mfb San Francisco
This bugfix will need a little tweaking if folks want to backport to Drupal 7 or Drupal 10.1, but should be easy..
Automatically closed - issue fixed for 2 weeks with no activity.
- Status changed to Fixed
7 months ago 7:36pm 16 May 2024 - 🇺🇸United States mfb San Francisco
@dsnopek I would suggest backporting the actual accepted patch, assuming you find that it works on Drupal 7