Remove all references to "self-closing" void elements in core

Tagging. Sorta related: #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag

+1 for standardizing on <br> instead of <br />.

I assume this applies to other self-closing tags as well. (<img>, <input>, <hr>, <link> <meta>, etc.)

Luckily, D7 only used it on test.

Personally, I suggest <br />

- <br /> already in CORE (may not a very good point ^_^)
- <br /> is a standard in XHTML, I think most themer preferred it. (Myself never tried to type <br> in pass few years)
- adaption of XHTML X.0 version ?

includes/mail.inc
353: * <a> <em> <i> <strong> <b> <br> <p> <blockquote> <ul> <ol> <li> <dl> <dt>

modules/aggregator/aggregator.admin.inc
405:    '#default_value' => variable_get('aggregator_allowed_html_tags', '<a> <b> <br> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>'),

modules/aggregator/aggregator.module
720:  return filter_xss($value, preg_split('/\s+|<|>/', variable_get('aggregator_allowed_html_tags', '<a> <b> <br> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>'), -1, PREG_SPLIT_NO_EMPTY));

modules/field/modules/text/text.module
409:  $line_breaks = array('<br />' => 6, '<br>' => 4);

modules/filter/filter.module
1589: * Convert line breaks into <p> and <br> in an intelligent fashion.

modules/filter/filter.test
1349: It is important www.example19.com to *<br/>test different URLs and http://www.example20.com in the same paragraph. *<br>
1544:    $f = _filter_htmlcorrector('<hr><br>');
1577:    $f = _filter_htmlcorrector('<br></br>');
1592:    $f = _filter_htmlcorrector('<p>Line1<br><STRONG>bold stuff</b>');

modules/simpletest/tests/mail.test
197:      '<br>Drupal<br>Drupal' => "Drupal\nDrupal\n",

sites/all/modules/devel/krumo/docs/errors.html
<a href="#Post-parsing">Post-parsing</a><br>

sites/all/modules/wysiwyg/CHANGELOG.txt
65:#973808 by David_Rothstein: Fixed CKEditor incorrectly formatting the <br> tag.

sites/all/modules/wysiwyg/editors/js/ckeditor-3.0.js
62:        // CKEditor adds default formatting to <br>, so we want to remove that

http://drupal.org/node/1089770

Guiding Principles
Below are the principles we will follow when developing HTML5 functionality for Drupal.

8. Mimic XHTML. Be HTML.

<br /> fits both criteria and gets my vote.

Yea, I too always prefer the self closing tag <br />.

This is one of those case like the required tag, it can be required or required="required" but we've decided to go with required="required"

I disagree on switching <br/> to <br> for personal preferences but I also think that themers would prefer self-omitting tags. keep the <br/>

Doesn't sound like this patch would be terrible hard. Just a find and replace. Tagging for Novice and putting this on my plate for tonight.

I just did a search on D8 and I couldn't find a single problem with the use of proper line breaks. I found a handful of issues wher "
" was used in the description of allowed html tags or in tests. But I didn't find an issue where the
was actually being outputted as markup.

I recommend we close (works as designed) this.

I'm not sure what you mean by problems but it is referenced in several places in core.

  \drupal\core\modules\field\modules\text\text.module
	Line 409:   $line_breaks = array('<br />' => 6, '<br>' => 4);
  \drupal\core\modules\filter\filter.module
	Line 1596:  * Convert line breaks into <p> and <br> in an intelligent fashion.
  \drupal\core\modules\aggregator\aggregator.admin.inc
	Line 447:     '#default_value' => variable_get('aggregator_allowed_html_tags', '<a> <b> <br> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>'),
  \drupal\core\modules\aggregator\aggregator.module
	Line 740:   return filter_xss($value, preg_split('/\s+|<|>/', variable_get('aggregator_allowed_html_tags', '<a> <b> <br> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>'), -1, PREG_SPLIT_NO_EMPTY));
  \drupal\core\includes\mail.inc
	Line 383:  * <a> <em> <i> <strong> <b> <br> <p> <blockquote> <ul> <ol> <li> <dl> <dt>

@NROTC_Webmaster I think I found those lines as well, look at those lines in context and I think you'll see that these lines don't refer to code that outputs markup for any of our components. It's all either (an appropriate) comment, help text to the user, or used in valid tests.

That's what I'm trying to say. This issue isn't productive.

I don't have a personal preference I just wanted to make sure everyone was aware that it was still reference in core.

226 usages in core/modules alone. Granted not all of it may be valid (ie: XML in aggregator.module), but there is definitely a ton still left in core.

system.install (ln 316):

    $description .= '<br />' . t('To run cron from outside the site, go to <a href="!cron">!cron</a>', array('!cron' => url('cron/' . \Drupal::state()->get('system.cron_key'), array('absolute' => TRUE))));

Also, we really shouldn't be promoting older XHTML styles moving forward (regardless if they're not being used as "output").

Working on it.

I change one instance of a br tag. Is this the way that should be done?

Yes

Thanks Mark. I'll continue working on this.

I'm updating the br tags and I got some questions on the process.

1. Should we update the test files too? Files like core/modules/filter/src/Tests/FilterUnitTest.php test for the presence of the closing slash '/'.
2. Filter module checks for '<br />' in the _filter_autop() function. They should not be updated, right?

1. Ultimately, yes. However, this will require modifying several possible theme hooks (templates/functions) and what they output. This in reality should probably be a separate issue.

2. No. Do not take out <br /> from the filter functions of filter.module. However, if there is output that is for the actual page itself (provided by core, not user input).. then yes.

FYI, setting to "Needs review" will trigger the test bot.

Thanks Mark. I'll continue on this.

I'll leave test assertions for another issue then. I won't update filter module's either, unless it is output for the page.

I have updated most tags on the core/modules and core/themes directories.

Some more questions:

1. In core/modules/filter/src/Tests/FilterUnitTest.php there are assertions that test the addition of the closing slash. They are under the 'HTML corrector -- XHTML closing slash.' group. Is the filter module adding those closing slash back? Anything to be done about this?

2. In core/modules/system/templates/table.html.twig there is a colgroup tag without child elements. Is it correct?

I think what you'd want for filters is that they don't forcibly remove the closing slash, but they shouldn't forcibly add it in either.

Colgroup can have no child elements. But it's not a void element, so you'd want to still have an end tag.

Thanks Mark.

This is a new patch with the end tag for colgroup. I think the filter functionality might be addressed in another issue. Please let me know otherwise.

I am at the Austin Sprint. I am working on this issue and have found some additional self closing void tags. Creating patch now.

Ran this regex in the /core/modules folder to find the stuff fixed in the patch:

<(area|img|base|br|col|embed|hr|input|keygen|link|meta|param|source|track|wbr)(.*)/>

I did not apply this regex to the "filters" module. If you want to, run the regex above on that folder in your IDE. :)

32: remove-self-closing-1388926-32.patch queued for re-testing.

Testing new patch. Sorry for the failed attempts. :S

Trying again.

Marking this to Needs Review to get our test bots attention on this one ;)

Thanks man. Applied the patch on my local and reviewed a lot of it. Seems good to go.

Removed some self-closing elements.

+++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.php
@@ -541,7 +541,7 @@ public function testQuestionSign() {
-    $value = Xss::filterAdmin('<style /><iframe /><frame /><frameset /><meta /><link /><embed /><applet /><param /><layer />');
+    $value = Xss::filterAdmin('<style /><iframe /><frame /><frameset /><meta><link><embed><applet /><param><layer />');

Does this count as don't remove from filters? Mentioned in the issue summary.

Yup, @joelpittet is right — some further issues:

1. +++ b/core/modules/filter/filter.module
   @@ -1121,7 +1121,7 @@ function _filter_autop($text) {
   -      $chunk = preg_replace('|<br />\s*<br />|', "\n\n", $chunk);
   +      $chunk = preg_replace('|<br>\s*<br>|', "\n\n", $chunk);
   

   If any changes to the paragraph filter are necessary, then those should be performed in a separate/dedicated issue.

2. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
   @@ -307,10 +307,10 @@ function testHtmlFilter() {
        // Some tags make CSRF attacks easier, let the user take the risk herself.
   -    $f = _filter_html('<img />', $filter);
   +    $f = _filter_html('<img>', $filter);
   

   Ditto for the Allowed HTML tags filter.

3. +++ b/core/modules/filter/src/Tests/FilterUnitTest.php
   @@ -793,7 +793,7 @@ function testHtmlCorrectorFilter() {
        // XHTML slash for empty elements.
        $f = Html::normalize('<hr><br>');
   -    $this->assertEqual($f, '<hr /><br />', 'HTML corrector -- XHTML closing slash.');
   +    $this->assertEqual($f, '<hr><br>', 'HTML corrector -- XHTML closing slash.');
   

   Ditto for the HTML corrector filter.

4. +++ b/core/modules/migrate_drupal/src/Tests/Dump/Drupal6AggregatorSettings.php
   @@ -35,7 +35,7 @@ public function load() {
   -      'value' => 's:70:"<a> <b> <br /> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>";',
   +      'value' => 's:70:"<a> <b> <br> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>";',
   
   +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateAggregatorConfigsTest.php
   @@ -59,7 +59,7 @@ public function testAggregatorSettings() {
   -    $this->assertIdentical($config->get('items.allowed_html'), '<a> <b> <br /> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>');
   +    $this->assertIdentical($config->get('items.allowed_html'), '<a> <b> <br> <dd> <dl> <dt> <em> <i> <li> <ol> <p> <strong> <u> <ul>');
   

   The D6 source configuration may contain self-closing tags, so migration tests should not be changed.

   That said, perhaps we want to process such config in a way to automatically adjust self-closing tags during the migration, but that should be discussed in a separate/dedicated issue.

5. +++ b/core/modules/quickedit/js/theme.js
   @@ -18,7 +18,7 @@
   -    html += '<div id="' + settings.id + '" />';
   +    html += '<div id="' + settings.id + '">';
   
   @@ -37,9 +37,9 @@
   -    html += '<div class="quickedit-toolbar-label" />';
   +    html += '<div class="quickedit-toolbar-label">';
   ...
   -    html += '<div class="quickedit-toolbar quickedit-toolbar-field clearfix" />';
   +    html += '<div class="quickedit-toolbar quickedit-toolbar-field clearfix">';
   
   @@ -65,7 +65,7 @@
   -    return '<div id="quickedit-toolbar-fence" />';
   +    return '<div id="quickedit-toolbar-fence">';
   
   @@ -78,7 +78,7 @@
   -    return '<div id="' + settings.id + '" />';
   +    return '<div id="' + settings.id + '">';
   

   A DIV is not really self-closing.

   At least I don't think we want to intentionally produce incomplete HTML in core.

6. +++ b/core/modules/rdf/rdf.module
   @@ -570,7 +570,7 @@ function template_preprocess_rdf_metadata(&$variables) {
   -    // The XHTML+RDFa doctype allows either <span></span> or <span /> syntax to
   +    // The XHTML+RDFa doctype allows either <span></span> or <span> syntax to
        // be used, but for maximum browser compatibility, W3C recommends the
        // former when serving pages using the text/html media type, see
        // http://www.w3.org/TR/xhtml1/#C_3.
   

   This (outdated?) comment should probably not be changed, so the original intention of the code can be properly revisited later on.

7. +++ b/core/modules/system/src/Tests/Mail/HtmlToTextTest.php
   @@ -88,10 +88,10 @@ public function testTags() {
   -      '<br />Drupal<br />Drupal<br /><br />Drupal' => "Drupal\nDrupal\nDrupal\n",
   -      '<br/>Drupal<br/>Drupal<br/><br/>Drupal' => "Drupal\nDrupal\nDrupal\n",
   
   @@ -108,9 +108,9 @@ public function testTags() {
   -      '<hr />Drupal<hr />' => "------------------------------------------------------------------------------\nDrupal\n------------------------------------------------------------------------------\n",
   -      '<hr/>Drupal<hr/>' => "------------------------------------------------------------------------------\nDrupal\n------------------------------------------------------------------------------\n",
   

   These test cases are explicitly asserting that self-closing tags in the input HTML are properly converted into plain-text, so they should not be changed.

8. +++ b/core/modules/system/templates/table.html.twig
   @@ -45,11 +45,11 @@
   -      <colgroup{{ colgroup.attributes }} />
   +      <colgroup{{ colgroup.attributes }}>
   

   As above, I don't think we want to intentionally produce incomplete/bogus HTML here. A COLGROUP is not really self-closing; the UA rendering engine needs to correct the DOM first.

9. +++ b/core/modules/text/src/Tests/TextSummaryTest.php
   @@ -82,14 +82,14 @@ function testLength() {
        // This string tests a number of edge cases.
   -    $text = "<p>\nHi\n</p>\n<p>\nfolks\n<br />\n!\n</p>";
   +    $text = "<p>\nHi\n</p>\n<p>\nfolks\n<br>\n!\n</p>";
   

   For this text summary test, the possible appearance of a self-closing tag attempts to cover an edge-case parsing/processing scenario, so the changes should be reverted.

10. +++ b/core/modules/text/text.module
    @@ -120,7 +120,7 @@ function text_summary($text, $format = NULL, $size = NULL) {
       // If no complete paragraph then treat line breaks as paragraphs.
    -  $line_breaks = array('<br />' => 6, '<br>' => 4);
    +  $line_breaks = array('<br>' => 6, '<br>' => 4);
    

    Functional change, to be reverted.

11. +++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.php
    @@ -541,7 +541,7 @@ public function testQuestionSign() {
       public function testFilterXSSAdmin() {
    -    $value = Xss::filterAdmin('<style /><iframe /><frame /><frameset /><meta /><link /><embed /><applet /><param /><layer />');
    +    $value = Xss::filterAdmin('<style /><iframe /><frame /><frameset /><meta><link><embed><applet /><param><layer />');
         $this->assertEquals($value, '', 'Admin HTML filter -- should never allow some tags.');
       }
    

    This unit test method should probably be duplicated into a second, so that both possible syntax cases are covered.

    Ideally in a separate issue, so as to not include any non-minor changes in this big conversion patch here.

We can let someone else grab this one, unassigning grab it again if you have the time to review #48

Working on reroll from 44...

Here I rerolled patch taking into consideration comments on #48

Working on remove modification on test files to pass test...

Here again the patch removin modifications from tests files.

Sorry, I forgot the «FilterHtmlImageSecureTest» test modifications, here the patch again.

I can still find some "self-closing" elements, for example:

core/modules/editor/src/Tests/QuickEditIntegrationLoadingTest.php -> <img src="druplicon.png" />
core/includes/form.inc -> $batch_set['init_message'] .= '<br/>&nbsp;';
core/modules/system/src/Tests/Mail/HtmlToTextTest.php -> '<br />Drupal<br />Drupal<br /><br />Drupal' => "Drupal\nDrupal\nDrupal\n",

You can use the following regexp in the command line to find a lot more:

rgrep --color '<img.*/>' core

Back to "needs work" sorry :(

Here a new patch including the rest of self-closing elements found out of core/vendor or tests folders.

+++ b/core/modules/text/text.module
@@ -120,7 +120,7 @@ function text_summary($text, $format = NULL, $size = NULL) {
   // If no complete paragraph then treat line breaks as paragraphs.
-  $line_breaks = array('<br />' => 6, '<br>' => 4);
+  $line_breaks = array('<br>' => 6, '<br>' => 4);
   // Newline only indicates a line break if line break converter
   // filter is present.
   if (isset($format) && $filters->has('filter_autop') && $filters->get('filter_autop')->status) {

At a glance this kinda looks like it's changing the filter functionality.

Patch no longer applies, tagging for reroll.

Reroll.

FunctionsTest.php back to green ( after trivial fixup )

Reroll.

I'll review this patch right now.

+++ b/core/modules/text/text.module
@@ -120,7 +120,7 @@ function text_summary($text, $format = NULL, $size = NULL) {
   $break_points[] = array('</p>' => 0);
 
   // If no complete paragraph then treat line breaks as paragraphs.
-  $line_breaks = array('<br />' => 6, '<br>' => 4);
+  $line_breaks = array('<br>' => 6, '<br>' => 4);
   // Newline only indicates a line break if line break converter
   // filter is present.
   if (isset($format) && $filters->has('filter_autop') && $filters->get('filter_autop')->status) {

I agree with @Cotser in #60. It looks like this changes the filter functionality. At least it breaks the resulting $line_breaks array.
$line_breaks = array('
' => 6, '
' => 4);
results in an array containing two elements.
$line_breaks = array('
' => 6, '
' => 4);
results in an array containing one element.

Besides that, the patch looks good to me.

filter_autop currently insert <br /> in texts, so yes, this would alter this filter asaik, we should change after this patch filter_autop to return <br> as well.
#2350049: filter_autop() returns <br /> but should return <br> for HTML5

Rerolled 65

Addressing the comments from #48 that were missed and #60.

Undid the JS changes mentioned in #48 and the table colgroup tag.

Reviewing to confirm changes following the feedback in #48, I found the following:

1. The patch adds some changes in core/modules/filter/src/Tests/FilterUnitTest.php, but I don't think most of those changes should be here (if the filter.module changes are a separate issue)?

2. +++ b/core/modules/filter/src/Plugin/Filter/FilterAutoP.php
   @@ -33,7 +33,7 @@ public function process($text, $langcode) {
   -      return $this->t('Lines and paragraphs are automatically recognized. The <br /> line break, <p> paragraph and </p> close paragraph tags are inserted automatically. If paragraphs are not recognized simply add a couple of blank lines.');
   +      return $this->t('Lines and paragraphs are automatically recognized. The <br> line break, <p> paragraph and </p> close paragraph tags are inserted automatically. If paragraphs are not recognized simply add a couple of blank lines.');
   

   should this change to FilterAutoP be in #2350049: filter_autop() returns <br /> but should return <br> for HTML5 instead?

3. +++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
   @@ -103,7 +103,7 @@ public function tips($long = FALSE) {
   -      'br' => array($this->t('By default line break tags are automatically added, so use this tag to add additional ones. Use of this tag is different because it is not used with an open/close pair like all the others. Use the extra " /" inside the tag to maintain XHTML 1.0 compatibility'), $this->t('Text with <br />line break')),
   +      'br' => array($this->t('By default line break tags are automatically added, so use this tag to add additional ones. Use of this tag is different because it is not used with an open/close pair like all the others. Use the extra " /" inside the tag to maintain XHTML 1.0 compatibility'), $this->t('Text with <br>line break')),
   
   +++ b/core/modules/simpletest/src/WebTestBase.php
   @@ -1787,9 +1787,9 @@ protected function drupalPostAjaxForm($path, $edit, $triggering_element, $ajax_p
   +    $verbose .= '<hr>' . $this->content;
   

   Wondering if this change should also be moved?

And some new questions:

1. +++ b/core/modules/editor/src/Tests/EditorFileReferenceFilterTest.php
   @@ -69,44 +69,44 @@ function testEditorFileReferenceFilter() {
   -    $input = '<video src="llama.jpg" data-editor-file-uuid="' . $uuid . '" />';
   +    $input = '<video src="llama.jpg" data-editor-file-uuid="' . $uuid . '">';
   

   The <video> tag is not in the official list of void elements.

2. +++ b/core/modules/system/src/Tests/Mail/HtmlToTextTest.php
   @@ -219,7 +219,7 @@ public function testDrupalHtmltoTextCollapsesWhitespace() {
   -      . '<br />[br]'
   +      . '<br>[br]'
   
   @@ -230,7 +230,7 @@ public function testDrupalHtmlToTextBlockTagToNewline() {
   -      . '<hr />[hr]'
   +      . '<hr>[hr]'
   

   Should these tests not remove the line, but just add a new one so it checks for both styles (until fitlers are updated)?

3. +++ b/core/modules/system/src/Tests/Mail/HtmlToTextTest.php
   @@ -257,7 +257,7 @@ public function testDrupalHtmlToTextBlockTagToNewline() {
   -        . '<br />should  be equal to <br />'
   +        . '<br>should  be equal to <br />'
   

   Same as above, though even if this change is kept I believe there's an error in the added line?

This last group of items was found using the regex provided in #23

1. Found $batch_set['init_message'] .= '<br/>&nbsp;'; in core/includes/form.inc
2. Found '#prefix' => "<br style='clear:both'/>", in FileTransferAuthorizeForm
3. Found $sample_picture = '<img src="' . file_create_url('core/misc/icons/bebebe/pencil.svg') . '" alt="' . t('contextual links button') . '" />'; in contextual.module
4. Found a few <br/>s in allowedValuesDescription() from ListIntegerItem
5. Found in many tests: AddFeedTest, CascadingStylesheetsTest, RenderElementTypesTest, AdminMetaTagTest, DefaultMobileMetaTagsTest, TextSummaryTest
6. Found in Classy file-upload-help.html.twig
7. Found in a code comment in system.install

In my opinion (newbie) this ticket should be reviewed in a much later stage, codebase is being updated every minute and new "br"s are being/will be introduced.

Is there a way to tag or flag this issue to be reviewed in a later stage?

I am removing the Novice tag from this issue until we can get an issue summary update. Right now this issue is a little long and convoluted and could really use an update on what the remaining tasks (or roadblocks) are, and then which remaining tasks are novice if not all of them.

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

@woombo the sooner this gets wrapped up and added into core, the sooner it becomes the standard that everyone else needs to match. :)

Personally, I would like to see this change make it into the core, so I will try to revive this issue.

Uploading an updated patch. In 2 years Drupal has changed so much that this is more like a new patch than a reroll, there is no point of creating an interdiff.

As already discussed at length in this thread, in this patch I deliberately ignore the filter and editor modules, migrations, fixtures, mail-to-text transformations and security checks in fear of introducing regressions — all of this can be done in the follow-up issues. This patch is focused only on static HTML markup, documentation, JS and tags generation.

I largely agree with @sun comments in #48. One thing I disagree with though is the <colspan> tag. It is, in fact, possible to omit its closing tag, so I have included this change as well.

Updating status for initial patch testing.

Reverting change in StandardTest that tests the output of filter module.

One of the patched files was deleted, so it does not apply cleanly. Uploading updated patch.

I cannot find any other self-closing tags in core, other than in filter and editor modules, migrations, fixtures, mail-to-text transformations and security checks. This patch is ready for manual review.

Updating patch to include recent additions to the core.

+++ b/core/modules/rest/src/Tests/RESTTestBase.php
@@ -229,11 +229,11 @@ protected function httpRequest($url, $method, $body = NULL, $mime_type = NULL, $
+      (isset($options['headers']) ? '<hr>Request headers: ' . nl2br(print_r($options['headers'], TRUE)) : '') .

Despite stripping the closing slashes, the PHP function nl2br still outputs the closing slashes by default - http://php.net/manual/en/function.nl2br.php

Setting the second parameter for nl2br to false will close the loop on comprehensive void elements without the closing slash.

If #2441811: Upgrade filter system to HTML5 gets it in, it is probably time to revive this and make the rest of core output HTML5 as well.

Added another related issue #3346188: Remove trailing slashes on void elements.

Briefly discussed this with @longwave and @laurii during Drupal Dev Days 2023 and decided to mark #3346188: Remove trailing slashes on void elements as duplicate. I'm copying the work that I did there over to this issue.

It's probably still worth waiting for #2441811: Upgrade filter system to HTML5 to get in first.

I have attached patch for 11.x, please review

Fixed the one remaining test failure.

#105 onwards has lost all the work done in #86 and before. There are still plenty of instances of <br /> and other void tags in core, perhaps we need to break this out into multiple issues.

Hope this issue will help to progress with related #1268180: Newline character in HtmlTag causes unwanted whitespace

#108 is already a very large patch to review. So think expanding on it would be even more difficult. So think breaking off into smaller tickets make sense. Maybe by tag?

Moving to NW for the follow ups to be created and issue summary to be updated with the new plan (if agreed upon). #108 is a good start so maybe after the follow ups we still merge this?

@smustgrave

#108 is already a very large patch to review. So think expanding on it would be even more difficult. So think breaking off into smaller tickets make sense. Maybe by tag?

Why not open a MR? it is easy to review without open new issues.

Breaking this into smaller issues is not about whether it is a patch or an MR. It is about the scope and patch size. There is a limit to what a reviewer can effectively review and once past that limit, mistakes are made. And we can help avoid those mistakes by splitting this into smaller pieces. There is more information about scope and patch size in the wiki.

I was looking at the patch in #86 and I do agree with smustgrave that doing this by tag is worth pursuing. However, this must be balanced if there are only a few instances of some tags that need changing. It could be sensible to combine some tags together while still keeping the change size small.

I think that thanks to #2441811: Upgrade filter system to HTML5, this now became actually feasible to do!

In that context would great to tackle #1268180: Newline character in HtmlTag causes unwanted whitespace