Problem/Motivation

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.

Proposed resolution

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:

Before:

{#
/**
 * @file
 * Default theme implementation to display a single page.
 *

After:

{#
/**
 * @file
 * Classy's theme implementation to display a single page.
 *

Remaining tasks

  • Create patch
  • Review patch

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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.

User interface changes

n/a

API changes

n/a

Files: 
CommentFileSizeAuthor
#12 fixed_docblocks_on_classy_templates-2452363-12.patch42.05 KBjoekers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,488 pass(es). View
#10 fixed_docblocks_on_classy_templates-2452363-10.patch42.04 KBjoekers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,264 pass(es). View
#3 fixed_docblocks_on_classy_templates-2452363-3.patch43.21 KBjoekers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,188 pass(es). View
#2 fixed_docblocks_on_classy_templates_2452363.patch43.21 KBjoekers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,786 pass(es). View

Comments

joekers’s picture

Assigned: Unassigned » joekers
joekers’s picture

FileSize
43.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,786 pass(es). View

My first contribution to Drupal - hope it's ok? Simply updated all of the templates in the Classy theme like you said.

joekers’s picture

FileSize
43.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,188 pass(es). View

Renamed the patch once I'd read the Novice Contribution Guide :)

joekers’s picture

Status: Active » Needs review
Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @joekers, works for me! I reviewed the patch with git diff --color-words and checked the diff stats (with Dreditor or git diff --shortstat) to ensure that this gets all the .html.twig files inside Classy:

find core/themes/classy -type f -name *.html.twig | wc -l
      80
joekers’s picture

Great! I didn't know those commands before - thanks :)

Cottser’s picture

Here's the output from --shortstat :)

git diff --shortstat
 80 files changed, 80 insertions(+), 80 deletions(-)
jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

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

Cottser’s picture

I like that idea, was just trying to follow what Bartik and Seven were doing but that is very likely an improvement for all :)

joekers’s picture

FileSize
42.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 89,264 pass(es). View

I've removed the "Classy's theme" bit so it just reads "Override of" or "Implementation for" etc.

jhodgdon’s picture

Status: Needs review » Needs work

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

+++ b/core/themes/classy/templates/block/block--search-form-block.html.twig
 /**
  * @file
- * Default theme override for the search form block.
+ * Override for the search form block.

Probably this should say "Theme override for the search form block.", or "Template override for the search form block."?

joekers’s picture

FileSize
42.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,488 pass(es). View

Ok. I went with "Theme override" and kept it consistent across all files.

Cottser’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: fixed_docblocks_on_classy_templates-2452363-12.patch, failed testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

Looks like a random testbot fail:

Drupal\simpletest\Tests\SimpleTestBrowserTest

Requesting https.php with a legitimate simpletest User-Agent returns OK.
joekers’s picture

Hmm it passed testing when I first submitted it - what should I do now?

Cottser’s picture

@joekers no need to do anything, testbot has bad days sometimes :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Docs are not frozen in beta. Committed 0346369 and pushed to 8.0.x. Thanks!

  • alexpott committed 0346369 on 8.0.x
    Issue #2452363 by joekers: Classy's @file docblocks shouldn't say "...

Status: Fixed » Closed (fixed)

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