Make Action module pass Coder Review

Based on the learnings from #1533244: Make help module pass Coder Review issue, I have made fixes, related to coding standards, in Action module. As per learning there, I have ignored following issues,

- Missing parameter type at position x
- Data type of return value is missing
- If the line declaring an array spans longer than 80 characters, each element should be broken into its own line
- No scope modifier specified for function getInfo() in class files

Attaching a Patch here which will fix the remaining issues. I am also attaching Coder output coder_issues_action_2012_10_22.txt so that we can see the list of all issues.

I am not sure about this particular issue "Data type of return value is missing". Please let me know if I need to fix it.

The patch fixes the issues stated in #3.

But the coding standard changed for Drupal 8.

http://drupal.org/node/1354#functions

Data types on param/return

Note: This section of the standard was adopted as of Drupal 8. In previous versions, data type specification was recommended in the case of non-obvious types or specific classes/interfaces only.

The data type of a parameter or return value MUST be specified in @param or @return directives, if it is not obvious or of a specific class or interface type.

The data type MAY be specified (as in: recommended) for other @param or @return directives, including those with primitive data types, arrays, and generic objects (stdClass).

I am setting this to "needs work" as it seems we now should check if the data type for return is obvious. But I have no clue what "obvious" really means here. Is it obvios if its a very short function? How obvious is an array?

@Kars-T - Please see #5 here #1518116: [meta] Make Core pass Coder Review. It's clearly mentioned that "The one exception to the rule is for adding param/return datatypes which will be handled in #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*".

So changing the status back to 'needs review'.

Ah okay. Than I'd say it is RTBC but as my english is to weak to evaluate the comments I leave the status as it is :)

Postponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!

We have the go ahead with all these issues again, see #1518116: [meta] Make Core pass Coder Review for more details

I work on it

Here is a patch

Great start, thanks!

1. +++ b/core/modules/action/tests/src/Unit/Menu/ActionLocalTasksTest.php
   @@ -16,6 +16,9 @@
   +  /**
   +   * Overrides Tests\Core\Menu\LocalTaskIntegrationTest::setUp().
   +   */
   

   This should be "{@inheritdoc}". We are using this in favour of "Overrides xyz". See Comment standards - inheritdoc.

When scanning all files in the action module I still found 3 additional coding standards violations:

$ phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,js,css,info,txt ./core/modules/action

FILE: /home/pieter/v/drupal/drupal/core/modules/action/action.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 41 | ERROR | Inline doc block comments are not allowed; use "// Comment"
    |       | instead
--------------------------------------------------------------------------------


FILE: ...r/v/drupal/drupal/core/modules/action/src/Plugin/Action/EmailAction.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 161 | WARNING | Line exceeds 80 characters; contains 87 characters
--------------------------------------------------------------------------------


FILE: ...er/v/drupal/drupal/core/modules/action/src/Plugin/Action/GotoAction.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 53 | WARNING | Line exceeds 80 characters; contains 83 characters
--------------------------------------------------------------------------------

If you could have a look at those as well that would be great.

Thank you.

Remains GotoAction.php Line53, as we encounter an exception for function description
(@ee : https://www.drupal.org/node/1539712#comment-5937986)

Thanks for all the work here so far. See #1518116-86: [meta] Make Core pass Coder Review. This issue is postponed until the meta issue is either closed or reopened.

I've cleaned up the current actions module and attaching the patch file with the changes.

Sorry, the previous one was checked out from 8.0.x, the current one should be passed.

I'm putting it back to 8.0.x based on this started discussion and send my patch to review.
https://www.drupal.org/node/1518116

Sorry for everyone to make noise here, this patch file will work (i really hope)

All changes look good to me, accept for this one:

+++ b/core/modules/action/src/Plugin/migrate/source/d6/Action.php
@@ -30,7 +30,7 @@ public function query() {
-    );
+      );

Are you sure about this change?

I also get this:

FILE: /app/www/active/core/modules/action/action.module
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
41 | ERROR | Inline doc block comments are not allowed; use "/*
| | Comment */" or "// Comment" instead
----------------------------------------------------------------------

@daffie : which you're proposed Action.php change, I get a coder issue:

FILE: ...tive/core/modules/action/src/Plugin/migrate/source/d6/Action.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
33 | ERROR | [x] Line indented incorrectly; expected at least 6
| | spaces, found 4
----------------------------------------------------------------------

I'm closing the ticket because opened a new one here: #2567357: Make Action module pass Coder Review Please follow it there.

@tatarbj: It's not standard practice to arbitrarily close or duplicate issues. Valuable information, such as comments, subscribers, and relationships to other issues is all lost when you do. See Use the Issue Queue if you don't understand how it works. If you would like to contribute to this issue, please do so here. Thanks!

@tatarbj: I just saw #1816690-27: Make Ban module pass Coder Review. The point there was not to re-open closed issues. This issue was still open, so you needn't create a new one. In short: Keep open issues open until fixed. Once fixed, keep closed issues closed. :)

@TravidCarden i'm confused what should i do @alexpott asked me to close a similar issue and create a new one here: https://www.drupal.org/node/1816690#comment-10313705
Just to make sure: i shouldn't close issues and open a new one if i want to work on a 3+ years and postponed topic which can be solved by me like all of the d8 core modules what i started to do.
In this case i'm attaching the patch what i've created for the duplicated issue #2567357: Make Action module pass Coder Review

@tatarbj: The reason @alexpott asked you to open a new issue in #1816690-27: Make Ban module pass Coder Review is because that issue had already been closed. The patch had been committed. The issue was fixed. He was asking you not to re-open closed (fixed) issues. This issue, on the other hand, was already open; so it was appropriate to continue working in it. It should not have been marked fixed because the issue itself was not, in fact, fixed. Does that clear it up? The Status settings of issues handbook page may be of further help, if necessary.

Closing in favor of #2571965: [meta] Fix PHP coding standards in core. In this issue the coding standards will be fixed on a sniff-per-sniff basis rather than a module-per-module basis.