This is part of meta-issue #1518116: [meta] Make Core pass Coder Review focused on the new D8 Action module.

#3 coder_issues-action-2012_10_22.patch3.4 KBamitgoyal
PASSED: [[SimpleTest]]: [MySQL] 42,810 pass(es).
[ View ]
#3 coder_issues_action_2012_10_22.txt10.51 KBamitgoyal


#1802070: Add missing type hinting to Action module docblocks is open to deal separately with any missing type hinting from @param and/or @return directives.

Assigned:Unassigned» amitgoyal
Status:Active» Needs review
new10.51 KB
new3.4 KB
PASSED: [[SimpleTest]]: [MySQL] 42,810 pass(es).
[ View ]

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.

Status:Needs review» Needs work

The patch fixes the issues stated in #3.

But the coding standard changed for Drupal 8.

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?

Status:Needs work» Needs review

@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".

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

Thanks for the patch @amitgoyal. I hope to do a detailed review of this later today.

If you have not seen the comment yet, please take a look at what I added in #1816690-7: Make Ban module pass Coder Review. I would love to see similar reports to what is in #3 for the Node, System and/or Taxonomy modules.

The other thing that would be helpful is to generate a separate patch for #1802070: Add missing type hinting to Action module docblocks that addresses all of the type hinting for @param and @return directives that are highlighted in the report in #3, but not included in the patch for this issue.

As you might be aware, performing a detailed review of type hinting patches for @param and @return directives takes quite a bit of time. Hence, they are being broken out separately, and in the case of modules needing many additions, being added in several incremental patches.

This past weekend I spent considerable time performing such reviews of patches for the Overlay, PHP, Poll, Syslog, Tracker, and Update modules. (Each of those patches were generated without the benefit of an excellent automated report like that in #3).

I would be happy to review any such similar patch(es) that might be generated for the Action module.

Status:Needs review» Postponed

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

Status:Postponed» Active

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