Follow-up to #2542392: Document scripts_bottom template variable

Problem/Motivation

There two different approaches used (AFAWK):
1) <table>
2) TH tag

Example https://api.drupal.org/api/drupal/core!modules!system!templates!table.ht...

Proposed resolution

  • Find all instances of uppercase tag names used in comments and make them all lowercase. <TABLE> becomes <table>
  • Find any instances where a tag is directly referred to but not written literally as a tag and change it. For example, "The text is wrapped in a p tag" should read "The text is wrapped in a <p> tag."

All Drupal core files should be checked for changes.

Remaining tasks

  1. Discuss
  2. Clean-up comments using uppercase tag names in comments and docblocks.

User interface changes

None.

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because updating comments
Issue priority Not critical because we're only changing comments
Prioritized changes The main goal of this issue is to improve the consistency of how html tags are written in code comments throughout core.
Disruption Non-distruptive.

Comments

andypost created an issue. See original summary.

joelpittet’s picture

Issue tags: +Twig

When I grepped, it was actually easier to grep as a tag in the comment instead of uppercase.

Regex find \* .*< in *.html.twig files for reference. And most of the results returned were <tag> form. So I'm +1 for doc standards to use that way consistently.

davidhernandez’s picture

I agree with Joel. I prefer the literal tag form.

Cottser’s picture

Yup. The all caps besides being all caps feels like old school HTML with table layouts and stuff :(

Cottser’s picture

(Double post)

jhodgdon’s picture

It sounds like our de-facto standard (as far as "what is commonly in use") is to put the tags in as <tagname> So let's get a patch to make the rest more consistent?

joelpittet’s picture

Category: Plan » Task
Issue summary: View changes
Issue tags: +Novice

Let's just re-use this issue?

joelpittet’s picture

Issue summary: View changes
jhodgdon’s picture

Title: Use similar style to mention HTML tags in code comments » Use consistent style to mention HTML tags in code comments
davidhernandez’s picture

I assume this is only for the html tag. I'm also assuming it is only in the context of when "html" is referring to the tag and not just in general.

For example:

// An HTML tag should not contain any special characters.

This is not referring to the html tag. Looking at a quick search there seems to be lots of places where "HTML" is mentioned, and mostly the phrase "HTML tag", but very few mentions of the actual <html> tag. Do we want to lowercase all the cases of the allcaps HTML? That seems a bit much, and then what do we do about references to XML and others? If want to replace all mentions of the actual html tag with <html> somebody has to read all of it.

joelpittet’s picture

Re #10 yes only when refering to actual tags and not the acronyms.

An HTML tag

!=

An <html> tag
jhodgdon’s picture

This issue is mostly about other tags, like referring to "the BODY tag" vs. "the <body> tag". Definitely if you are referring to the markup language, you should always say HTML, but as mentioned in #11, if you are referring to the outer <html> tag, according to this proposal/issue, it should be lower-cased.

davidhernandez’s picture

Ok, so this issue is to make a patch to change all instances of the inserttagname tag into the <inserttagname> tag.

jhodgdon’s picture

Yes.

andypost’s picture

So maybe better to split this one on per-module basis, and attach on upcoming sprints?

jhodgdon’s picture

We should only split this up if the patch would be too big to do as one, and then not necessarily by module.... We don't really want 50 small patches; nor do we want one that is too huge to review. How many changes are we looking at?

davidhernandez’s picture

There probably aren't as many changes as we might think. Finding them I think is the harder part. I'm doing some simple grepping and getting a lot of false positives. I'm not sure if there is a clever way that is better than just searching for every tag name and the reading the line for content. Not all are followed by the word tag even.

Maybe best effort is enough; fix the ones that are easy to find and leave the rest to get fixed organically?

andypost’s picture

Makes sense, also we need to add the topic to docs somewhere to point noobs

ifrik’s picture

This could be taken up during the sprints at DrupalCon Barcelona.

To do so, we need to update the description of the issue, and then tag it with Barcelona2015

davidhernandez’s picture

Issue summary: View changes
Issue tags: +Barcelona2015
mikebell_’s picture

FileSize
2.34 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,723 pass(es). View

Bartik theme is clean.
I've found a few instances in Classy and Seven which I've fixed.

akalata’s picture

Status: Active » Needs review

@mikebell_, in order for testbot to trigger on the patch, the issue status must be set to "Needs review".

XaviP’s picture

Status: Needs review » Needs work

Reviewed patch #21:

I think the two first corrections are wrong. The comment in code is talking about available variables in the template:

 * - header: Table header cells. Each cell contains the following properties:
 *   - tag: The HTML tag name to use; either TH or TD.

This properties are used in the template:

  {% if header %}
    <thead>
      <tr>
        {% for cell in header %}
          <{{ cell.tag }}{{ cell.attributes }}>

So if you use <th> or <td> the markup will be wrong.

mikebell_’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,721 pass(es). View

Yeah that makes way more sense. Rather than keeping them as uppercase I've left it lowercase to conform with the initial OP.

joachim’s picture

+++ b/core/themes/classy/templates/dataset/table.html.twig
@@ -24,7 +24,7 @@
- *     - tag: The HTML tag name to use; either TH or TD.
+ *     - tag: The HTML tag name to use; either th or td.

If that is telling me what value to use, then it should be quoted as a string: either 'th' or 'td'.

erik.erskine’s picture

Reviewing this now with @darrenwh at Barcelona sprint.

LewisNyman’s picture

Status: Needs review » Needs work
+++ b/core/themes/classy/templates/dataset/table.html.twig
@@ -12,7 +12,7 @@
- *   - tag: The HTML tag name to use; either TH or TD.
+ *   - tag: The HTML tag name to use; either th or td.

+++ b/core/themes/classy/templates/form/fieldset.html.twig
@@ -36,7 +36,7 @@
-  {#  Always wrap fieldset legends in a SPAN for CSS positioning. #}
+  {#  Always wrap fieldset legends in a <span> for CSS positioning. #}

+++ b/core/themes/seven/css/components/tables.css
@@ -112,7 +112,7 @@ td.is-active {
-/* Force browsers to calculate the width of a 'select all' TH element. */
+/* Force browsers to calculate the width of a 'select all' <th> element. */

This doesn't seem to be consistent, in one place we are using <> and the other we aren't?

erik.erskine’s picture

+++ b/core/themes/classy/templates/dataset/table.html.twig
@@ -12,7 +12,7 @@
+ *   - tag: The HTML tag name to use; either th or td.

In this instance, the comment for that parameter is referring to possible values that can be used. That value would not contain the angle brackets.

Something like this illustrates the context: (but might be a bit verbose)
- tag: The HTML tag name to use; either "th" or "td" for a <th> or <td> cell respectively.

XaviP’s picture

Status: Needs work » Needs review
FileSize
4.22 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 72,423 pass(es), 11,007 fail(s), and 7,370 exception(s). View
3.14 KB
  • Corrected as proposed in #28
  • Found another pair of TH-TD comments in core/modules/system/templates/table.html.twig
  • I've been using this command, maybe this can help somebody to find more:
    grep -nr '^\s\*\s' . | grep '\sTD\|\sTH\s\|\sFORM\s\|DIV\|HEADER\|TABLE\|SPAN\|IMG'
XaviP’s picture

FileSize
3.6 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,953 pass(es), 1 fail(s), and 0 exception(s). View
2.66 KB

This are the good patch and interdiff.

erik.erskine’s picture

Hey @XaviP - are you at the sprint in Barcelona today? We're on the multilingual table in room 115 if you want to join us.

The last submitted patch, 29: 546248-29.patch, failed testing.

XaviP’s picture

@ingaro: Nice to meet you!
I'm in documentation (en/es) table.

darrenwh’s picture

Issue summary: View changes
FileSize
1.39 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,985 pass(es). View

Adding extra tags I have found and annotated correctly, this is in addition to 29 and a interdiff to 24

{Please Ignore}

darrenwh’s picture

Issue summary: View changes
FileSize
5.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,961 pass(es). View
1.39 KB

Adding a interdiff for 30, correcting/replacing 34.

davidhernandez’s picture

It would appear the entire issue summary was deleted. Can someone restore that please?

erik.erskine’s picture

Issue summary: View changes

Restoring issue summary.

The last submitted patch, 29: 546248-29.patch, failed testing.

erik.erskine’s picture

FileSize
14.87 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,974 pass(es). View

New patch with some more instances in comments in twig templates and core/includes/theme.inc

erik.erskine’s picture

+++ b/core/themes/classy/templates/form/select.html.twig
@@ -4,8 +4,8 @@
  * Theme override for a select element.
  *
  * Available variables:
- * - attributes: HTML attributes for the select tag.
- * - options: The option element children.
+ * - attributes: HTML attributes for the <select> tag.
+ * - options: The <option> element children.

Re. comments in #27 and #28: Some clarification is probably needed on what is an HTML tag and what is not - here select element could mean a tag or a form API element. The patch in #39 assumes it's the latter and leaves the word select unchanged unless it's clearly referring to an HTML tag.

The last submitted patch, 30: 546248-30.patch, failed testing.

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.

cdykstra’s picture

I'm reviewing the latest patch at DrupalCon NOLA 2016 mentored sprint.

keithdoyle9’s picture

I've read through the documentation updates. After lunch I'm going to apply the latest patch and test it.

keithdoyle9’s picture

Reviewed the latest patch, after lunch will be applying and testing the patch.

cdykstra’s picture

this line needs to wrap at 80 characters:

- tag: The HTML tag name to use; either 'th' or 'td' for a

or cell respectively.

there are several instances of this line in the patch

cdykstra’s picture

Status: Needs review » Needs work
urbanlegend’s picture

Re-rolled the last patch to fix conflicts with core/includes/theme.inc

urbanlegend’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Thanks.. I started reviewing the patch, but it appears to have a LOT of stuff in it that is way out of scope for this issue. Here is what I reviewed... I stopped after a while because there was too much stuff in this patch unrelated to this issue... We need to go back to a previous patch (or start over?) and just do what is needed to solve this issue.

  1. +++ b/core/includes/theme.inc
    @@ -601,8 +601,8 @@ function template_preprocess_datetime_wrapper(&$variables) {
    - *   - attributes: A keyed array of attributes for the <ul> containing the list
    - *     of links.
    + *   - attributes: A keyed array of attributes for the <ul> containing the
    + *     list of links.
    

    I do not understand why this change is in the patch. The wrapping was fine before the change. Please do not put changes like this into this patch. Thanks!

  2. +++ b/core/includes/theme.inc
    @@ -631,6 +631,15 @@ function template_preprocess_datetime_wrapper(&$variables) {
    + * Unfortunately links templates duplicate the "active" class handling of l()
    + * and LinkGenerator::generate() because it needs to be able to set the "active"
    + * class not on the links themselves (<a> tags), but on the list items (<li>
    + * tags) that contain the links. This is necessary for CSS to be able to style
    + * list items differently when the link is active, since CSS does not yet allow
    + * one to style list items only if it contains a certain element with a certain
    + * class. I.e. we cannot yet convert this jQuery selector to a CSS selector:
    + *   jQuery('li:has("a.is-active")')
    

    This change is out of scope for the issue. Seems to be adding documentation... file another issue if you want to make a chagne like this. Thanks!

  3. +++ b/core/includes/theme.inc
    @@ -844,28 +853,28 @@ function template_preprocess_image(&$variables) {
    + *       include a "data" attribute. To add attributes to <col> elements, set the
    

    This line is now over 80 characters and needs rewrapping.

  4. +++ b/core/includes/theme.inc
    @@ -1106,6 +1115,23 @@ function template_preprocess_item_list(&$variables) {
     /**
    + * Returns HTML for an indentation <div>; used for drag and drop tables.
    + *
    + * @param $variables
    + *   An associative array containing:
    + *   - size: Optional. The number of indentations to create.
    + *
    + * @ingroup themeable
    + */
    +function theme_indentation($variables) {
    +  $output = '';
    +  for ($n = 0; $n < $variables['size']; $n++) {
    +    $output .= '<div class="js-indentation indentation">&nbsp;</div>';
    +  }
    +  return $output;
    +}
    +
    +/**
    

    Um, for some reason a whole function was added in this patch... why?

    So this is where I stopped reviewing. Needs a new patch.

emma.maria’s picture

Issue tags: +Needs reroll

The content of the patch in #39 is good. It needs a reroll and then we can carry on with the progress here :-)

scythian’s picture

Hi there,
Here is latest patch for branch 8.1.*, based on re-rolled patch from comment #39

scythian’s picture

Status: Needs work » Needs review
emma.maria’s picture

Thanks @scythian!

emma.maria’s picture

Issue tags: -Needs reroll
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! Some of this looks good; other bits need some work:

  1. +++ b/core/includes/theme.inc
    @@ -858,28 +858,28 @@ function template_preprocess_image(&$variables) {
    + *       attributes applied to the <col> element.
    + *     - An array of attributes applied to the <colgroup> element, which must
    + *       include a "data" attribute. To add attributes to <col> elements, set the
    

    Um. I don't know of any HTML "col" tag? I think this should be left as it was. This is actually table processing code, so probably it is going to end up as TD tags, not COL tags. COL here must be referring to something else, not an HTML element.

  2. +++ b/core/includes/theme.inc
    @@ -858,28 +858,28 @@ function template_preprocess_image(&$variables) {
    - *       // COLGROUP with one COL element.
    + *       // <colgroup> with one <col> element.
      *       array(
      *         array(
    - *           'class' => array('funky'), // Attribute for the COL element.
    + *           'class' => array('funky'), // Attribute for the <col> element.
      *         ),
      *       ),
    - *       // Colgroup with attributes and inner COL elements.
    + *       // <colgroup> with attributes and inner <col> elements.
      *       array(
      *         'data' => array(
      *           array(
    - *             'class' => array('funky'), // Attribute for the COL element.
    + *             'class' => array('funky'), // Attribute for the <col> element.
      *           ),
      *         ),
    - *         'class' => array('jazzy'), // Attribute for the COLGROUP element.
    + *         'class' => array('jazzy'), // Attribute for the <colgroup> element.
    

    See above note, this all should be left as it was.

  3. +++ b/core/misc/ajax.js
    @@ -1036,7 +1036,7 @@
    +      // elements are not allowed (e.g., within <table>, <tr>, and <span> parents),
    

    This line is now over 80 characters. The comment block here needs to be rewrapped.

  4. +++ b/core/modules/system/templates/fieldset.html.twig
    @@ -4,12 +4,12 @@
    - *   - attributes: HTML attributes to apply to the legend.
    + *   - attributes: HTML attributes to apply to the <legend> element.
    

    I really am not sure about this change...

    But if you are going to make this change, shouldn't it be done consistently? A few lines up there is "the fieldset element" and "the legend element" and neither of those is changed. I don't understand why you have changed this line (which may not need to be changed) and not the others.

  5. +++ b/core/modules/system/templates/table.html.twig
    @@ -12,7 +12,7 @@
    - *   - tag: The HTML tag name to use; either TH or TD.
    + *   - tag: The HTML tag name to use; either 'th' or 'td' for a <th> or <td> cell respectively.
    

    This line is over 80 characters, needs to be wrapped.

    But the wording here is wrong... why introduce the word "cell"? It wasn't there before. Please limit the changes in this patch to only what is needed to resolve this issue.

    There are several other places where new wording is introduced. Please go through and take them all out.

  6. +++ b/core/modules/system/templates/table.html.twig
    @@ -24,7 +24,7 @@
    + *     - tag: The HTML tag name to use; either 'th' or 'td' for a <th> or <td> cell respectively.
    
    +++ b/core/modules/system/templates/textarea.html.twig
    @@ -5,7 +5,7 @@
      * - value: The textarea content.
    

    see above.

  7. +++ b/core/themes/classy/templates/dataset/table.html.twig
    @@ -12,7 +12,7 @@
    + *   - tag: The HTML tag name to use; either 'th' or 'td' for a <th> or <td> cell respectively.
    

    see above.

  8. +++ b/core/themes/classy/templates/dataset/table.html.twig
    @@ -24,7 +24,7 @@
    + *     - tag: The HTML tag name to use; either 'th' or 'td' for a <th> or <td> cell respectively.
    

    see above.

scythian’s picture

Assigned: Unassigned » scythian
scythian’s picture

Hi jhodgdon,

Thanks for Your comments above. I've fixed some of them but have skipped few comments.

1. Skipped.
HTML tags used very seldom, but it really exist. And used for column styles settings.
http://www.w3schools.com/tags/tag_col.asp

2. Skipped
@see above.

3. Fixed.

4. Fixed.
Just made same wrappers for and tags. As I understand from proposed resolution, any directly referred tag should be wrapped.

5./6./7./8. Fixed. Reset to default text and wrapped.

scythian’s picture

Status: Needs work » Needs review
scythian’s picture

Assigned: scythian » Unassigned
jhodgdon’s picture

Status: Needs review » Needs work

Looking better, thanks! I agree about the COL stuff, my bad!

A few things still to fix:

  1. +++ b/core/modules/system/templates/table.html.twig
    @@ -12,7 +12,7 @@
      * - header: Table header cells. Each cell contains the following properties:
    - *   - tag: The HTML tag name to use; either TH or TD.
    + *   - tag: The HTML tag name to use; either <th> or <td>.
    

    I think this change is incorrect. The value here is the tag name, which is either 'th' or 'td', not <th>.

  2. +++ b/core/modules/system/templates/table.html.twig
    @@ -24,7 +24,6 @@
    - *     - tag: The HTML tag name to use; either TH or TD.
    

    Why was this line removed?

  3. +++ b/core/themes/classy/templates/dataset/table.html.twig
    @@ -12,7 +12,7 @@
    - *   - tag: The HTML tag name to use; either TH or TD.
    + *   - tag: The HTML tag name to use; either <th> or <td>.
    

    See above, this is the tag *name*, not the tag itself.

  4. +++ b/core/themes/classy/templates/dataset/table.html.twig
    @@ -24,7 +24,7 @@
    - *     - tag: The HTML tag name to use; either TH or TD.
    + *     - tag: The HTML tag name to use; either <th> or <td>.
    

    see above

  5. +++ b/core/themes/stable/templates/admin/field-ui-table.html.twig
    @@ -12,7 +12,7 @@
    - *   - tag: The HTML tag name to use; either TH or TD.
    + *   - tag: The HTML tag name to use; either <th> or <td>.
    

    see above

  6. +++ b/core/themes/stable/templates/admin/field-ui-table.html.twig
    @@ -24,7 +24,7 @@
    - *     - tag: The HTML tag name to use; either TH or TD.
    + *     - tag: The HTML tag name to use; either <th> or <td>.
    

    see above

  7. +++ b/core/themes/stable/templates/dataset/table.html.twig
    @@ -12,7 +12,7 @@
    - *   - tag: The HTML tag name to use; either TH or TD.
    + *   - tag: The HTML tag name to use; either <th> or <td>.
    

    see above

  8. +++ b/core/themes/stable/templates/dataset/table.html.twig
    @@ -24,7 +24,7 @@
    - *     - tag: The HTML tag name to use; either TH or TD.
    + *     - tag: The HTML tag name to use; either <th> or <td>.
    

    see above

scythian’s picture

Assigned: Unassigned » scythian
scythian’s picture

Assigned: scythian » Unassigned
Status: Needs work » Needs review
FileSize
18.51 KB

Just fixed by Jennifer comments above (#61).

jhodgdon’s picture

Status: Needs review » Needs work

Thanks!

All of the changes in this patch look good to me, except one very small problem:

+++ b/core/includes/theme.inc
@@ -858,28 +858,28 @@ function template_preprocess_image(&$variables) {
+ *       include a "data" attribute. To add attributes to <col> elements, set the

This line is over 80 characters. Needs to be rewrapped.

I haven't verified that this fixes all of Core. Does someone want to check that? How did the people making this patch search for places that needed to be fixed?

scythian’s picture

Status: Needs work » Needs review
FileSize
18.58 KB

Hi Jennifer,
Just fixed line width for core/includes/theme.inc:863.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

  • xjm committed 8e2cf9d on 8.2.x
    Issue #2546248 by scythian, XaviP, darrenwh, mikebell_, ingaro,...

  • xjm committed fcf6846 on 8.1.x
    Issue #2546248 by scythian, XaviP, darrenwh, mikebell_, ingaro,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Thanks everyone for working on this patch! This consistency makes the documentation more readable (and also seems more in line with modern HTML).

@webchick, @effulgentsia, and I discussed whether this is implicitly adopting a coding standard and therefore should have a coding standards issue as per: https://www.drupal.org/core/scope#coding-standards
It seems like this was discussed as being a "de-facto standard" in coments #2 through #6 or so.

In this case, it's somewhat borderline because we cannot write an automatic rule to find all instances of tags referenced inconsistently in comments. (It would however be possible to write a rule to check for e.g. <TH> and indicate that it should be <th>.

Since it's borderline as to whether this would need to be a coding standard vs. a content style recommendation, we agreed to commit this patch while it applies (since it definitely improves readability), and ask for a followup issue to discuss whether to make this a standard (or not).

Committed to 8.2.x and cherry-picked to 8.1.x! Thanks everyone. Let's create a followup issue to discuss this documentation standard and at a minimum document it in the style guidelines even if it doesn't turn out to merit its own rules.

jhodgdon’s picture

Did you want an issue in the Coding Standards project? I'm not sure where we would document this style, or whether it merits a Coding Standards project discussion.

xjm’s picture

Issue tags: -Needs followup

@jhodgdon, turns out you and @joachim already started this: #2711751: add a standard for referring to HTML tags in code documentation :)

Adding that and removing the needs followup tag. I think we should probably postpone any future docs cleanups like this on adopting that.

xjm’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.