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
Discuss- Clean-up comments using uppercase tag names in comments and docblocks.
User interface changes
None.
API changes
None.
Beta phase evaluation
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. |
Comment | File | Size | Author |
---|---|---|---|
#65 | use-consistent-style-2546248-65.patch | 18.58 KB | scythian |
#63 | use-consistent-style-2546248-63.patch | 18.51 KB | scythian |
#58 | use-consistent-style-2546248-58.patch | 19.03 KB | scythian |
#52 | use-consistent-style-2546248-51.patch | 15.19 KB | scythian |
#48 | 2546248-48.patch | 15.62 KB | urbanlegend |
Comments
Comment #2
joelpittetWhen 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.Comment #3
davidhernandezI agree with Joel. I prefer the literal tag form.
Comment #4
star-szrYup. The all caps besides being all caps feels like old school HTML with table layouts and stuff :(
Comment #5
star-szr(Double post)
Comment #6
jhodgdonIt 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?Comment #7
joelpittetLet's just re-use this issue?
Comment #8
joelpittetComment #9
jhodgdonComment #10
davidhernandezI 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.Comment #11
joelpittetRe #10 yes only when refering to actual tags and not the acronyms.
!=
Comment #12
jhodgdonThis 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.Comment #13
davidhernandezOk, so this issue is to make a patch to change all instances of
the inserttagname tag
intothe <inserttagname> tag
.Comment #14
jhodgdonYes.
Comment #15
andypostSo maybe better to split this one on per-module basis, and attach on upcoming sprints?
Comment #16
jhodgdonWe 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?
Comment #17
davidhernandezThere 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?
Comment #18
andypostMakes sense, also we need to add the topic to docs somewhere to point noobs
Comment #19
ifrikThis 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
Comment #20
davidhernandezComment #21
mikebell_ CreditAttribution: mikebell_ as a volunteer commentedBartik theme is clean.
I've found a few instances in Classy and Seven which I've fixed.
Comment #22
akalata CreditAttribution: akalata commented@mikebell_, in order for testbot to trigger on the patch, the issue status must be set to "Needs review".
Comment #23
XaviP CreditAttribution: XaviP commentedReviewed patch #21:
I think the two first corrections are wrong. The comment in code is talking about available variables in the template:
This properties are used in the template:
So if you use
<th>
or<td>
the markup will be wrong.Comment #24
mikebell_ CreditAttribution: mikebell_ as a volunteer commentedYeah that makes way more sense. Rather than keeping them as uppercase I've left it lowercase to conform with the initial OP.
Comment #25
joachim CreditAttribution: joachim commentedIf that is telling me what value to use, then it should be quoted as a string: either 'th' or 'td'.
Comment #26
erik.erskine CreditAttribution: erik.erskine as a volunteer commentedReviewing this now with @darrenwh at Barcelona sprint.
Comment #27
LewisNyman CreditAttribution: LewisNyman as a volunteer commentedThis doesn't seem to be consistent, in one place we are using <> and the other we aren't?
Comment #28
erik.erskine CreditAttribution: erik.erskine as a volunteer commentedIn 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.
Comment #29
XaviP CreditAttribution: XaviP commentedgrep -nr '^\s\*\s' . | grep '\sTD\|\sTH\s\|\sFORM\s\|DIV\|HEADER\|TABLE\|SPAN\|IMG'
Comment #30
XaviP CreditAttribution: XaviP commentedThis are the good patch and interdiff.
Comment #31
erik.erskine CreditAttribution: erik.erskine as a volunteer commentedHey @XaviP - are you at the sprint in Barcelona today? We're on the multilingual table in room 115 if you want to join us.
Comment #33
XaviP CreditAttribution: XaviP commented@ingaro: Nice to meet you!
I'm in documentation (en/es) table.
Comment #34
darrenwh CreditAttribution: darrenwh as a volunteer commentedAdding extra tags I have found and annotated correctly, this is in addition to 29 and a interdiff to 24
{Please Ignore}
Comment #35
darrenwh CreditAttribution: darrenwh as a volunteer commentedAdding a interdiff for 30, correcting/replacing 34.
Comment #36
davidhernandezIt would appear the entire issue summary was deleted. Can someone restore that please?
Comment #37
erik.erskine CreditAttribution: erik.erskine as a volunteer commentedRestoring issue summary.
Comment #39
erik.erskine CreditAttribution: erik.erskine as a volunteer commentedNew patch with some more instances in comments in twig templates and
core/includes/theme.inc
Comment #40
erik.erskine CreditAttribution: erik.erskine as a volunteer commentedRe. 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.Comment #43
cdykstra CreditAttribution: cdykstra commentedI'm reviewing the latest patch at DrupalCon NOLA 2016 mentored sprint.
Comment #44
keithdoyle9 CreditAttribution: keithdoyle9 as a volunteer commentedI've read through the documentation updates. After lunch I'm going to apply the latest patch and test it.
Comment #45
keithdoyle9 CreditAttribution: keithdoyle9 commentedReviewed the latest patch, after lunch will be applying and testing the patch.
Comment #46
cdykstra CreditAttribution: cdykstra commentedthis line needs to wrap at 80 characters:
- tag: The HTML tag name to use; either 'th' or 'td' for a
there are several instances of this line in the patch
Comment #47
cdykstra CreditAttribution: cdykstra commentedComment #48
urbanlegend CreditAttribution: urbanlegend as a volunteer and at Acquia commentedRe-rolled the last patch to fix conflicts with
core/includes/theme.inc
Comment #49
urbanlegend CreditAttribution: urbanlegend as a volunteer and at Acquia commentedComment #50
jhodgdonThanks.. 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.
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!
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!
This line is now over 80 characters and needs rewrapping.
Um, for some reason a whole function was added in this patch... why?
So this is where I stopped reviewing. Needs a new patch.
Comment #51
emma.mariaThe content of the patch in #39 is good. It needs a reroll and then we can carry on with the progress here :-)
Comment #52
scythian CreditAttribution: scythian at FFW commentedHi there,
Here is latest patch for branch 8.1.*, based on re-rolled patch from comment #39
Comment #53
scythian CreditAttribution: scythian at FFW commentedComment #54
emma.mariaThanks @scythian!
Comment #55
emma.mariaComment #56
jhodgdonThanks! Some of this looks good; other bits need some work:
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.
See above note, this all should be left as it was.
This line is now over 80 characters. The comment block here needs to be rewrapped.
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.
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.
see above.
see above.
see above.
Comment #57
scythian CreditAttribution: scythian at FFW commentedComment #58
scythian CreditAttribution: scythian at FFW commentedHi 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.
Comment #59
scythian CreditAttribution: scythian at FFW commentedComment #60
scythian CreditAttribution: scythian at FFW commentedComment #61
jhodgdonLooking better, thanks! I agree about the COL stuff, my bad!
A few things still to fix:
I think this change is incorrect. The value here is the tag name, which is either 'th' or 'td', not
<th>
.Why was this line removed?
See above, this is the tag *name*, not the tag itself.
see above
see above
see above
see above
see above
Comment #62
scythian CreditAttribution: scythian at FFW commentedComment #63
scythian CreditAttribution: scythian at FFW commentedJust fixed by Jennifer comments above (#61).
Comment #64
jhodgdonThanks!
All of the changes in this patch look good to me, except one very small problem:
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?
Comment #65
scythian CreditAttribution: scythian at FFW commentedHi Jennifer,
Just fixed line width for core/includes/theme.inc:863.
Comment #66
jhodgdonThanks!
Comment #69
xjmThanks 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.
Comment #70
jhodgdonDid 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.
Comment #71
xjm@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.
Comment #72
xjm