Make help module pass Coder Review

Here is the status of code review before I started.

FILE: /Users/neokrish/d8/drupal/core/modules/help/help.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 51 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
 56 | ERROR | If the line declaring an array spans longer than 80 characters,
    |       | each element should be broken into its own line
--------------------------------------------------------------------------------


FILE: /Users/neokrish/d8/drupal/core/modules/help/help.test
--------------------------------------------------------------------------------
FOUND 11 ERROR(S) AFFECTING 11 LINE(S)
--------------------------------------------------------------------------------
  18 | ERROR | Class property $big_user should use lowerCamel naming without
     |       | underscores
  23 | ERROR | Class property $any_user should use lowerCamel naming without
     |       | underscores
  25 | ERROR | Missing function doc comment
  33 | ERROR | Missing function doc comment
  39 | ERROR | If the line declaring an array spans longer than 80 characters,
     |       | each element should be broken into its own line
  84 | ERROR | Concatenating translatable strings is not allowed, use
     |       | placeholders instead and only one string literal
  85 | ERROR | Concatenating translatable strings is not allowed, use
     |       | placeholders instead and only one string literal
  86 | ERROR | Closing brace indented incorrectly; expected 6 spaces, found 7
 115 | ERROR | Class property $big_user should use lowerCamel naming without
     |       | underscores
 117 | ERROR | Missing function doc comment
 125 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------

Please find attached a patch to fix around 30 issues in various files of help module as reported by pareview.sh. All the issues are mentioned in the attached help_d8_issues.txt file.

Although all these issues are minor but we may need to fix them by applying patch so that code is clean and as per coding standards.

Thanks for the patch, @amitgoyal. I like how you put the pareview output in an attachment. We should adopt that practice across this initiative. Here's some feedback on the patch:

1. +++ b/core/modules/help/help.admin.inc
   @@ -20,7 +20,7 @@ function help_main() {
   - * @param $name
   + * @param string $name
   

   Adding @param and @return types has been excluded from this initiative. Please read #1518116: [meta] Make Core pass Coder Review, Proposed Resolution #5.

2. +++ b/core/modules/help/help.module
   @@ -68,7 +77,9 @@ function help_help($path, $arg) {
   - * Implements hook_preprocess_HOOK() for block.tpl.php.
   + * Implements hook_preprocess_HOOK().
   + *
   + * This implementation is for block.tpl.php.
   

   This was actually correct as it was, per http://drupal.org/node/1354#hookimpl. See #1443202: Apply documentation standards for hook_preprocess_HOOK().

3. +++ b/core/modules/help/lib/Drupal/help/Tests/HelpTest.php
   @@ -27,13 +27,16 @@ class HelpTest extends WebTestBase {
   +  /**
   +   * Returns Info.
   +   */
   

   Standard simpletest functions getInfo(), setUp(), and tearDown() don't get doxygen comments. We need to make sure the documentation explicitly states this somewhere and then file an issue against drupalcs for a false positive. Doing that latter would be a help to this initiative, if you'd like to do it!

4. +++ b/core/modules/help/lib/Drupal/help/Tests/HelpTest.php
   @@ -42,30 +45,37 @@ class HelpTest extends WebTestBase {
   -  function testHelp() {
   +  public function testHelp() {
   

   Last I saw, we were still awaiting direction on the approach to take with specifying simpletest class access. We need to get clarification and direction from @jhodgdon.

5. +++ b/core/modules/help/lib/Drupal/help/Tests/HelpTest.php
   @@ -93,9 +103,9 @@ class HelpTest extends WebTestBase {
   -        $this->assertTitle($name . ' | Drupal', t('[' . $module . '] Title was displayed'));
   -        $this->assertRaw('<h1 class="page-title">' . t($name) . '</h1>', t('[' . $module . '] Heading was displayed'));
   -       }
   +        $this->assertTitle($name . ' | Drupal', t("[$module] Title was displayed"));
   +        $this->assertRaw('<h1 class="page-title">' . t($name) . '</h1>', t("[$module] Heading was displayed"));
   +      }
   

   The $module value should be passed into t() in an $args array argument.

There are kind of a lot of blockers on this issue and kin. The best progress right now will probably be made addressing the blockers rather than actually writing patches. Thanks for getting involved!

RE previous comment point 5 - actually we don't want to be using t() at all in test assertions -- see #1794012: Remove t() from asserts for database system tests and related issues.

RE point 4 I have no idea whether the methods in test classes should be public, protected, or what. Why are they being updated at all? Is something flagging them as an error if they are unspecified? I don't think we have anything in our coding standards saying that every method should have public/private/protected on it do we? If not, that is a Coder/checker error that should be taken care of.\

RE point 3 it is already documented in the standards for how to write tests, and there is also an issue about maybe changing the standards because it is inconsistent with how we do things otherwise:
#338403: Use {@inheritdoc} on all class methods (including tests)

Thanks a lot @TravisCarden and @jhodgdon for reviewing this and providing your inputs. I have made the following changes as suggested by you,

1) Removed types for @param and @return wherever required.
2) Reverted back the change to "Implements hook_preprocess_HOOK() for block.tpl.php."
3) Removed the doxygen comments from getInfo(), setUp(), and tearDown() functions.
4) Removed "public" scope modifier from test class methods wherever I have added them.
5) As suggested by @jhodgdon, I have removed t() in test assertions.

Due to these updates, pareview.sh is reporting some issues which I have attached here as help_d8_ignore_issues.txt. That means we are not fixing these issues as per your suggestions which makes more sense.

Please find attached updated patch file help_d8_1533244_2012_09_28.patch and let me know if more changes are required!

Thanks! Can we make sure issues are filed for whatever code checker you are using so these false positives that are not part of our standards are not flagged (aside from the param/return types, which is in the standards)?

Also, this change (and a few below that are similar) are not required by any of our coding standards that I am aware of:

-      $output .= theme('item_list', array('items' => $links, 'title' => t('@module administration pages', array('@module' => $info[$name]['name']))));
+      $output .= theme('item_list', array(
+        'items' => $links,
+        'title' => t('@module administration pages', array(
+          '@module' => $info[$name]['name'],
+          )),
+        )
+      );

And this one is scary enough that please don't assign this issue to me to commit, because I will leave that to someone else to decide on:

/**
    * The admin user that will be created.
    */
-  protected $big_user;
+  protected $bigUser;

@jhodgdon - As mentioned earlier, I am using pareview.sh script by @klausi. He is a Project application review administrator and security team member.

I don't mind reverting the last 2 suggested changes by you but can you please confirm on but basis we should fix coding standard issues? Just based on the Coder module suggestions or pareview.sh script which we use during new project application.

Please confirm and I will make the required changes.

@amitgoyal: See #1518116: [meta] Make Core pass Coder Review, Proposed Resolution #5. We don't use pareview.sh. We use Coder and drupalcs, but there are unfortunately still a lot of false positives and unresolved ambiguity.

Thanks for working on this issue @amitgoyal.

I would suggest creating a follow-up issue to this one that contains only the missing type hinting of @param and @return directives. Such patches are take time to review and commit. However, it would great to capture the start you made in #2 so that we can eventually get all modules to conform with coding and documentation standards.

I happily will review any type hinting only patches.

For those that care, I have opened up #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing* to specificly address the time consumming task of getting @param and @return type hinting added to core code files.

As a follow-up on this pareview.sh script... why is it flagging issues that are not part of our coding standards either?? Someone should follow-up on that too. There is no reason to hold new projects to standards that we don't even have.

@TravisCarden - drupalcs has been merged into Coder module. What is the best to review d8 modules using Coder locally as there is no 8.x release for Coder module?

I did this by removing d7's help module and putting help module of d8 in d7 folder and then changing .info files of help module. It reported just 1 error and the results are similar to http://qa.drupal.org/8.x-status#tabset-tab-4. Results,

modules/help/lib/Drupal/help/Tests/HelpTest.php
HelpTest.php

    severity: minorLine 98: Use an indent of 2 spaces, with no tabs

           }

Not sure if it makes sense to provide the patch for this one small change? Anyways, attaching the patch here for this one minor change. You may choose to ignore this patch and close this issue as everything else looks fine.

@TravisCarden - drupalcs has been merged into Coder module. What is the best to review d8 modules using Coder locally as there is no 8.x release for Coder module?

I did this by removing d7's help module and putting help module of d8 in d7 folder and then changing .info files of help module. It reported just 1 error and the results are similar to http://qa.drupal.org/8.x-status#tabset-tab-4. Results,

modules/help/lib/Drupal/help/Tests/HelpTest.php
HelpTest.php

    severity: minorLine 98: Use an indent of 2 spaces, with no tabs

           }

Not sure if it makes sense to provide the patch for this one small change? Anyways, attaching the patch here for this one minor change. You may choose to ignore this patch and close this issue as everything else looks fine.

@amitgoyal: Hmm. That's a big change, and it doesn't look like the dust has settled yet. I would recommend postponing a little while.

#12: help_d8_1533244_2012_10_03.patch queued for re-testing.

amitgoyal: It seems that the issue summary on the parent issue for this effort has instructions for how to run Coder in D8 -- if the instructions are not clear, maybe Travis (the coordinator of this effort) can update them to be clearer?
#1518116: [meta] Make Core pass Coder Review

Yes @jhodgdon! The only thing is that drupalcs module has been merged into Coder module. So I am assuming we don't need to use drupalcs module anymore. We just need to apply the D8 patch in latest version of Coder module (D7).

@amitgoyal: Based on a quick dive into the issue, it appears that drupalcs has more or less just been dropped into Coder as-is and renamed Coder Sniffer. Perhaps you would like to try it out and report back on the outcome? (Unfortunately I don't have the bandwidth at the moment.) Otherwise I'd say you can continue using drupalcs and we just keep an eye on the developments in Coder.

@TravisCarden - I did the code review of help module using Coder 7.x-2.x branch as Coder Sniffer has been added in 7.x-2.x branch only. The D8 Patch didn't work for this Coder 7.x-2.x branch so I changed .info files and did the review.

Attaching help_issues_by_coder_7x2x_2012_10_05.html file which contains coding issues reported by Coder module using "Drupal CodeSniffer".

Most of the issues are similar to #2 as pareview.sh also uses coder + drupalcs but actually checks all files including css/js etc.

Please suggest if I should provide any patch as most of the reported issues are not supposed to be done as suggested by you and @jhodgdon.

Based strictly on the output you provided, here at the items that can be fixed as of today (attached). Thanks!

Thanks for running this!

Here are my thoughts on these errors:
a) If the line declaring an array spans longer than 80 characters, each element should be broken into its own line

This is a problem in the Coder module -- it is NOT part of our coding standards at all, and should not be "sniffed" as an error. We need to file an issue with Coder if that hasn't been already done.

b) Format should be "* Implements hook_foo()." or "Implements hook_foo_BAR_ID_bar() for xyz_bar()."
* Implements hook_preprocess_HOOK() for block.tpl.php.

We should also file an issue with Coder for this one, because "for *.tpl.php" is acceptable syntax for this.

c) Inline comments must end in full-stops, exclamation marks, or question marks
// Add CSS

This type of error is an actual error and should be patched in the help module.

d) Missing parameter type at position 1
* @param $name
or
Data type of return value is missing
* @return

These are legitimate errors, but as noted above, we're not addressing them in this particular cleanup effort.

e) Class property $big_user should use lowerCamel naming without underscores

That seems to be to be a legitimate error that should be patched.

f) Missing function doc comment
public static function getInfo() {

Our coding standards for tests unfortunately are not standard with our coding standards for other classes, and we specifically do *not* want doc comments on getInfo(), setUp(), and tearDown() methods in classes, so ignore this message for those methods.

g) No scope modifier specified for function "testHelp"
function testHelp() {

As mentioned above, as far as I know we do not have a coding standard saying that every method must be declared as public, protected, etc. and specifically our test writing standards do not require this. So this should be filed as an issue in coder because it is enforcing a coding standard that doesn't exist.

h) Closing brace indented incorrectly; expected 6 spaces, found 7
(and similar indentation/tab errors)

This type of error is an actual error and should be patched in the help module.

So... it looks like we need a patch to fix a few errors in the help module, and we need to make sure several issues are filed with the Coder module.

Travis: where is the coding standard about the array lines longer than 80 characters needing to be split? I don't think that is in our coding standards, so I don't understand why coder/sniffer is catching it?

I filed an issue http://drupal.org/node/1805542 in Coder issue queue to address #22 a).

Edit: Also opened http://drupal.org/node/1805544 to address false positive noted in #22 b).

Open an issue in Coder issue queue for #22 f). That issue is: http://drupal.org/node/1805550.

@Lars Toomre - Thanks a lot for opening the issues in Coder issue queue!

@TravisCarden and @jhodgdon - Thanks again for providing your feedback. I have made the required changes based on your feedback. Please review the attached patch.

@jhodgdon: The coding standards for arrays state "Note that if the line declaring an array spans longer than 80 characters (often the case with form and menu declarations), each element should be broken into its own line, and indented one level". In light of the fact, I marked @Lars Toomre's issue against Coder "works as designed". If it's decided to change or remove the standard, we'll re-open it, but I guess it's technically correct to be flagging it right now.

@Lars Toomre: A freebie: When linking to issues in the queue, use this syntax, [#nid], and it will automatically be turned into the title of the issue with color coding and an informative tooltip. E.g.:

• #1805542: Remove false positive for 'array spans longer than 80 characters"
• #1805544: Modify sniff for "Implements hook_foo_BAR_ID_bar() for xyz_bar()"
• #1805550: Modify sniff for "Missing function doc comment" for test classes

By the way, thanks for filing these! :)

Thanks for the tip @TravisCarden. When I tried that yesterday in creating posts #24 and #25 and looked at the preview, there was no substitution for [#nid]. I thought it must be that I was linking to something outside of the core issue queue. Hence, I went back and added the full URLs so that they at least showed up as a link.

I am glad that you added them in the usual format in #27. Thanks.

Thanks @amitgoyal for your patches in this issue. I reviewed your proposed changes in #26 and they generally look good.

However, the resulting member of test classes $this->bigUser makes me cringe. Within the last week, I posted at patch at #1803656: Improve Tests consistency to standards in modules A-M which attempts to bring some consistency to the various Test classes across the core modules. (It has yet to receive any reviews, which certainly would be welcome, so it can be committed). In the interim, may I suggest that you lift the changes there for the Help module and include them here?

The other thing that I would request before saying this is RBTC is to check that the code inline comments are correct. I am unsure if the Coder module checks for line length exceeding 80 characters or whether multiple inline comment lines are wrapped correctly.

After my issue about bigUser is addressed in a patch, I will apply this locally and do a code comment review so that we can get this in. Thanks for your work on this issue.

Regarding the array coding standard, this is under dispute, at least in some cases. See:
#1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines

@Lars Toomre - I checked on code inline comments for line length exceeding 80 characters or whether multiple inline comment lines are wrapped correctly. This looks good to me. Regarding other suggestion $this->bigUser, would like to take inputs from @TravisCarden and @jhodgdon as both of them suggested to make such changes.

@TravisCarden / @jhodgdon - Please confirm if anything else needs to be done here. Otherwise we can apply the given patch.

Oh, actually I found that there is a standard for all class properties/methods having protected/public/private (#22-g)... And the standard specifically suggests that nearly all should be "protected" (see http://drupal.org/node/608152 -- the "visibility" section has the general standard). However, the test standards page (http://drupal.org/node/325974) shows that test methods should be declared "public". Anyway, it looks like those should be patched.

@amitgoyal I have no problem with the conversion of a member like $big_user to $bigUser.

However, in this case, I would suggest that we use a member name akin to adminUser as 'bigUser' has negative connotations in some circles. Does that make sense? Hence, I would replace all occurances of bigUser with adminUser in the most recent code.

@Lars - Sure, I have changed the variable name from $big_user to $adminUser.

@jhodgdon - Based on test standards page (http://drupal.org/node/325974), I have made test methods "public" wherever it wasn't mentioned.

Please review again. Hope everything looks good now and is ready to commit. :)

Thank you for the request change to adminUser. From doing a review of the patch, this looks good to RTBC. Let's see what the bot thinks.

@TravisCarden / @jhodgdon - Please let me know if anything else needs to be done here.

Hi amitgoyal -- I agree that the patch, as noted in #35, all looks like good changes. It would help if you could apply the patch and post the Coder output with the patch applied, so we can see if any further changes are needed (to compare with the output in #20.

@jhodgdon - I am able to apply the patch successfully. I won't be able to review it again using Coder module as it is referring to 'system' table at few places whereas in D8 'system' table has been removed.

http://drupal.org/node/1813642

I think we may need to wait for sometime so that the same can be fixed in Coder module.

OK then, can someone review the changes in the patch and verify that everything noted in #20 that was supposed to be fixed (see next few comments) has been fixed? That is the kind of review we need for this patch. Thanks!

@jhodgdon - I have created a patch for Coder module #1816302: Code Review 7.x-1.2 to 8.x-1.x Patch so that we can continue with this task. Please find attached a txt file which contains remaining issues in Help module as mentioned by Coder module after applying the #34 patch.

Please let me know if anything else needs to be done.

Looks like the only sniff to possibly address is this one:

severity: minorreview: sniffer_a5e3588cc10399225a71c9ee7aa0bd24Line 51: If the line declaring an array spans longer than 80 characters, each element should be broken into its own line [sniffer_a5e3588cc10399225a71c9ee7aa0bd24]

$this->adminUser = $this->drupalCreateUser(array('access administration pages', 'view the administration theme', 'administer permissions'));

I find it interesting that only five type hints are missing. We can add those in #1811862: Add missing type hinting to Help module docblocks.

The other false positive sniffs about getInfo() and setUp() have been filed for correction in Coder queue in #1805550: Modify sniff for "Missing function doc comment" for test classes.

@amitgoyal - If you have the chance, can you run some more files like in #40 for other sub-issues of #1518116: [meta] Make Core pass Coder Review? Thanks in advance.

RE #41 - see comment #30 - that standard is under discussion so we don't want to patch it right now.

So, the patch is as complete as we want it to be -- thanks for running Coder again! Anyway, this patch has been reviewed, and I just took another look, and it seems to be ready. I'll get it committed shortly.

Thanks to all who helped out on this one! Committed to 8.x.