Only the default output (usually) from a module should say in its @file docblock:
Default theme implementation…
All or most of Classy's template docblocks start with those words since they were originally copied from the default theme implementation.
As with Bartik and Seven, change Classy's template docblocks so they start with the (base) theme name: "Classy's theme implementation…"
Example before/after code from Classy's page.html.twig:
{#
/**
* @file
* Default theme implementation to display a single page.
*{#
/**
* @file
* Classy's theme implementation to display a single page.
*| Issue category | Bug because it doesn't mesh with our existing conventions |
|---|---|
| Issue priority | Normal |
| Unfrozen changes | Unfrozen because it only changes documentation |
| Prioritized changes | This is not a prioritized change for the beta phase. |
| Disruption | No disruption for core or contrib. |
n/a
n/a
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | fixed_docblocks_on_classy_templates-2452363-12.patch | 42.05 KB | joekers |
| #10 | fixed_docblocks_on_classy_templates-2452363-10.patch | 42.04 KB | joekers |
| #3 | fixed_docblocks_on_classy_templates-2452363-3.patch | 43.21 KB | joekers |
| #2 | fixed_docblocks_on_classy_templates_2452363.patch | 43.21 KB | joekers |
Comments
Comment #1
joekersComment #2
joekersMy first contribution to Drupal - hope it's ok? Simply updated all of the templates in the Classy theme like you said.
Comment #3
joekersRenamed the patch once I'd read the Novice Contribution Guide :)
Comment #4
joekersComment #5
star-szrThanks @joekers, works for me! I reviewed the patch with
git diff --color-wordsand checked the diff stats (with Dreditor orgit diff --shortstat) to ensure that this gets all the .html.twig files inside Classy:Comment #6
joekersGreat! I didn't know those commands before - thanks :)
Comment #7
star-szrHere's the output from --shortstat :)
Comment #8
jhodgdonHm. So what happens is that these doc blocks get copied and pasted around from one theme to another, and usually the docs do not get updated.
I'm not sure I like the idea of putting "Classy" in each one -- I think it's likely to result in a proliferation of other themes having "Classy" in their doc blocks. Maybe it should just say "Override of", so it is more generic? The file location will make it clear it is part of the Classy theme.
Comment #9
star-szrI like that idea, was just trying to follow what Bartik and Seven were doing but that is very likely an improvement for all :)
Comment #10
joekersI've removed the "Classy's theme" bit so it just reads "Override of" or "Implementation for" etc.
Comment #11
jhodgdonI think they should all say "Override" shouldn't they? They are all files where Classy is overriding the default theming for something. They are never "implementations" -- that is an example of a copy-and-paste problem where someone copied the Core implementation of a theme template and left the documentation alone.
And maybe they should include the word "theme" or "template" in there somewhere? For example:
Probably this should say "Theme override for the search form block.", or "Template override for the search form block."?
Comment #12
joekersOk. I went with "Theme override" and kept it consistent across all files.
Comment #13
star-szrComment #14
jhodgdonLooks good to me, thanks!
Comment #16
star-szrLooks like a random testbot fail:
Drupal\simpletest\Tests\SimpleTestBrowserTest
Comment #17
joekersHmm it passed testing when I first submitted it - what should I do now?
Comment #19
star-szr@joekers no need to do anything, testbot has bad days sometimes :)
Comment #20
alexpottDocs are not frozen in beta. Committed 0346369 and pushed to 8.0.x. Thanks!