Part of #2571965: [meta] Fix PHP coding standards in core.

Approach

We are testing coding standards with PHP CodeSniffer, using the Drupal coding standards from the Coder module. Both of these packages are not installed in Drupal core. We need to do a couple of steps in order to download and configure them so we can run a coding standards check.

Step 1: Add the coding standard to the whitelist

Every coding standard is identified by a "sniff". For example, an imaginary coding standard that would require all llamas to be placed inside a square bracket fence would be called the "Drupal.AnimalControlStructure.BracketedFence sniff". There are dozens of such coding standards, and to make the work easier we have started by only whitelisting the sniffs that pass. For the moment all coding standards that are not yet fixed are simply skipped during the test.

Open the file core/phpcs.xml.dist and add a line for the sniff of this ticket. The sniff name is in the issue title. Make sure your patch will include the addition of this line.

Step 2: Install PHP CodeSniffer and the ruleset from the Coder module

Both of these packages are not installed by default in Drupal core, so we need to download them. This can be done with Composer, from the root folder of your Drupal installation:

$ composer require drupal/coder squizlabs/php_codesniffer
$ ./vendor/bin/phpcs --config-set installed_paths ../../drupal/coder/coder_sniffer

Once you have installed the phpcs package, you can list all the sniffs available to you like this:

$ ./vendor/bin/phpcs --standard=Drupal -e

This will give you a big list of sniffs, and the Drupal-based ones should be present.

Step 3: Prepare the phpcs.xml file

To speed up the testing you should make a copy of the file phpcs.xml.dist (in the core/ folder) and save it as phpcs.xml. This is the configuration file for PHP CodeSniffer.

We only want this phpcs.xml file to specify the sniff we're interested in. So we need to remove all the rule items, and add only our own sniff's rule. Rule items look like this:

<rule ref="Drupal.Commenting.DocComment.ParamGroup"/>

Remove all of them, and add only the sniff from this issue title. This will make sure that our tests run quickly, and are not going to contain any output from unrelated sniffs.

Step 4: Run the test

Now you are ready to run the test! From within the core/ folder, run the following command to launch the test:

$ cd core/
$ ../vendor/bin/phpcs -p

This takes a couple of minutes. The -p flag shows the progress, so you have a bunch of nice dots to look at while it is running.

Step 5: Fix the failures

When the test is complete it will present you a list of all the files that contain violations of your sniff, and the line numbers where the violations occur. You could fix all of these manually, but thankfully phpcbf can fix many of them. You can call phpcbf like this:

$ ../vendor/bin/phpcbf

This will fix the errors in place. You can then make a diff of the changes using git. You can also re-run the test with phpcs and determine if that fixed all of them.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jungle created an issue. See original summary.

jungle’s picture

Copy and paste from another issue, but I think

+ should be -

If so, all IS of commenting related child issues need updating.

jungle’s picture

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

longwave’s picture

jungle’s picture

Thanks @longwave! Attaching a test only patch to prove that the rule enabled can detected violations as expected.

jungle’s picture

Status: Needs review » Reviewed & tested by the community

12 to fix in total, 11 of them are fixed by simply removing an empty line (literally, non-empty lines, as they contain whitespaces and a *). the special one gets adjusted. So #5 is RTBC to me.

jungle’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, the number does not match. it's 14. reviewing again.

jungle’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/phpcs.xml.dist
    @@ -55,10 +55,7 @@
    -    <!-- TagsNotGrouped and ParamGroup have false-positives.
    -      @see https://www.drupal.org/node/2060925 -->
    

    Remove the comment is good, as #2060925 was in

  2. +++ b/core/tests/Drupal/Tests/UiHelperTrait.php
    @@ -142,11 +141,10 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
        * @param array $edit
        *   Field data in an associative array. Changes the current input fields
    -   *   (where possible) to the values indicated.
    -   *
    -   *   When working with form tests, the keys for an $edit element should match
    -   *   the 'name' parameter of the HTML of the form. For example, the 'body'
    -   *   field for a node has the following HTML:
    +   *   (where possible) to the values indicated. When working with form tests,
    +   *   the keys for an $edit element should match the 'name' parameter of the
    +   *   HTML of the form. For example, the 'body' field for a node has the
    +   *   following HTML:
    

    The adjustment here is good, no lines exceed 80 chars, and no word(s) should move to the previous/next line.

  3. +++ b/core/modules/field/src/EntityDisplayRebuilder.php
    @@ -70,7 +70,6 @@ public static function create(ContainerInterface $container) {
    -   *
    
    +++ b/core/modules/statistics/src/StatisticsStorageInterface.php
    @@ -54,7 +54,6 @@ public function fetchView($id);
    -   *
    
    +++ b/core/modules/system/tests/src/Functional/Form/LanguageSelectElementTest.php
    @@ -102,7 +102,6 @@ public function testHiddenLanguageSelectElement() {
    -   *
    
    +++ b/core/modules/user/src/Plugin/views/argument_default/User.php
    @@ -37,7 +37,6 @@ class User extends ArgumentDefaultPluginBase implements CacheableDependencyInter
    -   *
    
    +++ b/core/modules/user/user.module
    @@ -1023,7 +1023,6 @@ function user_role_revoke_permissions($rid, array $permissions = []) {
    - *
    
    +++ b/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
    @@ -1365,7 +1365,6 @@ protected function renderAltered($alter, $tokens) {
    -   *
    
    @@ -1681,7 +1680,6 @@ protected function getFieldTokenPlaceholder() {
    -   *
    
    @@ -1793,7 +1791,6 @@ public function adminLabel($short = FALSE) {
    -   *
    
    +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDecoupledTranslationRevisionsTest.php
    @@ -255,7 +255,6 @@ public function dataTestUntranslatableFields() {
    -   *
    
    +++ b/core/tests/Drupal/Tests/UiHelperTrait.php
    @@ -130,7 +130,6 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
    -   *
    
    @@ -156,7 +154,6 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
    -   *
    
  4. The rest are removing unnecessary "empty" lines.

xjm’s picture

+++ b/core/tests/Drupal/Tests/UiHelperTrait.php
@@ -130,7 +130,6 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
    *   path to NULL and have it post to the last received page. Example:
-   *

@@ -142,11 +141,10 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
    *   Field data in an associative array. Changes the current input fields
-   *   (where possible) to the values indicated.
-   *
-   *   When working with form tests, the keys for an $edit element should match
-   *   the 'name' parameter of the HTML of the form. For example, the 'body'
-   *   field for a node has the following HTML:
+   *   (where possible) to the values indicated. When working with form tests,

@@ -156,7 +154,6 @@ protected function submitForm(array $edit, $submit, $form_html_id = NULL) {
    *   @code
    *   $edit["body[0][value]"] = 'My test value';
    *   @endcode
-   *

So, hm, this rule seems to be wrong. These are legitmate newlines, to create new paragraphs for readability. I guess the false positives are the combination of the @code and blank lines?

If we have a look at UiHelperTrait::drupalPostForm(), the blank lines there help with readability. The code groups seem to add whitespace before but not after. I'd also argue the source is hard to read without symmetrical blank lines around the paragraph.

I think we might need to make this sniff smarter before we enable it. Leaving RTBC for the moment but wanted to note the concern.

longwave’s picture

Is this whole paragraph with the example too much to stuff into an @param, it could just be in the main body of the docblock instead I think.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs followup

@longwave That'd be a simple workaround, but I still think the sniff itself is wrong here. I can't remember whether multi-paragraph @param works on api.d.o, but I quickly found an example of a multi-paragraph return formatted correctly on api.d.do:
hook_menu_links_discovered_alter()

The sniff also is only flagging multiple paragraphs with @code blocks inside them, so it's a false positive. You can test this by:

  1. Apply the patch from this issue.
  2. Apply the following diff:
    diff --git a/core/modules/node/node.module b/core/modules/node/node.module
    index eae4365c79..1b417a831e 100644
    --- a/core/modules/node/node.module
    +++ b/core/modules/node/node.module
    @@ -151,6 +151,10 @@ function node_entity_view_display_alter(EntityViewDisplayInterface $display, $co
      *   query joins the {comment_entity_statistics} table so that the comment_count
      *   field is available, a title attribute will be added to show the number of
      *   comments.
    + *
    + *   Other paragraph.
    + *
    + *   More paragraphs.
      * @param $title
      *   (optional) A heading for the resulting list.
      *
    @@ -179,6 +183,12 @@ function node_title_list(StatementInterface $result, $title = NULL) {
      *
      * @param int $nid
      *   Node ID whose history supplies the "last viewed" timestamp.
    + *
    + *   Try this:
    + *
    + *   @code
    + *     Hi.
    + *   @endcode
      * @param int $timestamp
      *   Time which is compared against node's "last viewed" timestamp.
      *
    
  3. Run:
    composer run phpcs -- -p core/modules/node/node.module
    
  4. The result is:
    FILE: /Users/xjm/git/drupal/core/modules/node/node.module
    ----------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    ----------------------------------------------------------------------
     192 | ERROR | Parameter tags must be grouped together in a doc
         |       | comment
    ----------------------------------------------------------------------
    
    Time: 294ms; Memory: 16MB
    

Since it only flags one of the changes and not both, I think this means the sniff needs work. So, I think we should file an issue in the coder queue, and then postpone this issue on fixing the sniff. Setting NR to look into that. Not so much as "Needs followup" as "Needs blocker" but you get the idea. :)

daffie’s picture

Status: Needs review » Needs work

The testbot returns with the following coding standard messages:

/var/lib/drupalci/workspace/jenkins-drupal_patches-46232/source/core/modules/field/src/EntityDisplayRebuilder.php
line 74	Parameter tags must be grouped together in a doc comment
/var/lib/drupalci/workspace/jenkins-drupal_patches-46232/source/core/modules/statistics/src/StatisticsStorageInterface.php
58	Parameter tags must be grouped together in a doc comment
/var/lib/drupalci/workspace/jenkins-drupal_patches-46232/source/core/modules/system/tests/src/Functional/Form/LanguageSelectElementTest.php
106	Parameter tags must be grouped together in a doc comment
/var/lib/drupalci/workspace/jenkins-drupal_patches-46232/source/core/modules/user/src/Plugin/views/argument_default/User.php
41	Parameter tags must be grouped together in a doc comment
/var/lib/drupalci/workspace/jenkins-drupal_patches-46232/source/core/modules/user/user.module
1027	Parameter tags must be grouped together in a doc comment
1030	Parameter tags must be grouped together in a doc comment
/var/lib/drupalci/workspace/jenkins-drupal_patches-46232/source/core/modules/views/src/Plugin/views/field/FieldPluginBase.php
1369	Parameter tags must be grouped together in a doc comment
1685	Parameter tags must be grouped together in a doc comment
1797	Parameter tags must be grouped together in a doc comment
/var/lib/drupalci/workspace/jenkins-drupal_patches-46232/source/core/tests/Drupal/KernelTests/Core/Entity/EntityDecoupledTranslationRevisionsTest.php
259	Parameter tags must be grouped together in a doc comment
/var/lib/drupalci/workspace/jenkins-drupal_patches-46232/source/core/tests/Drupal/Tests/UiHelperTrait.php
143	Parameter tags must be grouped together in a doc comment
169	Parameter tags must be grouped together in a doc comment
176	Parameter tags must be grouped together in a doc comment
178	Parameter tags must be grouped together in a doc comment

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs followup
FileSize
5.91 KB

The @code issue appears to be fixed upstream in Coder, there are not many fixes needed in order to enable this sniff now.

Spokje’s picture

Status: Needs review » Reviewed & tested by the community
  • When I run this specific sniff, I get the same errors the @longwave made changes for.
  • Testbot likes it.
  • Looked at the changes and they make sense to me.

RTBC for me.

alexpott’s picture

Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 93084ba and pushed to 9.2.x. Thanks!
Committed 0d2a8ca and pushed to 9.1.x. Thanks!

Backported everything apart from the phpcs.xml.dist changes to 9.1.x to keep the code aligned.

  • alexpott committed 93084ba on 9.2.x
    Issue #3123058 by longwave, jungle, xjm, daffie: Fix 'Drupal.Commenting....

  • alexpott committed 0d2a8ca on 9.1.x
    Issue #3123058 by longwave, jungle, xjm, daffie: Fix 'Drupal.Commenting....

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.