Follow-up from #552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag.

Motivation

HTML5 does not actually implement the concept of "self-closing tags". It is a relic from XHTML. From http://www.w3.org/TR/html5/syntax.html#syntax-start-tag:

Then, if the element is one of the void elements, or if the element is a foreign element, then there may be a single "/" (U+002F) character. This character has no effect on void elements, but on foreign elements it marks the start tag as self-closing.

It has "no effect on void elements", meaning it is ignored when being parsed.

Proposed resolution

Remove all unnecessary / characters from void elements in core-provided templates, documentation, sources and output:
area, base, br, col, embed, hr, img, input, keygen, link, meta, param, source, track, wbr

This DOES NOT (and should not) include user generated content. For example, if a content editor inserts a <br /> tag into a body of a node, it should be preserved by filter as is.

Remaining tasks

  • Document self-closing tags usage in coding standards both for HTML and JS.
    Documentation should clearly describe the difference between void tags and foreign elements, as well as the usage of <div /> syntax in JS.
  • Can templates in stable theme be modified in minor release?
  • Remove self-closing void tags from output of filter module without breaking its current behavior.
    Filters should not forcibly remove the closing slash if it is added by user, but they should not forcibly add it in either.
  • Reconfigure editor to not generate these tags in content and update tests in editor module.
  • Test the behavior of Drupal\Core\Mail\MailFormatHelper::htmlToText to make sure that it can transform both cases.
  • Update XSS filtering tests to verify that they cover both normal and self-closing void tags (i.e. core/tests/Drupal/Tests/Component/Utility/XssTest.php).
  • Decide if it is necessary to remove self-closing tags from test fixtures.

A quote from @sun's comment #48 on migrations:

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.

User interface changes

None

API changes

None

Template changes

Changes in stable, classy, bartik theme templates and default module templates.

Original report by @tim.plunkett

There are 11 uses of <br> in core, and 87 of <br />. I've never once used the latter, and both are valid HTML: http://dev.w3.org/html5/spec-author-view/syntax.html#syntax-start-tag

I propose core switch everything to <br>.

Either way, we should pick one for core, and clarify in the coding standards that one is preferred but both are allowed.

Files: 

Comments

Jacine’s picture

dcmouyard’s picture

Issue tags: -html5

+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.)

dcmouyard’s picture

droplet’s picture

Issue tags: +html5

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
tim.plunkett’s picture

The current usages are irrelevant. All of it could be trivially switched with some regex.
XHTML is dead, long live HTML.

bonus’s picture

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.

linclark’s picture

Since both <br> and <br /> are equally valid in HTML, I'd say it's best to mimic XHTML and ruffle fewer feathers. Let's go with <br />.

Also, @tim.plunkett, if you haven't once used the latter, it's because you aren't old like us oldies... IIRC, it was what Zeldman recommended in Designing with Web Standards back in the day (2003) and usage was how you knew if someone was a cool kid or not :-P

eric.duran7@gmail.com’s picture

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"

adnasa’s picture

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

cosmicdreams’s picture

Issue tags: +Novice

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.

cosmicdreams’s picture

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.

NROTC_Webmaster’s picture

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>
tim.plunkett’s picture

But in none of those places is it actually used, and it probably shouldn't be replaced.

cosmicdreams’s picture

@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.

NROTC_Webmaster’s picture

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

linclark’s picture

Status: Active » Closed (cannot reproduce)

So it sounds like this is no longer an issue in D8. Closing, feel free to reopen if it is still an issue.

markcarver’s picture

Title: Standardize usage of self-closing <br> tags » Remove all references to "self-closing" tags in core
Category: Feature request » Task
Issue summary: View changes
Status: Closed (cannot reproduce) » Active
Issue tags: +theme system cleanup, +Twig
Related issues: +#552478: Restrict "self-closing" tags to only void elements in drupal_pre_render_html_tag

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 .= '
' . t('To run cron from outside the site, go to !cron', 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").

markcarver’s picture

Title: Remove all references to "self-closing" tags in core » Remove all references to "self-closing" void elements in core
Issue summary: View changes
dinarcon’s picture

Assigned: Unassigned » dinarcon

Working on it.

dinarcon’s picture

FileSize
964 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

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

markcarver’s picture

Yes

dinarcon’s picture

Thanks Mark. I'll continue working on this.

dinarcon’s picture

FileSize
18.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,750 pass(es). View

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?

markcarver’s picture

Status: Active » Needs review

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.

The last submitted patch, 20: remove-self-closing-1388926-20.patch, failed testing.

dinarcon’s picture

Status: Needs review » Needs work

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.

dinarcon’s picture

FileSize
38.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,734 pass(es). View

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?

mdrummond’s picture

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.

dinarcon’s picture

FileSize
417 bytes
38.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,730 pass(es). View

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.

martin107’s picture

Status: Needs work » Needs review
outlierdavid’s picture

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

outlierdavid’s picture

FileSize
70.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove-self-closing-1388926-32.patch. Unable to apply patch. See the log in the details link for more information. View

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. :)

Status: Needs review » Needs work

The last submitted patch, 32: remove-self-closing-1388926-32.patch, failed testing.

outlierdavid’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: remove-self-closing-1388926-32.patch, failed testing.

outlierdavid’s picture

FileSize
70.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove-self-closing-1388926-34.patch. Unable to apply patch. See the log in the details link for more information. View

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

dinarcon’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 36: remove-self-closing-1388926-34.patch, failed testing.

outlierdavid’s picture

FileSize
97.22 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,721 pass(es), 64 fail(s), and 2 exception(s). View

Trying again.

lauriii’s picture

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

lauriii’s picture

Status: Needs work » Needs review
outlierdavid’s picture

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

Status: Needs review » Needs work

The last submitted patch, 39: remove-self-closing-1388926-39.patch, failed testing.

lauriii’s picture

FileSize
112.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,064 pass(es), 52 fail(s), and 2 exception(s). View

Removed some self-closing elements.

lauriii’s picture

Status: Needs work » Needs review
joelpittet’s picture

+++ 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.

Status: Needs review » Needs work

The last submitted patch, 44: remove-self-closing-1388926-44.patch, failed testing.

sun’s picture

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.

joelpittet’s picture

Assigned: dinarcon » Unassigned

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

pakmanlh’s picture

Assigned: Unassigned » pakmanlh

Working on reroll from 44...

pakmanlh’s picture

Assigned: pakmanlh » Unassigned
Status: Needs work » Needs review
Issue tags: +FUDK
FileSize
88.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,473 pass(es), 63 fail(s), and 0 exception(s). View

Here I rerolled patch taking into consideration comments on #48

Status: Needs review » Needs work

The last submitted patch, 51: remove-self-closing-1388926-51.patch, failed testing.

pakmanlh’s picture

Assigned: Unassigned » pakmanlh

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

pakmanlh’s picture

Assigned: pakmanlh » Unassigned
Status: Needs work » Needs review
FileSize
20.58 KB
72.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,562 pass(es), 13 fail(s), and 0 exception(s). View

Here again the patch removin modifications from tests files.

Status: Needs review » Needs work

The last submitted patch, 54: remove-self-closing-1388926-54.patch, failed testing.

pakmanlh’s picture

FileSize
71.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,609 pass(es). View
20.58 KB

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

er.pushpinderrana’s picture

Status: Needs work » Needs review
rteijeiro’s picture

Status: Needs review » Needs work

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 :(

pakmanlh’s picture

Status: Needs work » Needs review
FileSize
3.34 KB
75.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,609 pass(es). View

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

Cottser’s picture

+++ 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.

LinL’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies, tagging for reroll.

martin107’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
73.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,746 pass(es), 1 fail(s), and 0 exception(s). View

Reroll.

Status: Needs review » Needs work

The last submitted patch, 62: remove-self-closing-1388926-62.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
74.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,747 pass(es). View
1.04 KB

FunctionsTest.php back to green ( after trivial fixup )

Jolidog’s picture

FileSize
74.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,857 pass(es). View

Reroll.

legolasbo’s picture

I'll review this patch right now.

legolasbo’s picture

+++ 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.

legolasbo’s picture

Issue tags: +Amsterdam2014
Hanno’s picture

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 self closing br element with slash, lets alter to br

a-fro’s picture

FileSize
74.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,065 pass(es). View

Rerolled 65

joelpittet’s picture

FileSize
4.04 KB
73.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,041 pass(es). View

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

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

akalata’s picture

Issue summary: View changes
Status: Needs review » Needs work

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 &lt;br /&gt; line break, &lt;p&gt; paragraph and &lt;/p&gt; 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 &lt;br&gt; line break, &lt;p&gt; paragraph and &lt;/p&gt; 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 self closing br element with slash, lets alter to br 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
akalata’s picture

woombo’s picture

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?

Patrick Storey’s picture

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

akalata’s picture

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

20th’s picture

Version: 8.2.x-dev » 8.3.x-dev
FileSize
104.78 KB

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.

20th’s picture

Status: Needs work » Needs review

Updating status for initial patch testing.

Status: Needs review » Needs work

The last submitted patch, 79: 1388926-79.patch, failed testing.

20th’s picture

Status: Needs work » Needs review
FileSize
122.46 KB
815 bytes

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

20th’s picture

FileSize
103.89 KB

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

20th’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

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.

20th’s picture

20th’s picture

FileSize
104.59 KB
718 bytes

Updating patch to include recent additions to the core.

Status: Needs review » Needs work

The last submitted patch, 86: 1388926-86.patch, failed testing.

20th’s picture

Status: Needs work » Needs review

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.