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

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.TagGroupSpacing"/>

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.

CommentFileSizeAuthor
#20 2937513-8.9.x-20.patch55.11 KBlongwave
#14 2937513-14.patch51.58 KBlongwave
#2 2937513-2.patch102.84 KBeltori

Comments

eltori created an issue. See original summary.

eltori’s picture

eltori commented
Assigned: eltori » Unassigned
Status: Active » Needs review
StatusFileSize
new 2937513-2.patch102.84 KB

Initial patch version.

rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented
Status: Needs review » Reviewed & tested by the community

Applied patch, ran $ composer run phpcs - no issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -129,7 +129,8 @@ class DateTimePlus {
    *   A DateTime object.
    * @param array $settings
-   *   @see __construct()
+   *
+     * @see __construct()
    *

This doesn't look right to me - i.e. it looks like a bug in the phpcs rule.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Postponed

Postponing this on #2947589: TagGroupSpacing coding space fix doesn't work as it should

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

I just released a new Coder version where the wrong fixes from above should not happen again. This is now postponed on #3063323: Update drupal/coder to 8.3.6.

idebr’s picture

idebr commented

#3063323: Update drupal/coder to 8.3.6 has been committed, but the sniff still generates a few false positives. I have documented these on #2947589: TagGroupSpacing coding space fix doesn't work as it should

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Postponed » Active

Pushed a fix to Coder, please test with the Coder dev version here if we have resolved everything now.

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

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

nginex’s picture

nginex commented
Issue tags: +LutskGCW20

Tagging for Drupal Global Contribution Weekend

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

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 2937513-14.patch51.58 KB

phpcbf seemed to do a good job on this one.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

Nice, thanks!

* Checked that the patch only contains the comment empty lines changes
* Rule exclude in PHPCS config is removed
* PHPCS output on testbot is clean

  • xjm committed 0f66cce on 9.1.x 
    Issue #2937513 by eltori, longwave, klausi, catch, idebr: Fix 'Drupal....

  • xjm committed c77e219 on 9.0.x 
    Issue #2937513 by eltori, longwave, klausi, catch, idebr: Fix 'Drupal....
xjm’s picture

xjm
she/her
Primary language English
commented
Title: Fix 'Drupal.Commenting.DocComment.TagGroupSpacing' coding standard » [backport] Fix 'Drupal.Commenting.DocComment.TagGroupSpacing' coding standard
Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs followup, +beta target, +9.0.0 release notes 
  1. +++ b/core/lib/Drupal/Core/Entity/Query/ConditionAggregateInterface.php
@@ -44,6 +45,7 @@ public function exists($field, $function, $langcode = NULL);
    * @return ConditionInterface
    * @see \Drupal\Core\Entity\Query\QueryInterface::notExists()

    Interesting that it's detecting the missing newlines between @param and @return, or @return and @throws, but not between @return and @see. 

  2. +++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/TestDerivativeDiscoveryWithObject.php
@@ -13,6 +13,7 @@ class TestDerivativeDiscoveryWithObject implements DeriverInterface {
    * {@inheritdoc}
    * @param string $derivative_id
    * @param array $base_plugin_definition

    Also interesting that it doesn't detect the missing newline between the one-line summary and @param.

Is there a different rule that will catch those things? If so, we can wait for that rule; if not, we need a followup coder issue to expand the scope of the sniff a bit.

Meanwhile, this is successfully fixing the scope of "missing newline between @param/@return/@throws". I reviewed to ensure all changes were fixing that, and nothing else. I also ran composer run phpcs -- -p with the patch applied on 9.1.x to verify the rule was entirely fixed. Did likewise on 9.0.x after cherry-picking.

Tagging for the release notes as a newly enabled standard in 9.0. This is also eligible for backport to 8.9 still, but the patch did not cherry-pick cleanly. The backport should also be checked to ensure it fixes all cases on 8.9.x.

Thanks!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Haha, I read the issue comment notification email but didn't catch the author and by the end of it I knew this could only be an xjm comment. I love your rigor!

1. @see tag line spacing: I saw some usage of @see tags within @return or @param tags, so decided to not enforce any spacing on @see tags as they are a bit special.

2. Summary spacing: I think Coder might not complain here because this is an {@inheritdoc} comment, which is also treated as special case in Coder.

We can open issues for those in Coder to improve that, I personally think they are not that important. Coder pull requests welcome anyway!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 2937513-8.9.x-20.patch55.11 KB

Rerolled for 8.9.x by running phpcbf again.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community

Thanks! Verified the changes in the patch and that the phpcs.xml.dist change is also there.

PHPCS output is clean on the testbot.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: [backport] Fix 'Drupal.Commenting.DocComment.TagGroupSpacing' coding standard » Fix 'Drupal.Commenting.DocComment.TagGroupSpacing' coding standard
Status: Reviewed & tested by the community » Fixed

Committed 4f61b6f and pushed to 8.9.x. Thanks!

  • alexpott committed 4f61b6f on 8.9.x 
    Issue #2937513 by longwave, eltori, klausi, catch, idebr, xjm: Fix '...

Status: Fixed » Closed (fixed)

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

jonathan1055’s picture

jonathan1055 commented

I have raised #3183656: Fix 'Drupal.Commenting.DocComment.TagGroupSpacing' coding standard [part 2] as a follow-up

jonathan1055’s picture

jonathan1055 commented
Related issues: +#3183656: Fix 'Drupal.Commenting.DocComment.TagGroupSpacing' coding standard [part 2]

Following up xjm's comments in #18 and Klausi's answers:

#18.1 The adjustment I have made to this sniff now fixes this, and a blank line is required either side of the @eturn regardless of what is before or after it. This was always the intention but was not achieving it in practice.

#18.2 The missing blank line in this case is detected by another sniff Drupal.Commenting.DocComment.SpacingBeforeTags which reports "There must be exactly one blank line before the tags in a doc comment" and is dealt with in #2842949: Fix Drupal.Commenting.DocComment.SpacingBeforeTags coding standard

So #3183656: Fix 'Drupal.Commenting.DocComment.TagGroupSpacing' coding standard [part 2] is now ready for final review.