Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NROTC_Webmaster’s picture

Status: Active » Postponed
Lars Toomre’s picture

Status: Postponed » Needs work
Issue tags: -Novice

The patch in #1326672-66: Clean up API docs for menu module is getting close to commit. Thus, I am turning my attention to this sub-issue of making sure that Menu module now passes Coder review. Further, I have opened #1800744: Make file can be problematic to address the missing type hinting of @param and @return directives.

I am unable to run the correct Coder review of what is in the Menu module and flagged as being an issue. Could someone do so and post here what is reported so we all have some idea of what is still required to make the Menu module compliant with D8 coding standards?

On a related note, I have created and posted patches to #1803656: Improve Tests consistency to standards in modules A-M and #1804522: Improve Tests consistency to standards in modules N-Z that address a number of issues in the D8 Test classes. It would be great in those two issues could be reviewed and committed soon, as it will reduce the number of items that will need to be corrected in each of these sub-issues.

Also, note I have removed the 'Novice' tag since interpreting the flagged items report requires some understanding of what is a false positive and what legitimately needs to be corrected.

TravisCarden’s picture

Status: Needs work » Postponed

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

TravisCarden’s picture

Issue summary: View changes
Status: Postponed » Active
majoely’s picture

Here is my patch for the menu module with the following issues:
24 | ERROR | Missing function doc comment
that are because the getInfo() and setUp() functions don't have comments. I am not sure if there is a standard comment for those that should go there because looking through the rest of the core modules the ones I looked at had the same issue.
* Implements hook_block_view_BASE_BLOCK_ID_alter() for system_menu_block().
I changed a comment to remove an error to the code above, but I think it might be refering to a calss in the system module, if so I'm not sure what the appropriate comment is.
There were a few errors about the naming convention of variables that I haven't fixed yet.
48 | WARNING | Line exceeds 80 characters; contains 83 characters
* @param \Drupal\menu_link\MenuLinkStorageControllerInterface $menu_link_storage
Finnally, is there a way of getting around this warning or is it to be left?
A list of remaining error is attached as drupalcs.txt
Cheers,

majoely’s picture

Status: Active » Needs review
majoely’s picture

Status: Needs review » Needs work
majoely’s picture

Assigned: Unassigned » majoely
majoely’s picture

Assigned: majoely » Unassigned
Status: Needs work » Needs review
FileSize
30.43 KB
9.38 KB

Fixed all the errors from the coder review by having a better look at the coding standards comment section.
The only one that may pop up is:

 700 | WARNING | Format should be "* Implements hook_foo().", "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar().", or "* Implements
     |         | hook_foo_BAR_ID_bar() for xyz_bar.tpl.php.".
--------------------------------------------------------------------------------

and I am still not sure I did that correctly.

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Priority: Normal » Minor
Status: Needs review » Postponed

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.

tatarbj’s picture

Status: Postponed » Closed (duplicate)

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.