Make bartik theme pass Coder Review

Narrowing scope.

In the second comment in the bartik.theme file there was a warning on the code below:
* Implements hook_preprocess_HOOK() for page templates.

I put the "for page templates" part in a new paragraph but later found a page.html.twig file as part of the theme.
I tried to change the code to
* Implements hook_preprocess_HOOK() for page.html.twig.
like if it was a *.tpl.php file but the coder sniffer produced the same warning.

Am I right in thinking this should be the way this code is written/ did I do something stupid I haven't noticed yet?

Hi, @majoely. Welcome to Drupal.org and the core issue queue! Thanks for the patch. That's a great first pass. Your first instinct to change that doxygen to "* Implements hook_preprocess_HOOK() for page.html.twig." was actually correct. Coder Sniffer just hasn't been updated to support Twig files yet. I filed #2203627: "Implements hook_foo() for some-template.file" sniff doesn't support Twig template names for the false positive. If anybody's would like to pop over there and review/RTBC the patch, that would be awesome. In the meantime, I've updated my spork of Coder for this meta, so please update your local clone for the changes. (You'll need to `git fetch` and `git reset --hard` rather than `git pull` due to the rebase-based workflow.)

Here's an updated patch. I just noticed one issue I didn't fix:

+++ b/core/themes/bartik/bartik.theme
@@ -128,7 +133,7 @@ function bartik_menu_tree($variables) {
 /**
- * Implements theme_menu_tree__shortcut_default() {
+ * Implements theme_menu_tree__shortcut_default().
  */

There's no such thing as theme_menu_tree__shortcut_default(). Does anybody know what function or template file this actually corresponds to?

Oops. I did my interdiff backwards. :p

Those all look pretty sensible.

@TravisCarden, I dug around and found that function you mentioned in #6. Turns out it's a little menu-specific override for menu_tree. It's defined here in core: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/inclu...

I'm not sure what to put in the comment. I've found examples of people using it, who refer to it like THEME_menu_tree__MENUNAME. Do we use something like that?

Good work, @bryanbraun! Thanks for tracking that down. THEME_menu_tree__MENUNAME() seems the closest to anything we actually do in our docs. But there's not really an exact precedent. Three of the only four places in the entire codebase that declare they implement theme_{anything}() are right here in Bartik:

$ ag "Implements theme_" -Ui
core/themes/bartik/bartik.theme
134: * Implements theme_menu_tree().
141: * Implements theme_menu_tree__shortcut_default() {
148: * Implements theme_field__field_type().

core/modules/system/tests/themes/test_theme/test_theme.theme
9: * Implements THEME_preprocess_twig_theme_test_php_variables().

Here's what I would suggest: 1) Update the three docblocks above in bartik.theme to at least be consistent (i.e., make them THEME_{whatever}(), capitalized, and use THEME_menu_tree__MENUNAME() in the instance under discussion). Then we can RTBC this issue. 2) Create a new issue to address the documentation pattern (or lack thereof) across the whole codebase using the analogous #1443202: Apply documentation standards for hook_preprocess_HOOK() as a guide. Would you like to do the honors on one or both of those?

@TravisCarden, I rerolled the patch and applied the changes you suggested in #11. I had to make a few fixes to some new code that appeared in bartik.theme as well, but everything should pass now.

As for your 2nd suggestion, I'm not sure I understand the documentation pattern issue (is it with THEME_{whatever}()? ) or what a good solution looks like. You may be better suited to take the lead on that issue.

Great job, @bryanbraun! This is ready to go (unless a core committer has a ready solution to #11). I'll try to create an issue for the documentation pattern later and link to it from here.

Needs a reroll

error: patch failed: core/themes/bartik/bartik.theme:178
error: core/themes/bartik/bartik.theme: patch does not apply

Reroll

+++ b/core/themes/bartik/bartik.theme
@@ -10,13 +10,13 @@ use Drupal\Core\Template\RenderWrapper;
-  /** @var \Drupal\Core\Page\HtmlPage $page_object */
+  // @var \Drupal\Core\Page\HtmlPage $page_object

This one does not look right to me. While generally speaking it's a rule that in-line comments should be, well, inline… if you grep for @var \\Drupal' you'll see we've standardized on /** … */ for these, because they're PHPDoc.

Other than that, these look like fine changes, so I've simply reverted that one and committed/pushed to 8.x. Thanks!

Thanks, @webchick! Re the inline doxygen change, an issue has been filed with Coder to remove that sniff, and our Coder fork for this meta has been patched accordingly. See #1518116-76: [meta] Make Core pass Coder Review.