There were some coding standards issues identified while testing the patch at https://www.drupal.org/node/2804195

➜  drupalcon git:(8.3.x) ✗ phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md core/modules/views/src/Plugin/Derivative/ViewsLocalTask.php

FILE: ...lcon/core/modules/views/src/Plugin/Derivative/ViewsLocalTask.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  74 | ERROR | [ ] Inline doc block comments are not allowed; use "/*
     |       |     Comment */" or "// Comment" instead
 113 | ERROR | [ ] Inline doc block comments are not allowed; use "/*
     |       |     Comment */" or "// Comment" instead
 154 | ERROR | [x] Separate the @return and @code sections by a blank
     |       |     line.
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 73ms; Memory: 7.5Mb



➜  drupalcon git:(8.3.x) ✗ phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md core/modules/views/tests/src/Unit/Plugin/Derivative/ViewsLocalTaskTest.php

FILE: ...es/views/tests/src/Unit/Plugin/Derivative/ViewsLocalTaskTest.php
----------------------------------------------------------------------
FOUND 5 ERRORS AND 1 WARNING AFFECTING 6 LINES
----------------------------------------------------------------------
  35 | ERROR   | [ ] Missing short description in doc comment
  42 | WARNING | [x] A comma should follow the last multiline array
     |         |     item. Found:
     |         |     '\Drupal\views\Plugin\Derivative\ViewsLocalTask'
  52 | ERROR   | [x] Missing function doc comment
 142 | ERROR   | [ ] If the line declaring an array spans longer than
     |         |     80 characters, each element should be broken
     |         |     into its own line
 244 | ERROR   | [ ] If the line declaring an array spans longer than
     |         |     80 characters, each element should be broken
     |         |     into its own line
 313 | ERROR   | [ ] If the line declaring an array spans longer than
     |         |     80 characters, each element should be broken
     |         |     into its own line
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 133ms; Memory: 10.25Mb
CommentFileSizeAuthor
#5 views_coding_standards-2809251-5.patch4.12 KBmsti
#3 views_coding_standards-2809251-3.patch4.13 KBmsti

Comments

msti created an issue. See original summary.

msti’s picture

msti
Location Heraklion
commented

Working on the issue at the mentored Sprints @ DC Dublin

msti’s picture

msti
Location Heraklion
commented
Status: Active » Needs review
StatusFileSize
new views_coding_standards-2809251-3.patch4.13 KB

This patch fixes the coding issues described above

bserem’s picture

bserem commented
Status: Needs review » Needs work

Patch is fine, phpcs output is clear after it.

Though, per this guide: https://www.drupal.org/node/1354#inline
it is suggested to use // instead of /*.
I would advise to change it and then we can RTBC it.

C style comments (/* */) and standard C++ comments (//) are both fine, though the former is discouraged within functions

msti’s picture

msti
Location Heraklion
commented
StatusFileSize
new views_coding_standards-2809251-5.patch4.12 KB

Re-rolling patch

bserem’s picture

bserem commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +Dublin2016

Patch in #5 applies nicely and outputs clean results in phpcs.
Since it introduces no new code I say it is safe.

jp.stacey’s picture

jp.stacey commented

I can confirm that the patch in #5 applies cleanly and fixes all the phpcs issues mentioned. Great work!

However, I think the syntax /** @var has a special status: as an inline docblock, it isn't stripped by APC, and can be parsed by phpDocumenter. Also I think it's used by IDEs that check variable type, to ensure that the variable mentioned on the next line is of the type mentioned. (From the looks of it, though, this usefulness is already being ignored, as the first /** @var $executable in the patch is not immediately before the variable $executable, so it's clearly of limited of use.)

With this in mind, we need to agree to either:

1. change it to // @var here (will IDEs parse that?)
2. change the coding standard to permit the inline docblocks.

Would really appreciate any thoughts!

dawehner’s picture

dawehner
Primary language German
commented
Status: Reviewed & tested by the community » Closed (won't fix)

Thank you for your work on cleaning up Drupal core's code style!

In order to fix core coding standards in a maintainable way, all our coding standards issues should be done on a per-rule basis across all of core, rather than fixing standards in individual modules or files. We should also separate fixes where we need to write new documentation from fixes where we need to correct existing standards. This all should be done as part of #2571965: [meta] Fix PHP coding standards in core, stage 1. A good place to start is the child issues of #2572645: [Meta] Fix 'Drupal.Commenting.FunctionComment' coding standard.

For background information on why we usually will not commit coding standards fixes that aren't scoped in that way, see the core issue scope guidelines, especially the note about coding standards cleanups. That document also includes numerous suggestions for scoping issues including documentation coding standards cleanups.

Contributing to the overall plan above will help ensure that your fixes for core's coding standards remain in core the long term.

I'm going to close this issue - migrate is part of core and should not be dealt with separately.